[TwigBundle] Added auto namespacing for new templates directory structure #23339

HeahDude · 2017-06-30T16:16:58Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	~
License	MIT
Doc PR	todo

Using app/Resources/views has never needed any configuration, and it was really easy to override any templates of a bundle by adding a folder for it like app/Resources/TwigBundle/views.

Using the new directory structure, every path needs to be added in Twig configuration, which can be handled "automatically" when installing the TwigBundle with Symfony Flex https://github.com/symfony/recipes/blob/master/symfony/twig-bundle/3.3/etc/packages/twig.yaml#L2, but this is not automatic when migrating an app to the new directory strucure.

Even with Flex, overriding templates needs to add an explicit path for each bundle (example https://github.com/EnMarche/en-marche.fr/blob/master/app/config/config.yml#L55).

Moreover, this can be very ugly to have many *Bundle among templates folders which most of the time match controllers structure with lower case (see https://github.com/EnMarche/en-marche.fr/tree/master/templates).

Thus to ease auto configuration, I propose to place overridden bundle templates in a bundles sub folder:

/templates/bundles/Twig
/templates/bundles/EasyAdmin

What do you think?

linaori · 2017-07-01T08:43:18Z

Looks like a good solution to me.

nicolas-grekas · 2017-07-03T07:54:42Z

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

+        }
+
+        if ($container->fileExists($dir = $container->getParameter('kernel.project_dir').'/templates/bundles', false)) {
+            foreach (glob($dir.'/*', GLOB_ONLYDIR) as $path) {


the current bundle related logic is in getBundleHierarchy, why doing it another way here?

I'm not sure to get your comment right. Why would we add paths that do not exist? Here we just want to add paths for existing folders that need to override bundles at the application level.

How would you see it?

I agree with @nicolas-grekas here. You should add paths for all bundles like done above. The fact that the directory currently exists is not relevant.

yceruto

Making (by default) the main templates directory "configurable" is already the fact with Flex/Twig, so, is there a way to detect the main templates directory to do this without introduce a new convention?

yceruto · 2017-07-07T21:23:46Z

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

@@ -122,6 +122,17 @@ public function load(array $configs, ContainerBuilder $container)
            $twigFilesystemLoaderDefinition->addMethodCall('addPath', array($dir));
        }

+        if ($container->fileExists($dir = $container->getParameter('kernel.project_dir').'/templates', false)) {
+            $twigFilesystemLoaderDefinition->addMethodCall('addPath', array($dir));


In Flex when we're installing the Twig bundle this path will be added twice? AFAIK Twig Filesystem loader doesn't checks duplicated paths.

The goal is to remove that config, if this PR is merged we'll be able to update the recipe.

Then this would make it mandatory to use the templates directory to be able to override templates bundles automatically 👎

Registering automatically a directory to override bundles templates would require a configur 8000 ation key and this can easily be managed from the TwigExtension, templates/bundles being then a default if that's really a concern ;).

For now this is also adding the new conventional templates folder (the bundles sub directory being a pending proposal), as it was done for app/Ressources/views a few lines above.

Anyone is still free to add its own paths, even use the new templates directory structure in symfony 2.8 if wanted. Also the addition happens only if the templates folder exists which should be the new best practice now, we used to assume that app/Resources/views and app/Resources/*Bundle/views exist as well.

I got the point, just that I liked the idea to remove the old convention in favor of explicit configuration. I thought in this in the other way around (add the feature to the configured templates path) but there is not way to know what is it right now. So I guess rely on the approved structure should be the path. 👍

Instead of harcoding the templates/ directory here, what about adding a new configuration setting that would default to templates/ and use it here? Should be easy enough.

JasonCo 8000 al · 2017-07-21T18:10:44Z

On Jul 21, 2017, at 1:56 PM, Yonel Ceruto <notifications@github.com> wrote: @yceruto commented on this pull request. In src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php:

@@ -122,6 +122,17 @@ public function load(array $configs, ContainerBuilder $container)

$twigFilesystemLoaderDefinition->addMethodCall('addPath', array($dir)); } + if ($container->fileExists($dir = $container->getParameter('kernel.project_dir').'/templates', false)) { + $twigFilesystemLoaderDefinition->addMethodCall('addPath', array($dir)); I got the point, just that I liked the idea to remove the old convention in favor of explicit configuration. I thought in this in the other way around (add the feature to the configured templates path) but there is not way to know what is it right now. So I guess rely on the approved structure should be the path. 👍 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

fabpot · 2017-07-22T07:27:07Z

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

+        }
+
+        if ($container->fileExists($dir = $container->getParameter('kernel.project_dir').'/templates/bundles', false)) {
+            foreach (glob($dir.'/*', GLOB_ONLYDIR) as $path) {


I agree with @nicolas-grekas here. You should add paths for all bundles like done above. The fact that the directory currently exists is not relevant.

fabpot · 2017-07-22T07:27:43Z

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

@@ -122,6 +122,17 @@ public function load(array $configs, ContainerBuilder $container)
            $twigFilesystemLoaderDefinition->addMethodCall('addPath', array($dir));
        }

+        if ($container->fileExists($dir = $container->getParameter('kernel.project_dir').'/templates', false)) {
+            $twigFilesystemLoaderDefinition->addMethodCall('addPath', array($dir));


Instead of harcoding the templates/ directory here, what about adding a new configuration setting that would default to templates/ and use it here? Should be easy enough.

fabpot · 2017-07-27T06:30:49Z

@HeahDude Do you time to finish this one?

HeahDude · 2017-07-27T09:52:06Z

@fabpot Sure :) Is 851aee6 going in the right direction?

yceruto · 2017-07-27T13:29:07Z

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

@@ -122,12 +122,16 @@ public function load(array $configs, ContainerBuilder $container)
            foreach ($bundle['paths'] as $path) {
                $twigFilesystemLoaderDefinition->addMethodCall('addPath', array($path, $namespace));
            }
+
+            $twigFilesystemLoaderDefinition->addMethodCall('addPath', array($config['default_path'].'/bundles/'.$namespace, $namespace));


This path should be added before the bundle path, (here https://github.com/HeahDude/symfony/blob/851aee62eb905c8d1390a12f7f3a56fce5b8abe8/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L195):

// ---------> here <--------- if ($container->fileExists($dir = $bundle['path'].'/Resources/views', false)) { $bundleHierarchy[$name]['paths'][] = $dir; }

Otherwise we can't able to override templates from third-party bundles, because the parent template will be loaded first always.

Also the path needs to be checked if it exists before add it, otherwise Twig throw an exception:

[Twig_Error_Loader] The "templates/bundles/Twig" directory does not exist ("/home/yceruto/github/symfony/symfony-demo/templates/bundles/Twig").

rosier · 2017-07-30T14:51:17Z

I'm not sure if it's a good idea to auto configure /templates/bundles as the location to place overridden bundle templates. Since it puts the overridden bundle templates in a location that can conflict with the app templates inside the main /templates directory.

You can already place the overridden bundle templates in src/Resources directory without extra config. Which is similar to how the templates are stored inside the bundles and keeps the app templates and the overridden bundle templates separated from each other.

I propose to only keep the default_path config for the main app templates folder part of this PR.

yceruto · 2017-09-04T13:23:36Z

We can make "bundles" directory configurable too? default %twig.default_path%/bundles.

fabpot · 2017-09-13T13:43:49Z

closing in favor of #24179

…n to configure it (yceruto) This PR was merged into the 3.4 branch. Discussion ---------- [TwigBundle] Add default templates directory and option to configure it | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Feature freeze is coming so this one should be important for the new structure. Moving forward and alternative of #23339 but I'm proposing `templates/bundles/<BundleName>` instead of `templates/bundles/<BundleTwigNamespace>` to override bundles templates and easy migration from current `app/Resources/<BundleName>/views` convention. Also this fix the pending comments. Summary: * Added new option to configure default path for templates directory: ```yaml twig: default_path: '%kernel.project_dir%/templates' # default ``` * Added new path convention to override bundle templates `<default_path>/bundles/<BundleName>`: ``` # Examples: templates/bundles/TwigBundle/Exception/error.html.twig - @Twig/Exception/error.html.twig templates/bundles/FOSUserBundle/layout.html.twig - @FOSUser/layout.html.twig ``` Current templates in `<kernel.root_dir>/Resources/<BundleName>/views` have priority over the new one, and both have priority over the bundle `views` path. Commits ------- a1b391f Add default templates directory and option to configure it

…n to configure it (yceruto) This PR was merged into the 3.4 branch. Discussion ---------- [TwigBundle] Add default templates directory and option to configure it | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Feature freeze is coming so this one should be important for the new structure. Moving forward and alternative of symfony/symfony#23339 but I'm proposing `templates/bundles/<BundleName>` instead of `templates/bundles/<BundleTwigNamespace>` to override bundles templates and easy migration from current `app/Resources/<BundleName>/views` convention. Also this fix the pending comments. Summary: * Added new option to configure default path for templates directory: ```yaml twig: default_path: '%kernel.project_dir%/templates' # default ``` * Added new path convention to override bundle templates `<default_path>/bundles/<BundleName>`: ``` # Examples: templates/bundles/TwigBundle/Exception/error.html.twig - @Twig/Exception/error.html.twig templates/bundles/FOSUserBundle/layout.html.twig - @FOSUser/layout.html.twig ``` Current templates in `<kernel.root_dir>/Resources/<BundleName>/views` have priority over the new one, and both have priority over the bundle `views` path. Commits ------- a1b391fb00 Add default templates directory and option to configure it

carsonbot added Status: Needs Review Feature labels Jun 30, 2017

HeahDude changed the title ~~Added auto namespacing for new templates directory structure~~ [TwigBundle] Added auto namespacing for new templates directory structure Jun 30, 2017

HeahDude force-pushed the twig/overrides-bundles branch from 73c6a1e to 0980908 Compare July 1, 2017 11:18

nicolas-grekas added this to the 3.4 milestone Jul 3, 2017

nicolas-grekas reviewed Jul 3, 2017

View reviewed changes

HeahDude mentioned this pull request Jul 7, 2017

[TwigBundle] Allow changing bundle overrides root directory #23445

Closed

yceruto reviewed Jul 7, 2017

View reviewed changes

fabpot mentioned this pull request Jul 8, 2017

[Question] Where should the old app/Resources/ contents live now? symfony/flex#40

Closed

fabpot requested changes Jul 22, 2017

View reviewed changes

HeahDude added 2 commits July 27, 2017 11:50

Added auto namespacing for new templates directory structure

5c7d42e

fixup

851aee6

HeahDude force-pushed the twig/overrides-bundles branch from 0980908 to 851aee6 Compare July 27, 2017 09:50

yceruto reviewed Jul 27, 2017

View reviewed changes

yceruto mentioned this pull request Jul 27, 2017

Updated the application to use Symfony Flex symfony/demo#617

Closed

yceruto mentioned this pull request Aug 1, 2017

How to Override Templates from Third-Party Bundles for symfony4 #23740

Closed

yceruto mentioned this pull request Aug 31, 2017

[DX] A simpler way to define custom error pages #24049

Closed

yceruto mentioned this pull request Sep 12, 2017

[TwigBundle] Add default templates directory and option to configure it #24179

Merged

fabpot closed this Sep 13, 2017

HeahDude deleted the twig/overrides-bundles branch October 9, 2017 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[TwigBundle] Added auto namespacing for new templates directory structure #23339

[TwigBundle] Added auto namespacing for new templates directory structure #23339

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[TwigBundle] Added auto namespacing for new templates directory structure #23339

[TwigBundle] Added auto namespacing for new templates directory structure #23339

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!