Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Subthemes with a multi-parent hierarchy are inheriting templates from the wrong parent. It looks like the ordering is reversed.
Suppose the following:
Classy -> Base theme 1 -> Base theme 2 -> Active theme
If every theme has node.html.twig
the template in the Active theme
is used. If Active theme
does not have the template, it gets used in the following order Classy -> Base theme 1 -> Base theme 2
. This is opposite of what it should do. It should first check Base theme 2
, then Base theme 1
, and then check Classy
last.
Proposed resolution
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff.txt | 4.18 KB | dawehner |
#54 | 2414255-54.patch | 19.41 KB | dawehner |
#51 | 2414255-51.patch | 16.17 KB | dawehner |
#39 | subtheme_template-2414255-39.patch | 545 bytes | davidhernandez |
#34 | subtheme_template-2414255-34.patch | 5.22 KB | lauriii |
Comments
Comment #1
davidhernandezIs this happening with all the templates, or just these two in particular?
Comment #2
star-szrI hate to ask but have you cleared caches? Any other information you can provide would be great. Unfortunately I can't test this myself right now.
Comment #3
lauriiiIs it only broken for same theme suggestion so that a more specific suggestion still overrides the less specific template in a subtheme?
Comment #4
star-szrOr in other words…
Comment #5
Jeff Burnz CreditAttribution: Jeff Burnz commentedI am running quite short on time today so I just did some quick tests:
#1. I only tested with these two templates, since they are the only two that are in Classy (atm) that I am overriding.
#2. Yes, caches are cleared etc, running debug: true and auto_reload: true also, i usually run these settings anyway at the moment
#3. YES - using more specific template suggestion worked for both these templates, i.e.:
Using the more specific template will override the Classy (base theme) template. Good call.
#4. Choose one of the mentioned templates in Classy and copy this to your sub-theme, I have this in a sub-directory, e.g.
Then I am sub-theming the sub-theme, this may or may not be important, it works precisely like this:
core -> base theme (Classy) -> sub theme -> sub-sub-theme (active default theme)
Thats what I got so far, sorry for the lack of debugging, I am a bit of a time crunch today.
Comment #6
Jeff Burnz CreditAttribution: Jeff Burnz commentedLet me quickly clarify that from above #3 - using a more specific template is actually using the wrong template name (at least the way I see it). Let me explain:
This the template suggestion, however this is the name of the sub-sub-theme, not the actual theme where the template is, "pixture-reloaded" is the last theme in the chain, the actual theme where this template resides is called something entire different.
block--pixture-reloaded-search.html.twig
Comment #7
davidhernandezI checked this in a fresh install using Bartik. Bartik has a copy of
block--search-form-block.html.twig
. The system used Bartik's version of the template. When I removed it, it used Classy's version of the template. (When I put it back it used Bartik's again.) That is the correct behavior. See the attached screenshot. I will say I had to completely reinstall Drupal and replace the copies of settings.php, settings.local, etc., after some recent changes.Comment #8
lauriiiI cannot reproduce this neither. De-escalating this to Major because doensn't fullfill the requirements of Critical issue at the moment regarding the documentation.
Comment #9
Jeff Burnz CreditAttribution: Jeff Burnz commented#7 - I did new git clone of core and reinstalled, same result - please note you are not actually replicating the bug - you need to sub-theme Bartik to see the bug.
See the screenshot, also I attach a simple sub theme, this is Bartik copied with every thing removed (templates removed etc).
EDIT: I am not extending the search block like Bartik is, I am overriding the whole block. Just for clarification with regards to this particular template.
Comment #10
davidhernandezI tried this with adding a user.html.twig (which is not extended) template to Bartik and it worked fine. Are you saying you have to be 3 themes deep to produce the problem?
Comment #11
Jeff Burnz CreditAttribution: Jeff Burnz commentedYes, you have to sub-theme Bartik.
It doesn't matter about extending the template, I was just clarifying that I am not doing this compared to Bartik.
Happens with comment template as well, see screenshot. Look at the top - you see my Bartik Copy sub-theme pulls node template from Bartik, but comment template from Classy, that is just not right.
Comment #12
davidhernandezI'm confused as to whether there is a problem with the subsubtheme overriding a template, or there is a problem with the subsubtheme inheriting the right template.
Classy has comment.html.twig. Bartik has comment.html.twig. Bartik uses its comment.html.twig. If a subtheme is created, with Bartik as the base theme, (Classy -> Bartik -> subtheme) it does not use Bartik's comment.html.twig. It uses Classy's comment.html.twig. The subtheme uses Bartik's node.html.twig. Likely because Classy does not have a node.html.twig.
Is that the problem, or is it that when you add comment.html.twig to the subsubtheme it doesn't get used at all?
Assuming the first scenario, it sounds like when there are multiple levels of inheritance, Drupal is using the topmost, instead of the bottommost parent?
Comment #13
joelpittet@davidhernandez
Comment #14
davidhernandezWhat I wrote in #12 assumes a subtheme of Bartik. I neglected to write two subs throughout.
Comment #15
joelpittetJust trying to alleviate the confusion indicated in your opening statement;)
Comment #16
joelpittet@Jeff Burnz thanks for the screenshot, it should be pulling bartik's template or if you still have a comment.html.twig in your copy (which I suspect you do) it should pick up that one.
Comment #17
hass CreditAttribution: hass commentedHas the subtheme a block.html.twig? This was at least required in D6 and D7.
Comment #18
joelpittetWe'll have to revisit the template loading to make sure the logic is correct with base themes template suggestions.
@Cottser any chance you want to snap this one up as you've dug deep into the loading with @MarkCarver and @Fabianx from what I remember? Or @davidhernandez or @lauriii want to take a crack at it.
I'll step through it likely tonight or tomorrow evening if nobody beats me to the punch.
Comment #19
joelpittet@hass bartik has a block.html.twig, and if it's a copy of bartik it likely has one in the subsub theme too.
Comment #20
joelpittetThis should be something we have a test for to prevent regressions in template inheritance.
Comment #21
davidhernandezI will not likely touch this in the next few hours, as I attempt to drive home in the approaching blizzard. I'll try to at least test/verify it tonight.
Comment #22
Jeff Burnz CreditAttribution: Jeff Burnz commented@davidhernandez - it's an inheritance issue I assume, I'll leave the issue title for now (agree its a crap title) until the actual bug is located.
@joelpittet - correct, I have not touched Classy or Bartik in these tests, just made that very simple Bartik sub-sub-theme which has no templates - it should inherit Bartiks templates, but does not, it reverts to inheriting Classy's wherever there IS a Bartik template of the same name. I think we are all clear now on what is going on.
@hass - in D8 you don't need the root template in the same dir as per D7.
Comment #23
lauriiiI created test patch so anyone who wants to work on this can use this as a base.
Comment #24
lauriiiHere's a patch with some content :P
Comment #27
lauriiiRemoved some debug changes from my patch.
Comment #29
davidhernandezI'm confirming what Jeff is experiencing. The template inheritance is working in reverse order. Instead of looking at the closest parent, it looks at the top and then works its way down.
This wasn't caused by #2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders. I checked out commits prior to that one and the problem still existed. I wouldn't be surprised if this bug has been around for a while because subtheming isn't an everyday thing to do while working on core. It is a normal use case in the wild, however.
I think this is critical because the bug makes subtheming useless.
Comment #30
davidhernandezComment #31
idebr CreditAttribution: idebr commentedCould this be related to #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name? While this issue covers templates files and the other is about css files, both issues are about theme inheritance.
Comment #32
joelpittet@idebr highly unlikely, CSS/JS files are dealt with a very different system than template loading and template suggestions.
Comment #33
lauriiiJust a test to see if I found the reason and if it causes some additional problems
Comment #34
lauriiiRemoved unrelated template added in the previous patch. Otherwise the same
Comment #35
dawehnerSo the base themes are coming from the following piece of code:
... in order to avoid that kind of bug in the future I think we should document both in Registry as well as ActiveTheme in which order the baseThemes are.
... Fixing it inline in the build() method though is wrong IMHO, ... $this->baseThemes should stay in the expected order at all times.
Comment #36
davidhernandezI don't know how else we can fix this other than in build(). Once the registry is built, it has the wrong template. As far as I can tell, that foreach loop is the problem. $this->processExtension() is getting the templates for each theme, but does so individually in the loop, with the $cache passed by reference. If the loop is in the wrong order for inheritance how else do we fix it?
Comment #37
lauriiiI think there is a point in @dawehner's comment that we shouldn't fix it by reversing the array. I thought the same when I first time tried to figure out what was happening. The array is being structured from top to bottom so that in the top there's heaviest template which makes kind of sense to me. We structure array so that we first take the main theme and then we just add all the depending theme for the bottom of the array. When that is being used in the foreach loop that of course means that it works exactly the reversed way so for the loop we have to reverse it to function how it should.
I think the question in this case is that which way it makes more sense to people. To have the heavier theme (primary theme) in the top of the array or in the bottom. For me it makes more sense the way it is but that should be discussed properly.
Leaving this as needs work because test theme names should be changed to be more clear. At the moment they don't make any sense.
Comment #38
lauriiiI guess I misunderstood @dawehner's point. If you meant that we shouldn't change the order to $this->baseThemes but instead create new variable for that, with that I agree. It makes it messy if we switch order for the original array.
Comment #39
davidhernandezI don't think we're looking to permanently reverse the array, so copying it to a temp array is fine with me. Could we use array_reverse in the foreach to do the same?
Comment #40
dawehner+1 On top of that it would be nice, as I tried to express ( ;) ) to document the occurrences of baseThemes in its intended order of elements.
Yeah, thank you for taking the time to fix it properly.
Comment #41
joelpittetI thought I posted this last night but I guess I missed the submit button:S
I think that reverse should be permanent, but not in
build()
, ininit()
like the other one that is permanent there.FYI current order of
$this->baseThemes = [bartik, classy]
if my subtheme is bartik2 and it's base is bartik, (without a$theme_name
ininit()
)It's already reversed on the object in the
init()
when a $theme_name is passed in, so I'd have to assume it should be reversed in both cases or neither, but it depends on which other systems areComment #42
davidhernandezJoel, are you proposing we remove the array_reverse() from init()? What is the reasoning for the current ordering since it's being reversed there?
Comment #43
joelpittet@davidhernandez I'm proposing doing the array_reverse consistently in init() and not in build(). Reason is so we don't have inconsistent ordering from different code paths, and hopes that it can be cached after init and shouldn't have to hit the reversal as often.
I have no idea the reasoning behind the reversal because it wasn't commented when it was put in.
If you call init then build, depending on the code path (with our without a $theme_name) you will get different orders, regardless of the reversal in build(). So it should be reversed in the other code path or removed from there.
I'm betting there is a reason for the reversal in the init() originally.
Comment #44
davidhernandezIf we keep reversing the order I don't see why we don't just change it completely then. I checked the original issue, and that array_reverse has been there since the beginning when Registry was committed, but I see no mention of the reason for it. It just appeared.
The current order [Bartik2, Bartik, Classy] I think makes sense. When you are looking through the base themes you probably expect the closest parent first. The build, however, uses a cascade, so it needs the closest parent last. In any case, I don't know enough about why those logic choices were made, so I defer to whatever Joel things is best.
Comment #45
joelpittetHmm, I like your idea better, now:) Mine was just to slightly modify the existing assuming the existing was intentional and correct.
I wonder though if we leave it
$this->baseThemes = [Parent, GrandParent, GreatGrandParent]
order ininit()
, and remove the otherarray_reverse()
ininit()
, it would be a bit cleaner, and we can reverse inbuild()
for that specific case, but not modifying it like you have in #39.EDIT: this reads wrong! I WOULD like #39 + removal of the array_reverse in init() + tests.
@lauriii mind tackling the tests again?
Comment #46
davidhernandezand remove the other array_reverse() in init()
I think that's what I was suggesting. But isn't this reverse what is causing the [parent, grandparent, etc] order? So if it was removed we wouldn't need to do a reverse in build() right? What I don't know is what code elsewhere is expecting the existing order, and if there is some logic behind it.
#39 shouldn't be permanently changing the order. Just reversing it for the loop.
Comment #47
joelpittet@davidhernandez That reverse isn't actually called in
init()
under most circumstances.It's the lack of it in the code path that causes the need for it in
build()
, the path that doesn't run thearray_reverse
that gets run more often, and maybe always for all that I noticed when stepping through it.I like #39 because it doesn't modify the order that the baseThemes are created in, it only modifies the order when needed (like you mentioned). But this also needs a comment to explain why we are doing this.
Comment #48
davidhernandezThat reverse isn't actually called in init() under most circumstances.
Yeah, I misread the 'if'. Thanks for clearing that up. So we'll proceed with #39, adding the tests and comments.
Comment #49
joelpittetTakes me so long to be clear:P and longer to clarify...
Comment #50
Gábor Hojtsy#2389735: Core and base theme CSS files in libraries override theme CSS files with the same name is shockingly similar and practically the same solution to process base themes in reverse order for CSS. Should these two have the same solution? Both should be critical? Folded into one? Note that we use #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name in the multilingual_demo distro to ensure that our style customization works.
Comment #51
dawehnerComment #53
tim.plunkettTest looks solid.
Why not inject the theme manager as well?
:)
Comment #54
dawehnerBecause this would be otherwise a circular dependency.
Comment #57
dawehner.
Comment #58
davidhernandezManually tested the patch in #54. All the template inheritance I looked for worked correctly. I used node templates from core -> Classy -> Bartik -> theme1 -> theme2. In all cases, the right template was grabbed as I worked my way from theme2 back up to core.
I don't know enough about all the code changes in the patch to RTBC it myself. Anyone else have time to review/test it?
Comment #59
tim.plunkettWoot.
The core changes look good to me, and @davidhernandez manually tested. I think this is RTBC, thanks @dawehner!
Comment #60
star-szr+1, looks good to me as well!
Comment #61
alexpottRegistry no longer has a base_themes property.
Can we get a followup to fix this documentation $theme here seems to always be a string.
Comment #63
catchDoh, cross-committed with the CNW.
I think we can remove the dead code in the same follow-up that fixes that comment though? Moving to fixed tentatively.
Comment #64
dawehnerOpened the follow up: #2425201: Small cleanup follow from 2414255