8000 [9.x] Removed action from SymfonyRoute options by TomHAnderson · Pull Request #35542 · laravel/framework · GitHub
[go: up one dir, main page]
Skip to content

[9.x] Removed action from SymfonyRoute options #35542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 9, 2020
Merged

[9.x] Removed action from SymfonyRoute options #35542

merged 1 commit into from
Dec 9, 2020

Conversation

TomHAnderson
Copy link
Contributor

This is a master PR of this original PR: #35451
The text below is copied from that original PR.

Description

Illuminate\Routing\Route::toSymfonyRoute() included two options: utf8 and action. The options do not include a compiler_class option so the default Symfony\Component\Routing\RouteCompiler will always be used. This compiler does not utilize an action parameter and, because you cannot override the RouteCompiler, I have removed that option from the toSymfonyRoute() options array.

Unit Tests

This PR removes unused code so no new unit tests would be useful. Removing the ephemera and testing with the current unit tests shows this does not affect the framework.

Discussion

Removing this unused (in Symfony) code from the Route simplifies the toSymfonyRoute() method and prevents developers from going on a wild goose chase to figure out why this code is there in the first place. There is no direct benefit to the end consumer.

The extraneous option was sent to Symfony classes so there are no Laravel unit tests which could apply to the call to the constructor of the Symfony Route class.

This change benefits the developer community around Laravel by only including code which has a purpose.

External Resources

This change is directed against the constructor of https://github.com/symfony/symfony/blob/5.x/src/Symfony/Component/Routing/Route.php#L53
and involves the route compiler https://github.com/symfony/symfony/blob/5.x/src/Symfony/Component/Routing/RouteCompiler.php
Additionally the valid options are listed in the comments of the constructor: https://github.com/symfony/symfony/blob/5.x/src/Symfony/Component/Routing/Route.php#L41

@TomHAnderson TomHAnderson changed the title [wip] Removed action from SymfonyRoute options [master] Removed action from SymfonyRoute options Dec 8, 2020
@GrahamCampbell GrahamCampbell change 8000 d the title [master] Removed action from SymfonyRoute options [9.x] Removed action from SymfonyRoute options Dec 8, 2020
@taylorotwell taylorotwell merged commit 6b05546 into laravel:master Dec 9, 2020
This was referenced Jul 13, 2021
Merged
Merged
@felixdorn felixdorn mentioned this pull request Sep 19, 2021
Closed
@otilor otilor mentioned this pull request Jan 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driesvints driesvints driesvints approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TomHAnderson @driesvints @taylorotwell
0