Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle][HttpKernel] Fix resources loading for bundles with custom structure#21113
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
ogizanagi commentedDec 31, 2016
I hope this can be accepted as a bugfix rather than a new feature though :/ |
xabbuh commentedDec 31, 2016
The minimum required version of the HttpKernel component in the FrameworkBundle must be updated from |
a49e87d to6c87d77Comparechalasr commentedDec 31, 2016
@xabbuh thanks, done |
| $reflection =new \ReflectionClass($class); | ||
| if (is_dir($dir =dirname($reflection->getFileName()).'/Resources/translations')) { | ||
| foreach ($container->getParameter('kernel.bundles_metadata')as$name =>$bundle) { | ||
| if (is_dir($dir =dirname($bundle['path']).'/Resources/translations')) { |
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.
Callingdirname() here (and in all places below) looks wrong to me as thepath key should already just refer to directory in which the bundle is stored.
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.
indeed, fixed
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.
Maybe it would be good to have a dedicated test case working with a bundle that follows the default behaviour (i.e. extending the baseBundle class without overriding thegetPath() method) to detect a similar issue in the future?
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.
Sorry, I missed this comment. I think the test you have in mind already exists, the one added here is a copy paste:https://github.com/symfony/symfony/pull/21113/files#diff-b0c828e732d3d31f0b2bfb2378bd1011R340
Correct me if I'm wrong
1582a32 to332aeb8Comparechalasr commentedDec 31, 2016
Tests are green |
| foreach ($this->bundlesas$name =>$bundle) { | ||
| $bundles[$name] =get_class($bundle); | ||
| $bundlesMetadata[$name] =array('parent' =>$bundle->getParent(),'path' =>$bundle->getPath(),'namespace' =>$bundle->getNamespace()); |
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.
To make this more readable I would change the code style to this:
$bundlesMetadata[$name] =array('parent' =>$bundle->getParent(),'path' =>$bundle->getPath(),'namespace' =>$bundle->getNamespace(),);
xabbuh commentedDec 31, 2016
👍 Status: Reviewed |
fc6afb3 to278186fComparea4b54c0 toe29fdf9Comparexabbuh commentedJan 4, 2017
ping @symfony/deciders |
stof commentedJan 4, 2017
This should also update the TwigBundle path registration |
cd5c429 to6ba13a9Comparechalasr commentedJan 4, 2017
@stof done |
| "twig/twig":"~1.28|~2.0", | ||
| "symfony/http-foundation":"~2.5", | ||
| "symfony/http-kernel":"~2.7" | ||
| "symfony/http-kernel":"~2.7.23" |
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 forbids using 2.8, you should add|~2.8.16.
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.
fixed
…verriding getPath()
6ba13a9 tofef3146Comparefabpot commentedJan 4, 2017
Thank you@chalasr. |
…ndles with custom structure (chalasr)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle][HttpKernel] Fix resources loading for bundles with custom structure| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18563| License | MIT| Doc PR | n/aThis fixes twig/translator/validator/serializer resource loading for bundles overriding `Bundle::getPath()`, adding a kernel parameter containing the bundle metadata (i.e. `path`, `namespace` and `parent`).Fixes#18563 and unlocks#19586Commits-------fef3146 Fix serializer/translations/validator resources loading for bundles overriding getPath()
Uh oh!
There was an error while loading.Please reload this page.
This fixes twig/translator/validator/serializer resource loading for bundles overriding
Bundle::getPath(), adding a kernel parameter containing the bundle metadata (i.e.path,namespaceandparent).Fixes#18563 and unlocks#19586