[HttpKernel] Deprecate bundle inheritance #24160
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
fabpot
commented
Sep 11, 2017
•
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #24048 |
| License | MIT |
| Doc PR | symfony/symfony-docs#8389 |
b02a36d to
cad7f61
Compare
cad7f61 to
5f20b5c
Compare
5f20b5c to
aa56c12
Compare
pierredup
reviewed
Sep 11, 2017
| @@ -206,6 +206,10 @@ public function getBundles() | |||
| */ | |||
| public function getBundle($name, $first = true) | |||
| { | |||
| if (false === $first) { | |||
| @trigger_error(sprintf('Passing "false" as the second argument to %s() is deprecated as of 3.4 and will be removed in 4.0.', __METHOD_), E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
Typo, __METHOD_ should be __METHOD__
aa56c12 to
4f2beb9
Compare
ro0NL
reviewed
Sep 11, 2017
| @@ -206,6 +206,10 @@ public function getBundles() | |||
| */ | |||
| public function getBundle($name, $first = true) | |||
| { | |||
| if (false === $first) { | |||
There was a problem hiding this comment.
at least locateResource is calling it with false.. should we get rid of $first there too? I believe it serves the same purpose.
546a76f to
8fb7938
Compare
ro0NL
approved these changes
Sep 11, 2017
8fb7938 to
9bb10fd
Compare
| @@ -204,8 +204,12 @@ public function getBundles() | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function getBundle($name, $first = true) | |||
| public function getBundle($name, $first = true, $noDeprecation = false) | |||
There was a problem hiding this comment.
signature change should be removed, func_get_arg should be used instead.
xabbuh
reviewed
Sep 12, 2017
| @@ -56,6 +56,8 @@ public function getContainerExtension(); | |||
| * bundle. | |||
| * | |||
| * @return string The Bundle name it overrides or null if no parent | |||
| * | |||
| * @deprecated This method is deprecated as of 3.4 and will be removed in 4.0. | |||
There was a problem hiding this comment.
Do we need to drop the final dot here according to the latest CS changes?
There was a problem hiding this comment.
Most @deprecated tags have a dot at the end currently. As this is a sentence, I think that's the way it should be.
|
Should we also deprecate |
d73a911 to
94707d2
Compare
|
@chalasr Not sure it makes sense. This method is mainly called by Symfony itself, and if a bundle defines a parent, the message won't be displayed. So, it seems to me better to not add this. |
|
RFR |
94707d2 to
89893c1
Compare
chalasr
approved these changes
Sep 15, 2017
fabpot
added a commit
that referenced
this pull request
Sep 15, 2017
This PR was squashed before being merged into the 3.4 branch (closes #24160). Discussion ---------- [HttpKernel] Deprecate bundle inheritance | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #24048 | License | MIT | Doc PR | symfony/symfony-docs#8389 Commits ------- 89893c1 [HttpKernel] deprecated bundle inheritance ee9f4c3 fixed CS
fabpot
added a commit
that referenced
this pull request
Sep 26, 2017
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
fabpot
added a commit
that referenced
this pull request
Sep 29, 2017
…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
This was referenced Oct 18, 2017
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.