-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Routing] Fixed priority getting lost when setting localized prefix #52913
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.
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
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
5a1519e
to
347a471
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm annoyed by the need for adding a method but I don't see any other non-hacky way.
Since this issue exists in 5.4, can you please rebase/retarget the PR for 5.4?
Thx. I will rebase tommorow. |
347a471
to
a17c4df
Compare
a17c4df
to
d9d6695
Compare
src/Symfony/Component/Routing/Tests/Fixtures/AttributeFixtures/RouteWithPriorityController.php
Show resolved
Hide resolved
src/Symfony/Component/Routing/Tests/Fixtures/AttributeFixtures/RouteWithPriorityController.php
Outdated
Show resolved
Hide resolved
{ | ||
new LoaderResolver([ | ||
$loader = new YamlFileLoader(new FileLocator(\dirname(__DIR__).'/Fixtures/localized')), | ||
new class() extends AttributeClassLoader { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class doesn't exist in 5.4.
Maybe we can define the routes using plain yaml or xml instead so that we don't have to deal with this concern (migrating from annotations to attributes when merging up ;) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thats a good idea. I will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to docs priority can be set only in annotations or attributes. So I don't see a clear way how to test this easily on 5.4 and 6 without changing the code between those
In YAML and XML you can move the route definitions up or down in the configuration file to control their priority. In routes defined as PHP attributes this is much harder to do, so you can set the optional priority parameter in those routes to control their priority
d9d6695
to
47dfa69
Compare
d9d6695
to
47dfa69
Compare
4133cd7
to
6360deb
Compare
6360deb
to
ba41175
Compare
Thank you @SystematicCZ. |
When a route prefix is an array, the original route is removed from the collection and then re-added with a new name and path. During this process, the route's priority is lost as it's not preserved during the removal and re-addition.
Current state of
PrefixTrait.php
To resolve this issue, I implemented a change where the priority of the route is stored before removal. This stored priority is then reassigned to the route when it's added back to the collection.