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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

Is this happening with all the templates, or just these two in particular?

star-szr’s picture

I 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.

lauriii’s picture

Is it only broken for same theme suggestion so that a more specific suggestion still overrides the less specific template in a subtheme?

star-szr’s picture

Or in other words…

Jeff Burnz’s picture

I 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.:

block--THEMENAME-search.html.twig
user--full.html.twig

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.

sub-theme/templates/block/block--search-form-block.html.twig
sub-theme/templates/user/user.html.twig

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.

Jeff Burnz’s picture

Let 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

davidhernandez’s picture

I 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.

lauriii’s picture

Priority: Critical » Major

I cannot reproduce this neither. De-escalating this to Major because doensn't fullfill the requirements of Critical issue at the moment regarding the documentation.

Jeff Burnz’s picture

#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.

davidhernandez’s picture

I 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?

Jeff Burnz’s picture

Yes, 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.

davidhernandez’s picture

I'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?

joelpittet’s picture

@davidhernandez

Yes, you have to sub-theme Bartik.
davidhernandez’s picture

What I wrote in #12 assumes a subtheme of Bartik. I neglected to write two subs throughout.

joelpittet’s picture

Just trying to alleviate the confusion indicated in your opening statement;)

I'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.

joelpittet’s picture

@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.

hass’s picture

Has the subtheme a block.html.twig? This was at least required in D6 and D7.

joelpittet’s picture

We'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.

joelpittet’s picture

@hass bartik has a block.html.twig, and if it's a copy of bartik it likely has one in the subsub theme too.

joelpittet’s picture

Issue tags: +Needs tests

This should be something we have a test for to prevent regressions in template inheritance.

davidhernandez’s picture

I 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.

Jeff Burnz’s picture

@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.

lauriii’s picture

Status: Active » Needs review
FileSize
0 bytes

I created test patch so anyone who wants to work on this can use this as a base.

lauriii’s picture

FileSize
6.06 KB

Here's a patch with some content :P

The last submitted patch, 23: cannot_override-2414255-23-tests.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 24: cannot_override-2414255-24-tests.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.75 KB

Removed some debug changes from my patch.

Status: Needs review » Needs work

The last submitted patch, 27: cannot_override-2414255-27-tests.patch, failed testing.

davidhernandez’s picture

Title: Cannot override template in Classy theme » Subtheme template inheritance working in reverse order
Priority: Major » Critical
Issue summary: View changes

I'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.

davidhernandez’s picture

Issue summary: View changes
idebr’s picture

Could 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.

joelpittet’s picture

@idebr highly unlikely, CSS/JS files are dealt with a very different system than template loading and template suggestions.

lauriii’s picture

Status: Needs work » Needs review
FileSize
6.53 KB

Just a test to see if I found the reason and if it causes some additional problems

lauriii’s picture

Removed unrelated template added in the previous patch. Otherwise the same

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -343,6 +343,7 @@ protected function build() {
 
+    $this->baseThemes = array_reverse($this->baseThemes);
     // Process each base theme.
     foreach ($this->baseThemes as $base) {

So the base themes are coming from the following piece of code:

      $active_theme = \Drupal::theme()->getActiveTheme();
      $this->theme = $active_theme;
      $this->baseThemes = $active_theme->getBaseThemes();

... 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.

davidhernandez’s picture

I 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?

lauriii’s picture

I 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.

lauriii’s picture

I 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.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
545 bytes

I 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?

dawehner’s picture

I 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?

+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.

I 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.

Yeah, thank you for taking the time to fix it properly.

joelpittet’s picture

Status: Needs review » Needs work

I 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(), in init() 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 in init())

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 are

davidhernandez’s picture

Joel, are you proposing we remove the array_reverse() from init()? What is the reasoning for the current ordering since it's being reversed there?

joelpittet’s picture

@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.

davidhernandez’s picture

If 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.

joelpittet’s picture

Hmm, 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 in init(), and remove the other array_reverse() in init(), it would be a bit cleaner, and we can reverse in build() 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?

davidhernandez’s picture

and 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.

joelpittet’s picture

@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 the array_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.

davidhernandez’s picture

That 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.

joelpittet’s picture

Takes me so long to be clear:P and longer to clarify...

Gábor Hojtsy’s picture

#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.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
16.17 KB
  • Let's clean up the special case and not longer have code there but rather use the services we have, so we don't have to worry any longer whether the array_reverse does things in the right order
  • Let's also document the order of the base themes.
  • Let's write a dedicated test for the order of the preprocess functions.
  • It still passes the excellent written test in #27

Status: Needs review » Needs work

The last submitted patch, 51: 2414255-51.patch, failed testing.

tim.plunkett’s picture

Test looks solid.

  1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
    @@ -184,38 +173,12 @@ protected function init($theme_name = NULL) {
    +      $this->theme = \Drupal::theme()->getActiveTheme();
    

    Why not inject the theme manager as well?

  2. +++ b/core/modules/system/src/Tests/Theme/RegistryTest.php
    @@ -28,7 +29,7 @@ class RegistryTest extends WebTestBase {
    -  function testRaceCondition() {
    +  function ptestRaceCondition() {
    

    :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
19.41 KB
4.18 KB

Why not inject the theme manager as well?

Because this would be otherwise a circular dependency.

Status: Needs review » Needs work

The last submitted patch, 54: 2414255-54.patch, failed testing.

dawehner queued 54: 2414255-54.patch for re-testing.

dawehner’s picture

Status: Needs work » Needs review

.

davidhernandez’s picture

Manually 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?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/system/src/Tests/Theme/RegistryTest.php
@@ -15,7 +16,7 @@
-class RegistryTest extends WebTestBase {
+class RegistryTest extends KernelTestBase {

Woot.

The core changes look good to me, and @davidhernandez manually tested. I think this is RTBC, thanks @dawehner!

star-szr’s picture

+1, looks good to me as well!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
class TestRegistry extends Registry {
...
  public function setBaseThemes(array $base_themes) {
    $this->baseThemes = $base_themes;
  }

Registry no longer has a base_themes property.

   * @param \stdClass $theme
   *   The loaded $theme object as returned from
   *   ThemeHandlerInterface::listInfo().
...
  protected function processExtension(&$cache, $name, $type, $theme, $path) {

Can we get a followup to fix this documentation $theme here seems to always be a string.

  • catch committed 4cd30ea on 8.0.x
    Issue #2414255 by lauriii, Jeff Burnz, dawehner, davidhernandez:...
catch’s picture

Status: Needs work » Fixed

Doh, 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.

dawehner’s picture

  • alexpott committed 48f5de4 on 8.0.x
    Issue #2425201 by jibran, dawehner: Small cleanup follow from #2414255.
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.