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:
core/themes/stable/templates/foo.html.twig
core/themes/classy/templates/foo.html.twig
contains{% extends "foo.html.twig" %}
(to add a specific class) and expects Stable'sfoo.html.twig
to be extendedcore/themes/bartik/templates/foo.html.twig
contains{% extends "foo.html.twig" %}
(to override a Twig block) and expects Classy'sfoo.html.twig
to be extended — or, really, any of its ancestor themes, because it couldn't care less if Classy'sfoo.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.
Comments
Comment #1
Dragan Eror CreditAttribution: Dragan Eror commentedMaybe to check the path of template and not to include "self" if tries.
Whether someone checked out for this issue in Twig community?
Comment #2
star-szrPutting this on my list to play with now that #2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders is in.
Comment #3
star-szrI 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.
Comment #4
star-szrCreated an issue upstream to ask about this: https://github.com/twigphp/Twig/issues/1627
Comment #5
davidhernandez@Cottser, I saw you closed the github issue. Does that mean you know where to poke around?
Comment #6
star-szrSomewhat, it sounds like this is going to be a hard problem though.
Comment #7
star-szrStill thinking about this but don't want to hog it, especially since I'm not actively working on code for this issue.
Comment #8
star-szrHere'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:
Comment #9
star-szrForgot some tags.
Comment #10
star-szrComment #11
star-szrJust 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 :)
Comment #12
davidhernandezI 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.
Comment #13
star-szr@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:
A rather obvious example but when using the registry loader,
extends "bar.html.twig"
could map to this, just not explicitly.Comment #14
davidhernandezThese 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.
Comment #15
star-szrRegarding "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…
Comment #16
Fabianx CreditAttribution: Fabianx commentedPotentially add a new issue to allow extending from the base parent:
{% extends 'base-theme:foo.html.twig' %}
Comment #17
star-szrPotentially _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.
Comment #18
star-szr_self only works for imports it seems: https://api.drupal.org/api/drupal/core%21vendor%21twig%21twig%21lib%21Tw...
Comment #19
star-szrAnother 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?
Comment #20
star-szrJust a reroll.
Comment #21
Wim Leers#12:
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 …
… 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:
… because that means those modules/themes are violating semantic versioning.
#13:
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 don't see how you can.
#16:
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.Comment #22
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThat 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:
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:
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.
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:
like the theme registry would do (it selects an explicit template first).
Then it would output:
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:
and for the one in classy:
and only add those hooks, whose templates do exist.
Only question remaining now is: Does our theme registry support this kind of information retrieval?
Comment #23
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedIt does not, but this fixes it:
gives output:
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).
Comment #25
Wim LeersTook 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:
And it apparently was even in the IS:
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, thenbase-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 %}
intoThat 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 thenThemeRegistryLoader
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.
Comment #26
Wim LeersThanks 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"
toextends ['path/to/parent/theme/FOO.html.twig', 'path/to/grandparent/theme/FOO.html.twig']
andinclude "FOO.html.twig"
toinclude ['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:Which can now simply be this:
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:
See how it respects the full hierarchy: Classy, Stable, then the module. All thanks to the theme registry.
Comment #27
Wim LeersNote 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.
Comment #29
Wim LeersThe 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.
Comment #30
Wim LeersThis fixes the unit test failures. Should go from 24 to 6 failures.
Comment #31
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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:
In addition to include and extends.
Comment #33
Wim LeersI 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
andinclude
, they cover 95% of use cases.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'sblock.html.twig
, not the parent theme's. That's what that check is for.Therefore this makes no sense:
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 anTwig_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.
Comment #34
Wim LeersThere are 4 failures in
Drupal\system\Tests\Theme\TwigExtensionTest
. In:testTwigExtensionLoaded()
testTwigExtensionFilter()
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.
Comment #35
Wim LeersThis will need to update the CR at https://www.drupal.org/node/2381103.
Comment #36
Wim LeersRe-uploading #34 so testbot can test it.
Comment #38
Wim Leers#34 contained one typo that was causing a lot of fails :P
Comment #40
Wim LeersAfter 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:
The first
drupalGet()
gets this as the Twig expression to load parents:The second
drupalGet()
gets this (i.e. after changing the default theme):(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.
Comment #41
Wim LeersAnd 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!
Comment #42
Wim LeersComment #44
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedTests++ - Great catch on the rebuild.
- The interdiff in #41 looks incorrect somehow. At least I cannot see it fixing tests ...
Comment #45
Wim Leers#44: you're right! No idea how that happened. Right interdiff attached.
Comment #46
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedJust 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).
Comment #47
Wim LeersAs 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.
Comment #48
Wim LeersAnd here's the fix.
Comment #49
Wim LeersIn doing #47+#48, I noticed some misnamed variables. This fixes that.
Comment #51
Wim Leers#46: done — restored
TwigRegistryLoader
, plus its test coverage that I removed in #41.Comment #52
Wim LeersSigh, d.o--
Reuploading the patch for #51 so that it gets tested.
Comment #53
Wim LeersAt #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.
Comment #54
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedHm, 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.
Comment #56
Wim Leersa) 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 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:
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.
Comment #57
Wim LeersDone.
Comment #58
Wim LeersOops, wrong interdiff.
Comment #59
Wim LeersI missed some
include
-related things in #57.Comment #60
Wim LeersAlso, 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.
Comment #61
star-szrReally 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.
Comment #62
Wim LeersDiscussed 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:
include
andembed
. Because in either of those cases, you just want the active theme's version of thatinclude
/embed
to be loaded. So actually, in those cases using HEAD's theme registry-basedTwigLoader
totally makes sense!In conclusion: HEAD works fine for
include
andembed
, only fails when usingextends
. Of course, usingextends
is absolutely crucial (for the reasons explained many times before in this issue).Hence the changes in #54+#55 totally make sense.
Comment #63
davidhernandezI 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.
Comment #64
Wim LeersCool :)
In custom themes perhaps, but not in generic reusable themes. Imagine a figure/captioned thing component like so:
and something that could potentially be captioned, like so:
You can then have something like
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.
Can you clarify this? (Which inconsistency?)
Comment #65
Wim LeersWhile 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
pretty much needs to come with the caveat …)Comment #66
davidhernandezI don't think generic reusable themes is our primary use case.
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.
Comment #67
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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.
Comment #68
Wim LeersIndeed :)
Comment #69
effulgentsia CreditAttribution: effulgentsia at Acquia commented#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:
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.Why?
Re #67: is any/all of what's below "WORKS!" a change from HEAD to #65? If so, what's HEAD's behavior?
Comment #70
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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.
Comment #71
Wim Leers#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 isblock.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: — exactly!
Comment #73
Wim LeersThis 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
.:(
Comment #74
Wim LeersMarking this as the bug that it is.
Comment #75
Wim LeersComment #76
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedIn #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.
Comment #77
Wim LeersWell that's just a tiny tiny detail that has absolutely zero functional impact. It really is blocked on review.
Comment #79
yareckon CreditAttribution: yareckon commentedHey, 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?
Comment #80
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Comment #82
jonathanshawThis looks like a bugfix that fully preserves BC. Is it therefore eligible for an 8.3 patch release?
Comment #86
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Comment #88
jofitz CreditAttribution: jofitz at ComputerMinds commentedI think I rolled the previous patch against 8.3.x by mistake, let's try again.
Comment #90
yogeshmpawarRemoved coding standard issues in #88 & also added a interdiff.
Comment #92
frederickjhHi!
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 twigattach_library()
function to load.Is this related to this issue or should I open a new issue for this?
Thanks!
Frederick
Comment #93
star-szr@frederickjh that would be a separate issue. There is
/core/themes/seven/templates/image-widget.html.twig
that does this, though:Comment #94
jonathanshawComment #95
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedI could apply the patch, no reroll is needed
Comment #96
effulgentsia CreditAttribution: effulgentsia at Acquia commentedReuploading #90 to see if that fixes the "Unable to generate test groups" problem.
Comment #99
Pranali.addweb CreditAttribution: Pranali.addweb at AddWeb Solution Pvt. Ltd. commented{% extends "foo.html.twig" %} in Twig templates does not respect theme inheritance
Replacing variables of a class using template extends here is the example to understand it better:
Template 1 ( foo.html.twig )
Template 2
This code is just an example to how to extend and work with inherit a template to an another one. And use the properties. I hope this article would be useful to solve this inherit the problem.
Comment #100
effulgentsia CreditAttribution: effulgentsia at Acquia commentedJust seeing if this fixes the #90 / #96 test failure. I have not reviewed the actual patch yet.
Comment #104
JacineThis is sort of off-topic. However, it's been bothering me for a while, and I gotta get it out.
IMO, core themes, at least one of them, should not being using these features of Twig at all. By "these features" I mean: Twig blocks, includes, embeds and extends.
It may be an unpopular opinion, but I strongly feel the implementation of these features are architectural-related decisions, which Drupal should NOT be making. It's bad enough we have assumptions all over the place while working with Drupal to overcome. These features of Twig are a great way to wrangle some of these issues, in our custom themes. However, I'm seeing an increased number of templates with arbitrary Twig blocks and some use of extends, in contrib, which is following core's lead. I just wish a line in the sand could be drawn and Drupal would choose to get out of the way, and just straight up NOT use these things and KISS.
Seems like the kinds of issues coming up in this thread could be avoided entirely by just using Twig in a basic, non-architectural way, to print variables. If there are cases where sharing markup is desired, then maybe that's a place where better theme hook suggestions should be implemented, since that is Drupal's forte.
Right now I'm using classy as a base theme, and I'm looking to override the base field template, but realizing there are a bunch of more specific templates in that theme extending it. So, I either have to make my implementation extendable in a way that works with classy's extensions, re-implement all those templates, or just abandon Classy entirely, which kind of sucks, because there are quite a few templates in there that are useful, and timesaving.
Anyway, that's my 2 cents that no one asked for (sorry lol)... 😇
Comment #105
davidhernandezIt is a fair criticism. Base themes should help, not hurt.
Comment #106
lauriiiThere's no way to ensure that Stable theme template is compatible with the type of extending the override is doing, so this might still break the theme. Removing the template from Classy would also be a BC break so it shouldn't happen. IMHO error stating that template is extending non-existing template is easier to debug than random bugs or errors caused by the fact that the template is extending template that it initially wasn't designed to extend.
I'm wondering if the best approach for solving this issue would be closing this issue and deprecating the theme registry loader meaning that we always require specifying which specific instance template is extending. This would reduce hard to debug theme inheritance problems, and would provide themers with clear documentation about which template is being extended.
Theme registry loading templates isn't broken per se, but It's easy to forget about this feature. It's also hard to work with because there's no clear way to see if a template is being extended because of the template can be loaded dynamically for extending. Also, our debugging tools don't provide information on which template is being extended.
What would you expect the behavior to be in this case?
Comment #107
JacineI would *hope* that Drupal just didn't use extends, block, etc, and that it would just either duplicate the markup and print variables, or create a theme hook suggestion that facilitates using one template, so I don't have to worry about Twig architecture when overriding a template. In this exact case, these two templates extend field--text.html.twig.
field--text.html.twig
field--text-long.html.twig
field--text-with-summary.html.twig
I don't care about these templates in my theme, and I'm overriding field.html.twig, which should really be enough by itself for my purposes. However, because of their presence in Classy, and the fact that they're more specific, I have to consider overriding them, or more likely just leaving them alone, to prevent future issues, even though, I don't need distinct classes (clearfix/text-formatted classes) for text fields, and I'm not using a text with summary field anywhere on this build.
This is just one example. Can I deal with this? Of course.... However, I'm chiming in on a more fundamental level to protest Drupal using Twig in this way, because I feel like Drupal is overstepping, and contrib WILL follow, which can balloon this into a real problem. I don't expect special Drupal Twig functionality/inheritance/trickery stuff to be happening in the background. I want Drupal to do its thing, in terms of the theme registry and suggestions with basic templates, and Twig to be left alone. That way both can have their own role, documentation and teachability without any additional WTFs along the way.
Comment #108
markhalliwellDiscussed this with @lauriii a bit in slack. I really haven't read the entire issue yet, but I share the concern about using specific names (e.g.
@classy
/@stable
) when extending templates. From my experience, it has done nothing but cause issues when in a multi-site environment where the "theme" is really a grandchild theme.A few quick notes:
@baseTheme
; a way to target the immediate parent theme dynamically. It shouldn't be used in core; it's more for site building (as needed)Comment #112
lauriiiI filed #3144443: Phase out Drupal\Core\Template\Loader\ThemeRegistryLoader for the discussion here.