Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] Fix resource miss#24642
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
1e00afb to1dd4f8fCompare| $subCollection->addPrefix($this->prefix); | ||
| $routeCollection->addCollection($subCollection); | ||
| foreach ($route->resourcesas$resource) { |
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.
shouldn't this be handled by$routeCollection->addCollection instead ?
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 doesn't work because$route->build() (https://github.com/symfony/symfony/pull/24642/files#diff-61a7c46324774b76b342efb3f3e87b7fL310) doesn't add sub-resources to$subCollection. This is exactly what this patch 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.
it is adding them just a few lines below in this code.
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.
$route->resources vs$this->resources
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.
While you are on this file... it looks like nothing calls the private methodaddResource, so maybe you can remove theresources property of that class, as well as the loop "below".
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.
sure. But the loop below happens inside->build() and so the subcollectionwill have the resources in it if it has any.
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.
But it’s not the case (the new test fail without my patch). I’ll Investigate this afternoon.
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 double checked and pushed a better fix:
$route can have resources but no routes. So the loop over$this->resources must be outside the loop on$this->routes.
Can you check if it's ok@stof?
fabpot commentedOct 20, 2017
Thank you@dunglas. |
This PR was squashed before being merged into the 2.8 branch (closes#24642).Discussion----------[Routing] Fix resource miss| Q | A| ------------- | ---| Branch? | 2.8| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aSome routing resources are not watched. To reproduce:1. Install Symfony 42. Change something in `config/routes.yaml`The change is not taken into account.This PR fixes this bug.Commits-------6610c25 [Routing] Fix resource miss
Some routing resources are not watched. To reproduce:
config/routes.yamlThe change is not taken into account.
This PR fixes this bug.