[HttpKernel] Remove bundle inheritance #24161
Conversation
e557cf6 to
2186261
Compare
|
The TwigBundle also deals with the bundle hierarchy when resolving paths for bundle templates: https://github.com/fabpot/symfony/blob/218626157e915f4e26cc8bb619637248c0008c72/src/Symfony/Bundle/TwigBundle/DependencyInjection/TwigExtension.php#L164-L214 I bet we can simplify a lot of code there too. |
080f83e to
9ca264c
Compare
|
@xabbuh now removed |
| @@ -169,55 +160,21 @@ public function load(array $configs, ContainerBuilder $container) | |||
| private function getBundleHierarchy(ContainerBuilder $container, array $config) | |||
There was a problem hiding this comment.
This inner code can be moved out safely, there is not much logic to encapsulate now.
There was a problem hiding this comment.
Not sure, I prefer to keep it.
There was a problem hiding this comment.
Could we update the method name? Maybe getBundleTemplatePaths() or something like that?
737e1da to
cd48de9
Compare
| @@ -166,58 +157,24 @@ public function load(array $configs, ContainerBuilder $container) | |||
| $container->registerForAutoconfiguration(RuntimeExtensionInterface::class)->addTag('twig.runtime'); | |||
| } | |||
|
|
|||
| private function getBundleHierarchy(ContainerBuilder $container, array $config) | |||
| private function getBundleTemplatePaths(ContainerBuilder $container, array $config) | |||
| { | |||
| $bundleHierarchy = array(); | |||
There was a problem hiding this comment.
Maybe variable should be renamed in conformity with method name change?
e7c272e to
b8639ee
Compare
|
Tests are green now! |
| /** | ||
| * @group legacy | ||
| * @expectedException \LogicException | ||
| * @expectedExceptionMessage Trying to register two bundles with the same name "DuplicateName" |
There was a problem hiding this comment.
that test is still relevant and should be kept as it is not related to bundle inheritance
There was a problem hiding this comment.
Indeed, I will fix it in 3.4 as this should not be deprecated there.
There was a problem hiding this comment.
I think you forgot to remove the legacy annotation in 3.4
There was a problem hiding this comment.
The @group annotation will be removed in #24364.
There was a problem hiding this comment.
#24364 is for master. I don't see how this is related to this 3.4 issue.
There was a problem hiding this comment.
The test fails with a deprecation on 3.4 otherwise.
b8639ee to
7f8b86c
Compare
2704c39 to
a8a0a21
Compare
This PR was merged into the 4.0-dev branch. Discussion ---------- [HttpKernel] Remove bundle inheritance | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Blocked by #24160 Commits ------- a8a0a21 [HttpKernel] removed bundle inheritance
…s (yceruto) This PR was merged into the 3.4 branch. Discussion ---------- [TwigBundle] Improve the overriding of bundle templates | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17557 | License | MIT | Doc PR | - ### [Overriding a Template that also extends itself](https://twig.symfony.com/doc/2.x/recipes.html#overriding-a-template-that-also-extends-itself) Now that bundles inheritance is deprecated and removed (#24160, #24161), I'm wondering if we can solve this old issue defining an exclusive namespace only for root bundles in `3.4` just bundles in `4.0`: ```yaml twig: paths: # adding paths behind the scene into TwigExtension app/Resources/FooBundle/views: Foo vendor/acme/foo-bundle/Resources/views: Foo vendor/acme/foo-bundle/Resources/views: !Foo # exclusive ``` Thus, one can decide when use the exclusive namespace to avoid the issue and then [we could to say also](http://symfony.com/doc/current/templating/overriding.html): > To override the bundle template partially (which contains `block`) creates a new `index.html.twig` template in `app/Resources/AcmeBlogBundle/views/Blog/index.html.twig` and extends from `@!AcmeBlogBundle/Blog/index.html.twig` to customize the bundle template: ```twig {# app/Resources/FooBundle/views/layout.html.twig #} {# this does not work: circular reference to itself #} {% extends '@Foo/layout.html.twig' %} {# this will work: load bundle layout template #} {% extends '@!Foo/layout.html.twig' %} {% block title 'New title' %} ``` I hear other suggestions about the excluse namespace. We will need to update http://symfony.com/doc/current/templating.html#referencing-templates-in-a-bundle too to add this convention. WDYT? Commits ------- 0a658c6 Add exclusive Twig namespace for bundles path
Blocked by #24160