Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedDec 6, 2023
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution 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 to347a471CompareThere 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. |
src/Symfony/Component/Routing/Tests/Fixtures/AttributeFixtures/RouteWithPriorityController.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Routing/Tests/Fixtures/AttributeFixtures/RouteWithPriorityController.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| { | ||
| new LoaderResolver([ | ||
| $loader = new YamlFileLoader(new FileLocator(\dirname(__DIR__).'/Fixtures/localized')), | ||
| new class() extends AttributeClassLoader { |
nicolas-grekasJan 30, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
SystematicCZJan 30, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 to47dfa69Compare4133cd7 to6360debCompareThank you@SystematicCZ. |
Uh oh!
There was an error while loading.Please reload this page.
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.phpTo 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.