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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TwigBundle] Improve the overriding of bundle templates #24264

Merged
merged 1 commit into from Sep 29, 2017

Conversation

Projects
None yet
8 participants
@yceruto
Contributor

yceruto commented Sep 19, 2017

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

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:

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:

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:

{# 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?

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 25, 2017

Contributor

I just updated the description to clarify the proposal, it's not about Bundle suffix but bundle class name instead. So, you have a bundle named AcmeBlogBundle? you will have two namespaces for their templates: @AcmeBlog and @AcmeBlogBundle. The last one is specially useful when you're overriding a template that also extends itself, without configure twig.paths manually.

At the same time, you can from a controller or anywhere to load directly the template of the bundle or the overridden one in your app, without create extra templates or extra configuration to achieve it.

Contributor

yceruto commented Sep 25, 2017

I just updated the description to clarify the proposal, it's not about Bundle suffix but bundle class name instead. So, you have a bundle named AcmeBlogBundle? you will have two namespaces for their templates: @AcmeBlog and @AcmeBlogBundle. The last one is specially useful when you're overriding a template that also extends itself, without configure twig.paths manually.

At the same time, you can from a controller or anywhere to load directly the template of the bundle or the overridden one in your app, without create extra templates or extra configuration to achieve it.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 27, 2017

Contributor

Feat. freeze is coming :) also we can use some preffix or suffix to achieve it, WDYT?

ping @fabpot, @javiereguiluz

Contributor

yceruto commented Sep 27, 2017

Feat. freeze is coming :) also we can use some preffix or suffix to achieve it, WDYT?

ping @fabpot, @javiereguiluz

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 27, 2017

Member

I don't dislike the idea ... but I'm not sold on it either. My concern is that you'll have two almost identical namespaces that are completely different: @FOSUser, @FOSUserBundle ... which one should I use? I already have a lot of problems remembering if the namespace contains the Bundle suffix or not. If I see @TwigBundle/... in a template, I don't think I'll remember that it refers to the original template path of the parent bundle.

Member

javiereguiluz commented Sep 27, 2017

I don't dislike the idea ... but I'm not sold on it either. My concern is that you'll have two almost identical namespaces that are completely different: @FOSUser, @FOSUserBundle ... which one should I use? I already have a lot of problems remembering if the namespace contains the Bundle suffix or not. If I see @TwigBundle/... in a template, I don't think I'll remember that it refers to the original template path of the parent bundle.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 27, 2017

Contributor

Another proposal that is easy to remember?

  • !FOSUser
  • FOSUser!
  • FOSUserSource
  • FOSUserOriginal
  • FOSUserParent
  • FOSUserBundle

This namespace should be used only to avoid the circular reference.

Contributor

yceruto commented Sep 27, 2017

Another proposal that is easy to remember?

  • !FOSUser
  • FOSUser!
  • FOSUserSource
  • FOSUserOriginal
  • FOSUserParent
  • FOSUserBundle

This namespace should be used only to avoid the circular reference.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 27, 2017

Member

I share @javiereguiluz' concerns though I also do not have a better proposal.

Member

xabbuh commented Sep 27, 2017

I share @javiereguiluz' concerns though I also do not have a better proposal.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 27, 2017

Member

Some random proposals:

{% extends 'parent@Foo/layout.html.twig' %}

{% extends '@@Foo/layout.html.twig' %}

{% extends '@Foo/layout.html.twig', { parent: true } %}
Member

javiereguiluz commented Sep 27, 2017

Some random proposals:

{% extends 'parent@Foo/layout.html.twig' %}

{% extends '@@Foo/layout.html.twig' %}

{% extends '@Foo/layout.html.twig', { parent: true } %}
@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 27, 2017

Contributor

@javiereguiluz only the second one @@Foo can be possible right now as valid Twig namespace, I'll take your proposal, thanks.

Contributor

yceruto commented Sep 27, 2017

@javiereguiluz only the second one @@Foo can be possible right now as valid Twig namespace, I'll take your proposal, thanks.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 27, 2017

Contributor

It's not intuitive anyway, so we need document it.

Contributor

yceruto commented Sep 27, 2017

It's not intuitive anyway, so we need document it.

@xabbuh

xabbuh approved these changes Sep 27, 2017

@ogizanagi

but my own preference would actually be

{% extends '@!Foo/layout.html.twig' %}
@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 28, 2017

Member

@ogizanagi's suggestion sounds good to me too.

Member

xabbuh commented Sep 28, 2017

@ogizanagi's suggestion sounds good to me too.

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 28, 2017

Contributor

Perfect! I like this too, changed to ! preffix :)

Contributor

yceruto commented Sep 28, 2017

Perfect! I like this too, changed to ! preffix :)

@Tobion

Tobion approved these changes Sep 29, 2017

@fabpot

fabpot approved these changes Sep 29, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 29, 2017

Member

Thank you @yceruto.

Member

fabpot commented Sep 29, 2017

Thank you @yceruto.

@fabpot fabpot merged commit 0a658c6 into symfony:3.4 Sep 29, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 29, 2017

feature #24264 [TwigBundle] Improve the overriding of bundle template…
…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

@yceruto yceruto deleted the yceruto:template_inheritance branch Sep 29, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 29, 2017

Member

@yceruto or anyone else with some free time: I broke tests when merging this into master, could you have a look please?

Member

nicolas-grekas commented Sep 29, 2017

@yceruto or anyone else with some free time: I broke tests when merging this into master, could you have a look please?

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 29, 2017

Contributor

@nicolas-grekas Still broken?

Contributor

yceruto commented Sep 29, 2017

@nicolas-grekas Still broken?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Sep 29, 2017

yes

@yceruto

This comment has been minimized.

Show comment
Hide comment
@yceruto

yceruto Sep 29, 2017

Contributor

The code in master look good to me:

if ($paths) {
// the last path must be the bundle views directory
$twigFilesystemLoaderDefinition->addMethodCall('addPath', array($path, '!'.$namespace));
}

and Travis pass (at least for TwigBundle):

taken from https://travis-ci.org/symfony/symfony/builds/281245817

Contributor

yceruto commented Sep 29, 2017

The code in master look good to me:

if ($paths) {
// the last path must be the bundle views directory
$twigFilesystemLoaderDefinition->addMethodCall('addPath', array($path, '!'.$namespace));
}

and Travis pass (at least for TwigBundle):

taken from https://travis-ci.org/symfony/symfony/builds/281245817

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 29, 2017

Member

Oh nice sorry, I didn't break it because if this PR, but another one, NVM :)

Member

nicolas-grekas commented Sep 29, 2017

Oh nice sorry, I didn't break it because if this PR, but another one, NVM :)

@Guite Guite referenced this pull request Oct 9, 2017

Open

Symfony 4 and Flex #1068

0 of 90 tasks complete

This was referenced Oct 18, 2017

@Guite Guite referenced this pull request Dec 1, 2017

Closed

Symfony 3.4 #1171

17 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment