Problem/Motivation

#2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders brought up that it's possible to get infinite recursion if you extend the current template. The expectation is that the parent template

Concrete example:

  1. core/themes/stable/templates/foo.html.twig
  2. core/themes/classy/templates/foo.html.twig contains {% extends "foo.html.twig" %} (to add a specific class) and expects Stable's foo.html.twig to be extended
  3. core/themes/bartik/templates/foo.html.twig contains {% extends "foo.html.twig" %} (to override a Twig block) and expects Classy's foo.html.twig to be extended — or, really, any of its ancestor themes, because it couldn't care less if Classy's foo.html.twig was removed

Notes:

  • in point 3, we specifically do NOT want to specify {% extends "@classy/foo.html.twig" %} because we don't want to care. We just want the parent theme's template — according to the theme registry — to be extended.
  • in point 1, the "root template" could just as well not live in a theme, but in a module. That's also a valid use case.

This issue is to research if it's possible to respect inheritance.

Proposed resolution

TBD.

Remaining tasks

Try things.

User interface changes

n/a

API changes

TBD, probably none.

Files: 

Comments

Dragan Eror’s picture

Maybe to check the path of template and not to include "self" if tries.
Whether someone checked out for this issue in Twig community?

Cottser’s picture

Assigned: Unassigned » Cottser
Cottser’s picture

I haven't found any way so far of getting the "context" of the calling template (the template that contains the include/extends/whatever tag) from a Twig loader. It looks like if we handle this it would have to be at a different place in the process, not in the loader step.

Cottser’s picture

Created an issue upstream to ask about this: https://github.com/twigphp/Twig/issues/1627

davidhernandez’s picture

@Cottser, I saw you closed the github issue. Does that mean you know where to poke around?

Cottser’s picture

Somewhat, it sounds like this is going to be a hard problem though.

Cottser’s picture

Assigned: Cottser » Unassigned

Still thinking about this but don't want to hog it, especially since I'm not actively working on code for this issue.

Cottser’s picture

Assigned: Unassigned » Cottser
Status: Active » Needs review
FileSize
2.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,435 pass(es). View

Here's some early and rough code, a node visitor that throws an exception when a template extends itself. Needs a lot more testing and work, but progress!

If anyone wants to give this a manual test at this stage, these steps should work:

  1. Standard install with the patch applied.
  2. twig_debug on (to get auto reload when changing the template).
  3. Change the top line of /core/themes/bartik/templates/block--search-form-block.html.twig to:
    {% extends "block--search-form-block.html.twig" %}
    
  4. You should see an exception when you visit the homepage while logged in.
Cottser’s picture

Cottser’s picture

Title: Check if Twig loaders can protect against infinite loops/self references » Protect against infinite recursion in Twig templates when extending
Status: Needs review » Needs work
Cottser’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,578 pass(es). View
4.29 KB

Just making this a bit more real and adding some light docs and more @todos, should be no functional changes here. Better class name suggestions more than welcome!

More importantly, if anyone can come up with some real use cases where you should be allowed to extend/include/embed the template you're in that would be great to know! I haven't tested embed yet but I assume that it will be similar to extend and include in its loop causing abilities. menu.html.twig can be looked at as an example of a recursive macro if that helps inspire you :)

davidhernandez’s picture

I think there are two sides to extending a template with the same name. It is useful for making a small modification to a template without overriding the whole template. For example, Bartik's block--search-form-block and block--system-menu-block. Each only change part of the template (a Twig block) but nothing else. Therefore, any changes to the original template are inherited. This reduces maintenance burden.

The other side is whether we are putting too much important on maintenance. When 8 releases, none of these templates will change, so what is gained by not overriding the whole template? I think we have to think about contrib and custom modules. The situation here may in fact be worse. If you extend a template from a contrib module or theme, and that template changes, it can break your template. It might be a better practice to override the whole template instead of extending it, which is more likely to future proof your theme. :/

The case where we've wanted this is when there are moving parts. If the template being extended moves, or the theme moves, the theme automatically adjusts. How likely is this to happen in the wild? I dunno. I also don't know it if makes sense. If you move your theme, do you want your template to automatically pick up a different template? There could be all kinds of unintended consequences of that.

Cottser’s picture

@davidhernandez thanks for your thoughts. I mean specifically a template extending itself, not a parent template. I still feel that extends in general and more specifically the registry loader are good things. Folks like @eatings have even said that the way the registry loader works is how they'd assumed inheritance would work in D8 :) yes there can be moving parts but we always have @namespaces to resolve any ambiguity. I think (and hope) that most people in their own themes will just use the registry loader.

Below is the kind of self-extension I'm talking about, but you'd need some kind of condition on this to prevent the infinite loop and I can't think of how you'd actually do that. I played around with a few things but didn't come out with anything useful, perhaps I need to make some nested arrays, though ;)

foo/templates/bar.html.twig:

{% extends "@foo/bar.html.twig" %}

A rather obvious example but when using the registry loader, extends "bar.html.twig" could map to this, just not explicitly.

davidhernandez’s picture

These extensions I believe are the only cases where we have to specify the namespace and template path. If the recursion is fixed we wouldn't have to specify it correct?

Re reading your example I think I got confused. You are taking about literally extending the exact same template? A template using a namespace and path to itself? I don't understand what that would accomplish.

Cottser’s picture

Regarding "fixing" the recursion: It might be possible but IMO it would probably be the wrong way to go and much too complex to "automatically" try and fix recursion. The current patch adds a NodeVisitor that just throws an exception when it finds a template that extends itself. It's a way to not accidentally shoot yourself in the foot.

Regarding your second paragraph - exactly! That's I'm trying to suss out. From the upstream issue (https://github.com/twigphp/Twig/issues/1627) it sounds like maybe "include" would be the more practical use case, at least to start with…

Fabianx’s picture

Potentially add a new issue to allow extending from the base parent:

{% extends 'base-theme:foo.html.twig' %}

Cottser’s picture

Potentially _self would be a way out of this exception for people that have use cases for when they want to extend/include themselves. Need to play with that.

Cottser’s picture

Cottser’s picture

Another thought: Maybe instead of trying to guess all the use cases there could be a killswitch for this, for example you could override the recursion checking in services.yml.

Thoughts, anyone?

Cottser’s picture

Assigned: Cottser » Unassigned
FileSize
3.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,713 pass(es). View

Just a reroll.

Wim Leers’s picture

Title: Protect against infinite recursion in Twig templates when extending » {% extends "foo.html.twig" %} in Twig templates does not respect theme inheritance
Priority: Normal » Major
Issue summary: View changes
20:13:40 <WimLeers> https://www.drupal.org/node/2291449 brought "Add Twig template inheritance based on the theme registry, enable adding Twig loaders"
20:13:41 <WimLeers> yay
20:13:56 <WimLeers> but it seems like it doesn't work correctly at all as soon as you have multiple inheritance?
20:14:13 <WimLeers> there's zero mentions of "base theme" or "multiple" or "inheritance" in both the issue and the committed patch
20:14:32 <WimLeers> e.g. foo.html.twig in bartik extends the same file in classy extends the same file in stable
20:15:30 <WimLeers> I also don't see how it could ever have worked
20:15:59 <WimLeers> Because you don't know what theme (or generally speaking, which extension) the extending template is from
20:16:11 <WimLeers> Twig just calls loadTemplate('foo.html.twig'), and that's it
20:18:53 <WimLeers> What actually ends up happening is that {% extends "foo.html.twig" %} in Bartik is indeed picked up by the theme registry, but rather than figuring out that it should load foo.html.twig from either classy, or stable, or perhaps the original module that defined it, it just returns the same template: Bartik's!
20:18:56 <WimLeers> IOW: endless loop.
20:19:08 <WimLeers> So then you get fun stuff like: ( ! ) Fatal error: Maximum function nesting level of '300' reached, aborting! in /Users/wim.leers/Work/drupal-unus/vendor/twig/twig/lib/Twig/Template.php on line 33
22:45:10 <Cottser> The short answer is https://www.drupal.org/node/2387069
22:45:46 <Cottser> There are other possible approaches for sure though. If I recall correctly Twig itself doesn't support multiple inheritance
22:46:13 <WimLeers> I need to run
22:46:19 <WimLeers> But this is not multiple inheritance
22:46:26 <WimLeers> this is single inheritance
22:46:35 <WimLeers> it may be a _long chain_ of inheritance
22:46:41 <WimLeers> but we're only inheriting from one thing directly
22:47:03 <WimLeers> A template has a public function getTemplateName()  method
22:47:15 <WimLeers> in there you can see the extension it belongs to
22:47:49 <WimLeers> … but loadTemplate() still doesn't allow you to pass context, sadly
22:48:07 <WimLeers> Perhaps if we subclass TwigTemplate, TwigEnvironment etc, to allow Twig to be aware of "the current theme"
22:48:24 <WimLeers> so that we can NOT do this:
22:48:25 <WimLeers> return $this->env->loadTemplate($template, $index);
22:48:28 <WimLeers> but instead:
22:48:33 <WimLeers> return $this->env->loadTemplate($template, $index, $current_theme_name)

#12:

Each only change part of the template (a Twig block) but nothing else.

Well, as we gradually remove all remaining preprocess functions into Twig templates themselves, Twig templates will also contain that former preprocess logic. Which means that …

When 8 releases, none of these templates will change, so what is gained by not overriding the whole template?

… if you're overriding entire templates, your templates will not benefit from bugfixes to that variable processing logic in the extended template. You'd be forced to manually keep them in sync, and verify after every Drupal core/contrib release that the original template did not change (i.e. did not get bugs fixed).

So I disagree with:

If you extend a template from a contrib module or theme, and that template changes, it can break your template.

… because that means those modules/themes are violating semantic versioning.


#13:

Folks like @eatings have even said that the way the registry loader works is how they'd assumed inheritance would work in D8 :)

As did I! Especially because it says "it works with the theme registry". Which I foolishly took to mean that it is aware of the inheritance tree, not the "fully resolved" inheritance tree — and that is what the theme registry actually is. So it makes sense that it doesn't work from an internals POV, but not from a themer POV.

I think (and hope) that most people in their own themes will just use the registry loader.

I don't see how you can.


#16:

Potentially add a new issue to allow extending from the base parent:

{% extends 'base-theme:foo.html.twig' %}

This is exactly what I tried, but failed to do, because Twig makes one huge assumption: that prefix is a "namespace" and therefore always maps to the same template. Twig doesn't have the concept of a "template caller context". In Drupal terminology: it doesn't allow us to know the theme of the template that is loading another template.


Conclusion: I think the infinite recursion is just a symptom. The real problem is that {% extends "foo.html.twig" %} works in an unexpected way: it does not respect theme inheritance (i.e. it does not go to the parent theme), and that is why it infinitely recurses. Therefore, retitling this issue.

Fabianx’s picture

That is an interesting aspect of the thing:

- The theme registry allows to overwrite templates, not extend it - in general.

However:

In a node--1.html.twig in 99% of cases you want with extends node.html.twig, not the parent template, but the template in your own theme.

Of course a potential rule could be:

Find me the template that is the same as mine (same theme hook), but not the same filename (avoid recursion).

I think first of all we need a simple & a complex example:

Simple:

block.html.twig: {% extends block.html.twig %}

Complex:

parent_theme: block.html.twig: {% extends block.html.twig %}
child_theme: block.html.twig: {% extends block.html.twig %}
child_theme: block--foo.html.twig: {% extends block.html.twig %}

---

Simple first:

block.html.twig: {% extends block.html.twig %}

creates a template calling:

$this->env->loadTemplate('block.html.twig', 'block.html.twig', 1);

However that is not enough as Wim pointed out.

Fortunately the multi-array support makes this rather simple to fix using a variation of the node visitor above:

If we change that for the same filename to:

  $this->loadTemplate(array("@bartik/block.html.twig", "@stable/block.html.twig", "@block/block.html.twig"), "block.html.twig", 1);

then we can codify the theme inheritance of the hook within the template (and always start the theme hierarchy based on where we are currently in the theme chain so remove the @bartik case if we are in bartik namespace).

And fortunately for every array TwigEnvironment::resolveTemplate() is called, so we can also shortcut this a lot or even use pseudo namespaces + context:

e.g.

  $this->loadTemplate(array("@base/block.html.twig", "@drupal_template_is_from/bartik:block.html.twig"), "block.html.twig", 1);

And resolve whatever template we want in resolveTemplate() using the additional context.

So solvable in various ways.

I kinda like adding the theme registry chain directly in the template as it improves debuggability and clarity one what it tries to load, but I don't know if it is flexible enough.

However in resolveTemplate we could still short-cut any namespaces with a quick visit to the theme registry to avoid calling the loader chain n times.

Edit:

I just see that context _is_ passed, just not to the loader:

If I change my test script to use:

require_once "core/themes/engines/twig/twig.engine";

print twig_render_template('core/themes/bartik/templates/block.html.twig', []);

like the theme registry would do (it selects an explicit template first).

Then it would output:

        return $this->loadTemplate("block.html.twig", "core/themes/bartik/templates/block.html.twig", 1);

However this is still not passed on, but means the NodeVisitor will work.

It also means to get the more complex scenarios to work, we should ensure we never load a template with just 'block.html.twig', but instead resolve it at compile time (where we know the theme).

e.g. I suggest to remove the ThemeRegistryLoader again (as it makes it impossible to know the template used without calling the loader again) and instead do the same in the Node Visitor with explicit namespaces.

So that non-namespaced and non-absolute templates are resolved in bartik to:

        return $this->loadTemplate(["@stable/block.html.twig", "@block/block.html.twig"], "core/themes/bartik/templates/block.html.twig", 1);

and for the one in classy:

        return $this->loadTemplate(["@block/block.html.twig"], "core/themes/classy/templates/block.html.twig", 1);

and only add those hooks, whose templates do exist.

Only question remaining now is: Does our theme registry support this kind of information retrieval?

Fabianx’s picture

It does not, but this fixes it:

diff --git a/core/lib/Drupal/Core/Theme/Registry.php b/core/lib/Drupal/Core/Theme/Registry.php
index e2a89a9..dfe0ddd 100644
--- a/core/lib/Drupal/Core/Theme/Registry.php
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -497,6 +497,17 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
           $result[$hook]['path'] = $path . '/templates';
         }
 
+        if (isset($cache[$hook]['template chain'])) {
+          $result[$hook]['template chain'] = $cache[$hook]['template chain'];
+        }
+        else {
+          $result[$hook]['template chain'] = array();
+        }
+
+        if (isset($result[$hook]['template'])) {
+          $result[$hook]['template chain'][] = [$theme, $result[$hook]['template'], $result[$hook]['path']];
+        }
+
         // If the default keys are not set, use the default values registered
         // by the module.
         if (isset($cache[$hook])) {

gives output:

Array
(
    [template] => block
    [path] => core/themes/bartik/templates
    [type] => theme_engine
    [theme path] => core/themes/bartik
    [template chain] => Array
        (
            [0] => Array
                (
                    [0] => block
                    [1] => block
                    [2] => core/modules/block/templates
                )

            [1] => Array
                (
                    [0] => stable
                    [1] => block
                    [2] => core/themes/stable/templates/block
                )

            [2] => Array
                (
                    [0] => classy
                    [1] => block
                    [2] => core/themes/classy/templates/block
                )

            [3] => Array
                (
                    [0] => bartik
                    [1] => block
                    [2] => core/themes/bartik/templates
                )

        )
    [render element] => elements
    [preprocess functions] => Array
        (
            [0] => template_preprocess
            [1] => template_preprocess_block
            [2] => comment_preprocess_block
            [3] => contextual_preprocess
            [4] => help_preprocess_block
            [5] => menu_ui_preprocess_block
            [6] => node_preprocess_block
            [7] => search_preprocess_block
            [8] => shortcut_preprocess_block
            [9] => system_preprocess_block
            [10] => user_preprocess_block
            [11] => bartik_preprocess_block
        )

)

So exactly the chain we need :). (Just need to reverse it obviously).

I also added the template name and path, so we can compare easily that something can extend itself for the complex scenario without having to resolve the namespace - however we could also skip that for space reasons and just use the namespace and resolve at run-time.

And that now makes the rest of the issue rather easy now (not trivial still).

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work
Issue tags: -sprint

Took another deep look at this, because it prevents us from having the simplicity we need for #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering). I created a new Twig issue, hopefully they'll help us out: https://github.com/twigphp/Twig/issues/2059.


Apparently this problem was knowingly introduced: #2291449-60: Add Twig template inheritance based on the theme registry, enable adding Twig loaders says this:

I think in the case of #2358037: Add search form block Twig template file we need to leave the classy namespace there, otherwise it would be referencing itself. I'm not sure if it's possible to protect from those, in other words I'm not sure if the loader class can be aware of the context from which it's called.

And it apparently was even in the IS:

Which by default will extend core/modules/block/template/block.html.twig, but if a theme overrides block.html.twig, it will extend from the theme's block.html.twig, and so on.

It was a step forward, but it means we now have to deal with all the fallout, because it was unfortunately not entirely thought through.


#16: a @base-theme namespace is also what I thought of, but that also doesn't work, because we don't get the current template's file name (like I said in #21). We could actually look at the global state to get the currently active theme. But that actually still doesn't help, because what if the current theme inherited a certain template, then base-theme actually refers to the base theme of the base theme, but we can only look at the currently active theme, so we'd use the base theme (we'd use Classy instead of Stable if the current theme is Bartik).

#20: that patch, and the previous versions of it, don't solve the actual problem. They still don't enable proper extending of the base theme's template. They only fix the infinite recursion problem.

#22: I love the idea of compiling {% extends "block.html.twig %} into

$this->loadTemplate(array("@bartik/block.html.twig", "@stable/block.html.twig", "@block/block.html.twig"), "block.html.twig", 1);

That would mean we would be using http://twig.sensiolabs.org/doc/tags/extends.html#dynamic-inheritance, which makes it all the more understandable.
+1 for removing ThemeRegistryLoader again in favor of this.


My idea was: override \Twig_Environment::loadTemplate() + \Twig_Template::loadTemplate() so we can pass in the additional context, so that we can pass that on to the Twig loaders, and then ThemeRegistryLoader will actually receive the necessary information.

But having seen @Fabianx's proposal in #23, I like that better. I'm currently trying to make that work.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
14.74 KB

Thanks to the idea from @Fabianx in #23 combined with some research of my own, I've got a working solution!

It uses the theme registry at Twig template compilation time to transform extends "FOO.html.twig" to extends ['path/to/parent/theme/FOO.html.twig', 'path/to/grandparent/theme/FOO.html.twig'] and include "FOO.html.twig" to include ['path/to/current/theme/FOO.html.twig', 'path/to/parent/theme/FOO.html.twig'].

In other words: this transforms Twig templates at compilation time to respect our theme registry, but still uses native Twig functionality, therefore simplifies Twig debugging, but doesn't put the burden on the themer to specify every parent theme template.


Concrete example where this already helps Drupal core: in core/themes/bartik/templates/status-messages.html.twig, we had this:

{% extends "@classy/misc/status-messages.html.twig" %}

Which can now simply be this:

{% extends "status-messages.html.twig" %}

No more need to specify the namespace, nor the path to it within the namespace. (The namespace is really just a shortcut for core/themes/classy/templates, nothing more.)

Thanks to this patch, it gets transformed to this at template compilation time:

        return $this->loadTemplate(array(0 => "core/themes/classy/templates/misc/status-messages.html.twig", 1 => "core/themes/stable/templates/misc/status-messages.html.twig", 2 => "core/modules/system/templates/status-messages.html.twig"), "core/themes/bartik/templates/status-messages.html.twig", 1);

See how it respects the full hierarchy: Classy, Stable, then the module. All thanks to the theme registry.

Wim Leers’s picture

Note that I developed this at the same time as #2702061-54: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering), so it's already proven to solve the problems we were seeing there. This issue would be the first big TX win brought by the theme component library work we've been doing. It'll benefit everybody today, even while we're not yet using components.

Status: Needs review » Needs work

The last submitted patch, 26: twig_theme_registry-2387069-26.patch, failed testing.

Wim Leers’s picture

The only failures are those in Twig tests. All other tests pass, which means it actually does work, I just need to update the expectations and mocks in the Twig tests. The lack of more failures proves it works :)

Now working on fixing the failures.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
17.32 KB
2.65 KB

This fixes the unit test failures. Should go from 24 to 6 failures.

Fabianx’s picture

#26: Great work! - Exactly how I envisioned this working.

One thing that might need fixing / testing:

The include statement does miss the "Is this the same template?" check.

Including the exact same template name can still be useful, if I e.g. want to wrap the template of the parent class with some additional divs :D.

So I suggest to use the same check for include as for extends.

Edit:

Other things that need to be fixed now that RegistryLoader is gone are:

  • - embed
  • - use
  • - import

In addition to include and extends.

Status: Needs review » Needs work

The last submitted patch, 30: twig_theme_registry-2387069-30.patch, failed testing.

Wim Leers’s picture

I tried to make embed also work, but there's a bug deep in Twig that prevents this from working: \Twig_TokenParser_Embed sets the parent template after the parser is invoked, i.e. after node visitors are applied. So that's literally impossible to fix. I don't want this to be blocked on Twig fixing their stuff, and then Drupal getting an updated Twig version (which can only happen in Drupal 8.2, not 8.1).

I think it's fine for this to only deal with extends and include, they cover 95% of use cases.


The include statement does miss the "Is this the same template?" check.

You're misreading the code. And the code should be more clear. That's not checking if it's the same exact template file. It's checking if the template file name matches. For example, when block--system-menu-block.html.twig contains {% extends "block.html.twig" %}, we want that to extend this theme's block.html.twig, not the parent theme's. That's what that check is for.

Therefore this makes no sense:

So I suggest to use the same check for include as for extends.

I'm not entirely sure this makes sense. This would even lead to endless recursion in stock Twig!
What we could do, is if the include is indeed referencing the same file name, then the candidates will include the current exact file, and we should exclude that file, because that would result in an endless recursion. (Even though core/modules/system/templates/menu.html.twig already demonstrates how to avoid that.) However … once you're dealing with an Twig_Node_Include node, there's no way at all to determine what the current file is! You can't get at the module node, you can't get at the stream, you can't get at the parser, and either one of those would allow us to know the current template's filename.

Conclusion: none of what you ask is actually possible … due to design flaws in Twig.


Keeping at NW because this is only renaming variables and adding docs to address #31. Same number of failures.

Wim Leers’s picture

There are 4 failures in Drupal\system\Tests\Theme\TwigExtensionTest. In:

  1. testTwigExtensionLoaded()
  2. testTwigExtensionFilter()
  3. testTwigExtensionFunction()

The last two modify the configuration to change the default theme. If I comment out those last two test methods, then the first one also passes. This shows that these tests do not actually run in isolation.

The root cause seems to be (related to the fact) that setThemeRegistry() is being called with NULL set. Which seems to be related to #2448847: [regression] Themes unable to implement hook_theme_registry_alter(), which made it use setter injection because as of that issue, there's a circular dependency between the theme registry and the theme manager.

I've spent >2 hours on this. Somebody who actually worked on this Twig + theme system stuff will have to really fix this.


This will bring it down to 2 failures.

Wim Leers’s picture

This will need to update the CR at https://www.drupal.org/node/2381103.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
18.79 KB

Re-uploading #34 so testbot can test it.

Status: Needs review » Needs work

The last submitted patch, 36: twig_theme_registry-2387069-34.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
18.79 KB
1014 bytes

#34 contained one typo that was causing a lot of fails :P

Status: Needs review » Needs work

The last submitted patch, 38: twig_theme_registry-2387069-38.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
19.92 KB
1.26 KB
1.29 KB

After hour upon hour upon hour of debugging, turns out the root cause & solution totally make sense!

\Drupal\Core\Template\ThemeRegistryNodeVisitor makes theme registry-dependent changes. And if the default theme changes, that means templates need to be updated too, to take into account the updated theme registry: the candidate parent Twig templates become different.

In the particular example of the test that is failing:

public function testTwigExtendsIncludeViaThemeRegistry() {
    // Test the module-provided extend and insert templates.
    $this->drupalGet('twig-theme-test/registry-loader');
    $this->assertText('This line is from twig_theme_test/templates/twig-registry-loader-test-extend.html.twig');
    $this->assertText('This line is from twig_theme_test/templates/twig-registry-loader-test-include.html.twig');

    // Enable a theme that overrides the extend and insert templates to ensure
    // they are picked up by the registry loader.
    $this->config('system.theme')
      ->set('default', 'test_theme_twig_registry_loader')
      ->save();
    $this->drupalGet('twig-theme-test/registry-loader');
    $this->assertText('This line is from test_theme_twig_registry_loader/templates/twig-registry-loader-test-extend.html.twig');

The first drupalGet() gets this as the Twig expression to load parents:

Twig_Node_Expression_Array(
  0: Twig_Node_Expression_Constant(value: 0)
  1: Twig_Node_Expression_Constant(value: 'core/modules/system/tests/modules/twig_theme_test/templates/twig-registry-loader-test-extend.html.twig')
)

The second drupalGet() gets this (i.e. after changing the default theme):

Twig_Node_Expression_Array(
  0: Twig_Node_Expression_Constant(value: 0)
  1: Twig_Node_Expression_Constant(value: 'core/modules/system/tests/themes/test_theme_twig_registry_loader/templates/twig-registry-loader-test-extend.html.twig')
  2: Twig_Node_Expression_Constant(value: 1)
  3: Twig_Node_Expression_Constant(value: 'core/modules/system/tests/modules/twig_theme_test/templates/twig-registry-loader-test-extend.html.twig')
)

(See interdiff-debug.txt and apply it if you want to reproduce this debug output to convince yourself.)

So, what was missing, was the deleting of all compiled Twig templates after changing the default theme.


This reroll then brings it down to a single fail.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
22 KB
1012 bytes

And this brings it down to zero fails and cleans things up a bit.

That last fail was from a test case that no longer makes sense. So, simply removed that one.

Now blocked on reviews!

The last submitted patch, 40: twig_theme_registry-2387069-40.patch, failed testing.

Fabianx’s picture

Tests++ - Great catch on the rebuild.

- The interdiff in #41 looks incorrect somehow. At least I cannot see it fixing tests ...

Wim Leers’s picture

FileSize
2.65 KB

#44: you're right! No idea how that happened. Right interdiff attached.

Fabianx’s picture

Status: Needs review » Needs work

Just a quick drive-by comment.

If we can't fix include / embed, etc. due to Twig limitations, then we probably need to keep the TwigRegistryLoader for now - even if it never will be invoked for the cases fixed by this patch.

CNW to keep the RegistryFileLoader, as now including another template won't work anymore (and there is missing test coverage obviously).

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
27.23 KB
6.28 KB

As of #40, we have test coverage proving this works :) But what is still missing, is test coverage proving that {% extends "SAME-TEMPLATE-NAME.html.twig" %} works.

This adds explicit test coverage for that. (The several template changes in this patch already proved that this works though.)

In doing so, I discovered that the current patch was only working for 2 levels of inheritance, not 3. Because \Drupal\Core\Template\ThemeRegistryNodeVisitor::getCandidateParentTemplates() was simply taking what's in the registry, and then removing the first entry in the lineage, instead of considering the current position in the lineage.

This patch merely adds the necessary test coverage, and will fail because of this oversight in earlier versions of this patch. Fix coming.

Wim Leers’s picture

And here's the fix.

Wim Leers’s picture

In doing #47+#48, I noticed some misnamed variables. This fixes that.

Status: Needs review » Needs work

The last submitted patch, 49: twig_theme_registry-2387069-49.patch, failed testing.

Wim Leers’s picture

FileSize
24.74 KB
4.09 KB

#46: done — restored TwigRegistryLoader, plus its test coverage that I removed in #41.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
24.74 KB

Sigh, d.o--

Reuploading the patch for #51 so that it gets tested.

Wim Leers’s picture

At #34, I basically gave up on fixing some of the test failures I was seeing. Thanks to the work I did since then, I understood what was happening. This fixes it.

This patch is now completely ready for final review.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Template/ThemeRegistryNodeVisitor.php
@@ -0,0 +1,200 @@
+    elseif ($node instanceof \Twig_Node_Include) {
+      $include = $node->getNode('expr');

Hm, maybe we should put "include" to a follow-up, because:

a) no new test coverage for include

b) I still cannot see why in block.html.twig I should not be allowed to include e.g classy's block.html.twig.

c) Patch mainly deals with extends and is about that.

d) RegistryLoader exists again, so there is no change for include.

The last submitted patch, 47: twig_theme_registry-2387069-47.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

a) if the test coverage was sufficient before, why is it not today? It proves what worked before still works. The added test coverage proves things broken today start working.
b) Do you mean Why not {% extends "@classy/block.html.twig" %} inside Bartik's block.html.twig? Because at some point in the future, that template may cease to exist. It should just be looked up via the theme registry.
c) Yes, mainly. I wish I could do it in a comprehensive manner, but sadly Twig works in a surprisingly brittle manner.
d) Right, there is no functional change.

… and only after I replied to those 4 points, I noticed the leading sentence:

Hm, maybe we should put "include" to a follow-up, because:

Works for me.

I started with extends, then tried to make it work for everything (extends, include, embed …), but then quickly got stuck.

I think it makes total sense to limit the scope of this to just extends — that's also the very thing that is so frustrating and brittle in use today.

Doing that.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
24.6 KB
1.79 KB

Done.

Wim Leers’s picture

FileSize
1.15 KB

Oops, wrong interdiff.

Wim Leers’s picture

I missed some include-related things in #57.

Wim Leers’s picture

Also, note that Twig maintainer @stof proposed this approach independently from @Fabianx at https://github.com/twigphp/Twig/issues/2059#issuecomment-225619055 as one of two possible solutions.

Cottser’s picture

Really great to see this. I tried to accomplish this a couple years ago but clearly didn't get to the full solution, only part way.

Wim Leers’s picture

Discussed further with @stof from Twig.

What I thought was a flaw in Twig in #33 (unable to make embed etc work) is not really a flaw. Well, it's flawed in that test coverage and documentation are missing (filed a PR for that: https://github.com/twigphp/Twig/pull/2069), and technically it should be possible and currently isn't.

But when you think about it: Where do we need templates to care about their parents, i.e. the template lineage?

  1. Well, only when extending templates of course, i.e. when you're actually extending a parent template, when you really are using inheritance to add more.
  2. The same cannot be said of include and embed. Because in either of those cases, you just want the active theme's version of that include/embed to be loaded. So actually, in those cases using HEAD's theme registry-based TwigLoader totally makes sense!

In conclusion: HEAD works fine for include and embed, only fails when using extends. Of course, using extends is absolutely crucial (for the reasons explained many times before in this issue).
Hence the changes in #54+#55 totally make sense.

davidhernandez’s picture

I was going to suggest that include was indeed not the same use case as extend, but didn't want to get yelled at. So I'm glad Wim said it first! :D An include is likely to be very particular about where the content is coming from.

We actually have a different problem with includes, in that I don't think you can include anything that isn't recognized by Drupal's theme system. But that's for another issue.

If this is the approach, we need to make sure it is well documented, as it creates a non-intuitive inconsistency.

Wim Leers’s picture

I was going to suggest that include was indeed not the same use case as extend

Cool :)

An include is likely to be very particular about where the content is coming from.

In custom themes perhaps, but not in generic reusable themes. Imagine a figure/captioned thing component like so:

<figure>
  {% block content %}
  {{ children }}
  {% endblock %}
  <figcaption>{{caption}}</figcaption>
</figure>

and something that could potentially be captioned, like so:

<marquee>{{ text }}</marquee>

You can then have something like

{% embed "figure.html.twig" with { caption: "Trolling David" } only %}
  {% block content %}
  {% include "beautiful-text.html.twig" with { text: "HI DAVID!!!!!!" } only %}
  {% endblock %}
{% endembed %}

Why would I ever want to be specific about either of those two templates? I'd want to be able to update that gorgeous marquee to something even more epic in my subtheme. And the caption component is always going to behave like that, but perhaps in my subtheme I want to add a class attribute. In either of those examples, there's no need for me to specify a full path to a component, or a particular theme and a component. I'd just want to use the component name, period, so that I could choose to extend that component in my subtheme for my site-specific needs.

If this is the approach, we need to make sure it is well documented, as it creates a non-intuitive inconsistency.

Can you clarify this? (Which inconsistency?)

Wim Leers’s picture

While working on #2702061, I noticed that this fails for a template that uses embed — see #2702061-56: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering). This patch didn't fail because it's used precisely zero times in all of Drupal 8 core.

(That's what frightens me most, now that I've been working a lot on Twig+Drupal integration: we provide all of Twig, but lots of Twig features are not exercised or tested anywhere. Which means Drupal supports Twig, yay! pretty much needs to come with the caveat (Except when it doesn't work because we didn't try using that feature.)…)

davidhernandez’s picture

In custom themes perhaps, but not in generic reusable themes.

I don't think generic reusable themes is our primary use case.

Can you clarify this? (Which inconsistency?)

If an embed/include ends up working differently than extend, we need to make sure people know that because it won't be obvious. A change record alone won't be enough. We just need to make sure that is detailed in the theming guide. That's all.

Fabianx’s picture

#66: The only difference:

In sub-theme/templates/block.html.twig

{% extends "block.html.twig" %}

WORKS!

In sub-theme/templates/foo.html.twig:

{% include "foo.html.twig" %}

leads to endless recursion.

In sub-theme/templates/foo.html.twig:

{% include "something.html.twig" %}

returns the first template in the registry chain. (e.g the path shown in theme_debug).

While with extends the chain is checked where we are currently - IF and only IF the template name is the same.

Wim Leers’s picture

Indeed :)

effulgentsia’s picture

#65 looks great to me. I haven't reviewed it thoroughly enough to RTBC it myself yet, but I didn't find anything significant to complain about during a quick review, and I think making this work per the issue summary is a great theme system improvement.

Some minor stuff:

  1. +++ b/core/modules/system/src/SystemConfigSubscriber.php
    @@ -41,6 +42,10 @@ public function onConfigSave(ConfigCrudEvent $event) {
    +      // Wipe the Twig PHP Storage cache, to ensure templates that extend or
    +      // include another template use the correct list of candidate templates.
    +      // @see \Drupal\Core\Template\ThemeRegistryNodeVisitor
    +      PhpStorageFactory::get('twig')->deleteAll();
    

    Why is the compilation of a given theme dependent on the configuration for which theme is default and/or admin? I read ThemeRegistryNodeVisitor, but don't see a clear answer to that.

  2. +++ b/core/modules/system/src/Tests/Theme/TwigExtensionTest.php
    @@ -49,7 +49,7 @@ function testTwigExtensionFilter() {
    -  function testTwigExtensionFunction() {
    +  function testsTwigExtensionFunction() {
    

    Why?

Re #67: is any/all of what's below "WORKS!" a change from HEAD to #65? If so, what's HEAD's behavior?

Fabianx’s picture

#69:

Uh, that was not clear:

What I meant was:

extends / include in HEAD all behave as described below WORKS! in #65.

In this patch extends gets fixed to work for the recursive case and also find the right template in the chain automatically.

So only the behavior for extends changes, all other things remain the same.

Wim Leers’s picture

#69.1: see #40 for original analysis. But I understand why you find this confusing, because from a Drupal POV, you'd assume that these compiled Twig templates are keyed/hashed/identified by the current theme, or even tied to their file path, right? That'd be a great assumption, but you'd be sorely mistaken. Twig identifies compiled templates not by the current theme, nor by their location on disk, but by their "name". And so if you do {% extends "block.html.twig" %}, the name that identifies a template is block.html.twig, not the resolved path. And so that means that the compiled template that this example points to will change depending on the default theme.

#69.2: hah, nice catch, will revert back!


#70: So only the behavior for extends changes, all other things remain the same. — exactly!

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

This has now been blocked on reviews for >5 months. Drupal 8.2 could have shipped with this fixed. So all themes for 8.2 and later could have been built with a functioning-as-intended Twig extends.

:(

Wim Leers’s picture

Category: Task » Bug report

Marking this as the bug that it is.

Wim Leers’s picture

Issue tags: +DrupalWTF
Fabianx’s picture

Status: Needs review » Needs work

In #71: You stated "Will revert back", so this is likely that people thought there was work to do in this issue before RTBC'ing it.

Wim Leers’s picture

Status: Needs work » Needs review

Well that's just a tiny tiny detail that has absolutely zero functional impact. It really is blocked on review.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

yareckon’s picture

Hey, I'm not competent to review this patch without a lot of study, but would use the hell out of it when this gets in.
@Wim Leers, is there a new patch needed for the feedback in #69 or not, regardless of the size of the issue?

I could take on the roll of re-rolling against whatever version branch we need, if you could clear up if there are still substantive changes to be made.

This is a call out to theme system people to come take a look at this! @fabianx, @effulgentsia, @joelpittet, @lauriii This seems like a RTBC several times, and would be terrible to have this slip more and more into future versions / code freezes. Could one of you clear up which branch this needs to be rerolled against?

Jo Fitzgerald’s picture

Status: Needs review » Needs work

The last submitted patch, 80: 2387069-80.patch, failed testing.

jonathanshaw’s picture

This looks like a bugfix that fully preserves BC. Is it therefore eligible for an 8.3 patch release?

almaudoh’s picture

Is @mithunray001 some kind of smart spambot that copies previous comments? (@davidhernandez #63) Or is he actually a person?

Jeff Burnz’s picture

#84 human or not, it's spam.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
22.31 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 86: 2387069-86.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
21.67 KB

I think I rolled the previous patch against 8.3.x by mistake, let's try again.

Status: Needs review » Needs work

The last submitted patch, 88: 2387069-88.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 90: extends-2387069-90.patch, failed testing.

frederickjh’s picture

Hi!
I found this issue while trying to solve an issue with why the attach_library() twig function was not loading my css file. The twig debug in the browser was showing my twig template was being used but the css file did not show up in the sources.

Removing the {% extends "foo.html.twig" %} and pasting in the contents of the foo.html.twig into my twig template allowed the css file that was to be loaded by the twig attach_library() function to load.

Is this related to this issue or should I open a new issue for this?

Thanks!
Frederick

Cottser’s picture

@frederickjh that would be a separate issue. There is /core/themes/seven/templates/image-widget.html.twig that does this, though:

{% include '@classy/content-edit/image-widget.html.twig' %}
{{ attach_library('classy/image-widget') }}
jonathanshaw’s picture

Issue tags: +Needs reroll
Saviktor’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

I could apply the patch, no reroll is needed

effulgentsia’s picture

Reuploading #90 to see if that fixes the "Unable to generate test groups" problem.

Status: Needs review » Needs work

The last submitted patch, 96: extends-2387069-90.patch, failed testing. View results