[TwigBundle] Fix bug where namespaced paths don't take parent bundles in account #19586

wesleylancel · 2016-08-10T08:00:18Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#6919
License	MIT
Doc PR

Currently namespaced paths for templates such as {% extends '@App/Layout/layout.html.twig' %} do not work with bundles that have overruled templates using the getParent() method in another bundle. See attached ticket. This change prepends the path of the bundle implementing getParent() to the paths of the namespace of bundle returned as a parent.

jakzal · 2016-08-10T08:38:29Z

We'll need a test case for this.

wesleylancel · 2016-08-10T09:27:11Z

@jakzal I modified the TwigExtensionTest to include a test case for this situation.

wesleylancel · 2016-08-10T09:46:07Z

AppVeyor ran on an outdated commit, might have to be restarted.

ro0NL · 2016-08-10T10:52:35Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php

@@ -92,6 +92,13 @@ public function load(array $configs, ContainerBuilder $container)
            $dir = dirname($reflection->getFileName()).'/Resources/views';
            if (is_dir($dir)) {
                $this->addTwigPath($twigFilesystemLoaderDefinition, $dir, $bundle);
+
+                $classMock = new $class();


This is not really a mock :) what about $object or just $bundle? However im not sure we should couple this here, this way. But i guess for now we dont have much choice...

I'll rename the variable. Don't think there are any options to retrieve the parent in this situation.

Maybe you could call registerBundles once =/ Basically new $class() is somewhat fragile.

I agree, and this won't work with a third level (ChildChildTwigBundle). How would you recommend calling registerBundles here? Is there any way to reach the kernel?

kernel service is out of scope, so i guess not. Maybe you can kick in at TwigBundle::createContainerExtension, without breaking BC.

This is somewhat related to #18563 in terms of improving the ecosystem.

What about using reflection and newInstanceWithoutConstructor() here?

@xabbuh some bundles may use constructor arguments in their getParent() method.
I tried to solve a similar issue in #18579

That's true, but the only ways to solve that issue completely would be to either pass bundles to extension classes or to write some logic that determines the paths during cache warmup.

Couldn't a parameter returning the bundle map be envisaged?

The issue is that you still would not have access to actual instances of the bundles (so this would not solve #19586 (comment)).

xabbuh · 2016-12-28T17:18:08Z

@wesleylancel Do you think you can finish this one?

wesleylancel · 2016-12-28T18:17:28Z

@xabbuh: I wish, as this is something that would be extremely useful for us. But, as pointed out by @ro0NL, bundles can extend infinitely A > B > C > D. My PR currently only checks for one level of extending. Ideally, I would be able to access the bundle map that is part of the kernel, but that's not available at this stage yet. If you could point me towards a possible solution I'd be happy to adjust my code accordingly.

ro0NL · 2016-12-28T20:53:36Z

Seeing the related ticket this goes back to 2013. And quite some issues are involved. I've not experienced it myself, but the template problem seems real.

Either way allowing kernel introspection would be extremely useful for everybody. Preferably with an immutable kernel. Practically we can move this a few lines up. Relying on a synthetic service before compilation.

We also can expose bundles thru parameters, we only need stringish values here. BundleInterface only defines name, path and namespace anyway. So passing a stringish bundle map could work, and seems to make sense as we also provide kernel info already. Any further details can be provided with Bundle::build.

But mayve @ogizanagi has more idea's? As i also liked his approach with a contextual builder. Maybe it can be simplified.

ogizanagi · 2016-12-28T21:21:21Z

Unfortunately #19646 was my only attempt to provide a naive feature for sharing some contextual information while the container is building, avoiding the need to rely on pseudo tmp synthetic services. Along with the BootingKernel and BootingBundle classes, it allowed to solves issues related to this one as a demonstration of the possibilities.
But despite the implementation is simple, it somehow looks overkilled for the few needs.

As you suggested, maybe exposing a simple representation of the bundle map and bundles informations in a kernel parameter could be the way. But I don't remember all the ins and outs and if it was easily feasible at this point. :/

ro0NL · 2016-12-28T21:29:35Z

But despite the implementation is simple, it somehow looks overkilled for the few needs.

Agree 👍 I feel the same about my own attempts now. Eventually having a builder with context can be done in userland easily. But it doesnt solve it for SF.

If there arent any other insights, i'd say go for %kernel.bundle_map% 👍

chalasr · 2016-12-29T11:38:01Z

i'd say go for %kernel.bundle_map%

Not sure it's doable. Putting Kernel::$bundleMap in a new kernel parameter would just lead to

Unable to dump a service container if a parameter is an object or a resource.

ro0NL · 2016-12-29T11:43:52Z

It's a stringish map, that's actually cached :) we're not passing bundle instances around here.

array(
  'name' => ['parent' => 'name2', 'namespace' => 'NS', 'path' => '/bundles/name' ],
  'name2' => //
)

Something like that i guess.

I mean.. this would lead to a nested <argument> collection in XML right?

ro0NL · 2016-12-29T12:11:58Z

Ie. looking at the issues, im not really sure we need true bundle instances here. Only metadata from bundle introspection. To build things properly.

I truly dont hope we're looking for ways to do this since 2013 😆 as it would definitely require the BootingKernel idea from @ogizanagi..

This makes the template compatible with projects disabling the Templating component in recent Symfony versions.

xabbuh · 2017-01-06T09:02:35Z

👍 I checked this with the issues described in FriendsOfSymfony/FOSUserBundle#2376 and FriendsOfSymfony/FOSUserBundle#2378 and I can confirm that this patch fixes them.

Status: Reviewed

maxime-pasquier · 2017-01-06T09:08:37Z

@xabbuh great news! Could you tell us on which branches this patch will be applied ?

chalasr · 2017-01-06T09:11:31Z

@Kmelia Once merged, this will be available in the next patch releases of each maintained branch (i.e. from 2.7 to 3.2) and the current development version (3.3).

stof · 2017-01-06T09:07:54Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php

@@ -97,6 +97,10 @@ public function load(array $configs, ContainerBuilder $container)
            if (is_dir($dir = $bundle['path'].'/Resources/views')) {
                $this->addTwigPath($twigFilesystemLoaderDefinition, $dir, $name);
            }
+
+            if (null !== $bundle['parent']) {
+                $this->prependParentPaths($twigFilesystemLoaderDefinition, $container, $name, $bundle['parent']);


Does this handle a chain of parents ?

@stof: Yes, this handles that. Should be covered with the changed test.

prependParentPaths was confusing at first. Maybe keep actual path registration explicit in load()

while ($parent) { // add parent } // add self

Basically we dont need any path prepending or recursion, only adding paths in the right calling order.

Maybe utilize with addBundlePaths or so.

stof · 2017-01-06T09:09:04Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php

@@ -97,6 +97,10 @@ public function load(array $configs, ContainerBuilder $container)
            if (is_dir($dir = $bundle['path'].'/Resources/views')) {
                $this->addTwigPath($twigFilesystemLoaderDefinition, $dir, $name);
            }
+
+            if (null !== $bundle['parent']) {
+                $this->prependParentPaths($twigFilesystemLoaderDefinition, $container, $name, $bundle['parent']);


Prepending paths looks wrong to me here, as it would overwrite user-configured paths for the same namespace too.
What you should do is collecting the child bundles for each bundle, and then adding paths for child bundles before adding the path of the bundle itself

@stof: Did not now about that. That'll require some change to adding the normal paths too, I'll see how to best fix this. Is there an easy way to cover this in the test?

Add a path in the user-configured paths for the same namespace, and ensure that it ends up as the first path in the loader config for this namespace

maxime-pasquier · 2017-01-06T09:30:35Z

@chalasr thanks!

stof · 2017-01-06T10:35:36Z

src/Symfony/Bundle/TwigBundle/Tests/DependencyInjection/TwigExtensionTest.php

+            array(__DIR__.'/Fixtures/Bundle/ChildTwigBundle/Resources/views', 'Twig'),
+            array(__DIR__.'/Fixtures/Bundle/ChildChildTwigBundle/Resources/views', 'ChildTwig'),
+            array(__DIR__.'/Fixtures/Bundle/ChildChildTwigBundle/Resources/views', 'Twig'),
+        ), $prependPaths);


The way these tests are written does not ensure that paths are added in the right order for each namespace. there is nothing guaranteeing that the combinations of appended paths and prepended paths is actually giving us the right priority.

What we should do to have reliable tests is rebuilding the internal structure of Twig_Loader_Filesystem::$path (i.e. the result of the configuration) and ensuring that it looks good for each namespace.

@stof: I think whatever gets prepended last will have the most priority. If the array above is in this order, the correct path will end up on top. However, this doesn't keep those user-configured paths in mind. I'm not sure where these get added to the loader, but if they get prepended after the bundle paths get prepended, it should work all right? Not sure if that's testable in the existing test though.

If the Twig internals of that loader do need to be changed, I don't think this can be merged before that is resolved?

@stof: What if we move these lines https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L84-L90 to here https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L109 and change the method call to prependPath instead of addPath? This should make sure that user-defined paths from config take precedence over anything else.

the issue is that the way the test is written does not allow to quickly validate what the final order will be when looking at it. This is why I think the test should not be written this way. We don't want to assert 2 separate lists, as we care about a single list (the one being used at runtime).

thus, the test is even confusing when looking at it, because the list of prependPath here is in reverse order compared to the order of paths in the loader (as prepending adds at the beginning while you append in $prependPaths), making it even harder to validate/understand the assertion.

@stof I'll see if I can collect all the paths before actually adding them to the loader so they can all be tested together in order.

wesleylancel · 2017-01-06T22:02:35Z

@stof I've added a separate commit so it's easier to see what I've changed (can be squashed later). A hierarchy of bundles is now built first, after that the paths are added to the loader. I'm not sure if I could simplify the code, especially the building of the hierarchy.

I could improve the test by asserting the array per namespace if that helps for readability?

xabbuh · 2017-01-07T09:08:42Z

I think an easier implementation could be to have two loops. The first would collect path information for bundles we override without actually registering them. The second would then first register the additional paths from child bundles followed by paths the bundle provides itself.

wesleylancel · 2017-01-10T10:57:23Z

@xabbuh: Isn't that what I'm doing now? First the hierarchy is built and then the paths are registered.

stof · 2017-01-11T15:41:55Z

src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php

+                );
+            }
+
+            if (is_dir($dir = $container->getParameter('kernel.root_dir').'/Resources/'.$name.'/views')) {


Note to maintainers: when merging to newer branches, we need to take care of the FileExistenceResource objects here (git conflicts should remind us about this btw)

stof · 2017-01-11T15:45:32Z

👍

dunglas · 2017-01-11T17:46:47Z

👍

fabpot · 2017-01-11T18:20:10Z

Thank you @wesleylancel.

…ent bundles in account (wesleylancel) This PR was squashed before being merged into the 2.7 branch (closes #19586). Discussion ---------- [TwigBundle] Fix bug where namespaced paths don't take parent bundles in account | Q | A | | --- | --- | | Branch? | 2.7 | | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #6919 | | License | MIT | | Doc PR | | Currently namespaced paths for templates such as `{% extends '@App/Layout/layout.html.twig' %}` do not work with bundles that have overruled templates using the `getParent()` method in another bundle. See attached ticket. This change prepends the path of the bundle implementing `getParent()` to the paths of the namespace of bundle returned as a parent. Commits ------- 0c77ce2 [TwigBundle] Fix bug where namespaced paths don't take parent bundles in account

homersimpsons · 2017-01-12T20:25:43Z

@fabpot Do you have any hint about the new release (containing this fix) date?

chalasr · 2017-01-12T20:27:07Z

@homersimpsons today :) see #21258

carsonbot added Status: Needs Review TwigBundle Bug labels Aug 10, 2016

wesleylancel force-pushed the namespaced-paths-parent-bundles branch from 7308a17 to 86c9111 Compare August 10, 2016 08:10

wesleylancel force-pushed the namespaced-paths-parent-bundles branch from b3d4a15 to 4ddc9ed Compare August 10, 2016 09:26

wesleylancel force-pushed the namespaced-paths-parent-bundles branch from 4ddc9ed to 57eceb7 Compare August 10, 2016 09:30

ro0NL reviewed Aug 10, 2016
View reviewed changes

This was referenced Aug 10, 2016

Serializer and Validator mappings don't use Bundle#getPath() #18563

Closed

[WIP][HttpKernel] Expose bundle hierarchy/metadata as a service #19594

Closed

[RFC][DependencyInjection][HttpKernel] Kernel as a service #19606

Closed

nicolas-grekas added this to the 2.7 milestone Dec 6, 2016

mhujer mentioned this pull request Dec 11, 2016

Both Symfony exception and Tracy are visible (Symfony exception should not be rendered) VasekPurchart/Tracy-Blue-Screen-Bundle#5

Closed

ro0NL mentioned this pull request Dec 28, 2016

[TwigBundle] register template paths for overridden bundles #21077

Closed

xabbuh mentioned this pull request Dec 28, 2016

[FrameworkBundle] Deprecate the Templating component integration #21035

Merged

StijnMaenhaut referenced this pull request in FriendsOfSymfony/FOSUserBundle Dec 30, 2016

Use native Twig references for templates

dd1fa8f

This makes the template compatible with projects disabling the Templating component in recent Symfony versions.

chalasr mentioned this pull request Dec 31, 2016

[FrameworkBundle][HttpKernel] Fix resources loading for bundles with custom structure #21113

Merged

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 6, 2017

xabbuh approved these changes Jan 6, 2017

View reviewed changes

stof reviewed Jan 6, 2017

View reviewed changes

Build all paths first, then add to loader

4e4702c

maryo added a commit to vaniocz/vanio-user-bundle that referenced this pull request Jan 9, 2017

Locked FOSUserBundle until symfony/symfony#19586 is resolved

d4a18c5

stof mentioned this pull request Jan 11, 2017

Overriding templates in a child bundle is broken after ed6a1a9 FriendsOfSymfony/FOSUserBundle#2378

Closed

stof reviewed Jan 11, 2017

View reviewed changes

fabpot closed this Jan 11, 2017

fabpot mentioned this pull request Jan 12, 2017

Release v2.7.23 #21258

Merged

This was referenced Jan 12, 2017

Release v2.8.16 #21259

Merged

Release v3.1.9 #21260

Merged

Release v3.2.2 #21261

Merged

umulmrum mentioned this pull request Jan 13, 2017

BUG: KNP Menu: Unable to find template "knp_menu.html.twig" #21281

Closed

This was referenced Feb 23, 2017

ParameterNotFoundException: You have requested a non-existent parameter "kernel.bundles_metadata" #21646

Closed

Template iheritance from parent bundle does not work. #21752

Closed

[TwigBundle] Fix bug where namespaced paths don't take parent bundles in account #19586

[TwigBundle] Fix bug where namespaced paths don't take parent bundles in account #19586

Conversation

wesleylancel commented Aug 10, 2016 • edited Loading

jakzal commented Aug 10, 2016

wesleylancel commented Aug 10, 2016

wesleylancel commented Aug 10, 2016

ro0NL Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Dec 28, 2016

wesleylancel commented Dec 28, 2016

ro0NL commented Dec 28, 2016 • edited Loading

ogizanagi commented Dec 28, 2016

ro0NL commented Dec 28, 2016 • edited Loading

chalasr commented Dec 29, 2016

ro0NL commented Dec 29, 2016 • edited Loading

ro0NL commented Dec 29, 2016 • edited Loading

xabbuh commented Jan 6, 2017

maxime-pasquier commented Jan 6, 2017

chalasr commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-pasquier commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleylancel commented Jan 6, 2017

xabbuh commented Jan 7, 2017 • edited Loading

wesleylancel commented Jan 10, 2017

Choose a reason for hiding this comment

stof commented Jan 11, 2017

dunglas commented Jan 11, 2017

fabpot commented Jan 11, 2017

homersimpsons commented Jan 12, 2017

chalasr commented Jan 12, 2017

wesleylancel commented Aug 10, 2016 •

edited

Loading

ro0NL Aug 10, 2016 •

edited

Loading

ro0NL Aug 10, 2016 •

edited

Loading

ro0NL Aug 10, 2016 •

edited

Loading

chalasr Dec 29, 2016 •

edited

Loading

ro0NL commented Dec 28, 2016 •

edited

Loading

ro0NL commented Dec 28, 2016 •

edited

Loading

ro0NL commented Dec 29, 2016 •

edited

Loading

ro0NL commented Dec 29, 2016 •

edited

Loading

ro0NL Jan 6, 2017 •

edited

Loading

xabbuh commented Jan 7, 2017 •

edited

Loading