Problem/Motivation

When you have a complex setup of a couple of themes that are inheriting each other, something like:

theme_1 <- theme_2 <- theme_3 <- client_theme

where theme_1 is the starting base theme, and each theme is overriding various libraries from parent themes library override definitions can fail when multiple themes are overriding the same library.

In LibraryDiscoveryParser::applyLibrariesOverride() next call on line 339

$all_libraries_overrides = $active_theme->getLibrariesOverride();

Will return array of library overrides for all themes but in the wrong order, like

[
  'theme_3' => [...],
  'theme_2' => [...],
  'theme_1' => [...],
  'client_theme' => [...],
]

The correct order would be

[
  'theme_1' => [...],
  'theme_2' => [...],
  'theme_3' => [...],
  'client_theme' => [...],
]

As a consequence of this wrong order applyLibrariesOverride() can and will fail for libraries that are overridden for example in theme_1 and theme_3 because override is applied in the wrong order. Order is important for now because of #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides.

Proposed resolution

Active theme libraries-override array should respect theme inheritance order and have parent themes in the correct order.

Remaining tasks

  • Figure should we fix some other definitions also like 'libraries' because they are also in the wrong order. Maybe this affects the loading of resources in the wrong order?

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

CommentFileSizeAuthor
#28 Screenshot 2023-10-27 at 11.24.48 AM.png33.19 KBsmustgrave
#26 23-26.txt1.01 KBtonibarbera
#26 3073053-26.patch5.67 KBtonibarbera
#23 3073053-23.patch5.94 KBpooja saraah
#13 libraries-override-wrong-override-order-3073053-13.patch5.79 KBpivica
#12 libraries-override-wrong-override-order-3073053-12.patch3.15 KBpivica
#12 interdiff-3073053-8-12.txt3.15 KBpivica
#8 libraries-override-wrong-override-order-3073053-8.patch2.9 KBPrimsi
#8 libraries-override-wrong-override-order-3073053-8.testonly.patch2.15 KBPrimsi
#2 libraries-override-wrong-override-order-3073053-2.patch768 bytespivica

Issue fork drupal-3073053

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pivica created an issue. See original summary.

pivica’s picture

pivica CreditAttribution: pivica at MD Systems GmbH commented
Status: Active » Needs review
FileSize
libraries-override-wrong-override-order-3073053-2.patch768 bytes

Here is a first patch that is fixing order for the libraries-override part.

I am not 100% sure should we reverse $base_themes on the start for all getActiveTheme() call, the comment that is explaining correct order is a bit confusing:

   * @param \Drupal\Core\Extension\Extension[] $base_themes
   *   An array of extension objects of base theme and its bases. It is ordered
   *   by 'next parent first', meaning the top level of the chain will be first.
   *
   * @return \Drupal\Core\Theme\ActiveTheme
   *   The active theme instance for the passed in $theme.
   */
  public function getActiveTheme(Extension $theme, array $base_themes = []);
It is ordered by 'next parent first', meaning the top level of the chain will be first.

I guess the comment is trying to say that the first parent of the active theme should be on the top level of the array. This would mean that $base_themes is passed in the correct order and that we should reverse it inside of getActiveTheme() method just for some cases like 'libraries-override' is.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Component: token system » asset library system
Issue tags: +Needs tests

Wow, excellent find! Thanks for explaining it so clearly, and for creating a patch that fixes it. For this to be committable, it needs to have a test that reproduces the bug. Do you think you could do that? :)

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs review » Needs work

Setting it to Needs work because of missing tests.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Primsi’s picture

Primsi CreditAttribution: Primsi commented
Assigned: Unassigned » Primsi
Primsi’s picture

Primsi CreditAttribution: Primsi commented
Status: Needs work » Needs review
FileSize
libraries-override-wrong-override-order-3073053-8.testonly.patch2.15 KB
libraries-override-wrong-override-order-3073053-8.patch2.9 KB

Adding a test. I added it to LibraryDiscoveryIntegrationTest, or it would make more sense somewhere else?

Primsi’s picture

Primsi CreditAttribution: Primsi commented
Assigned: Primsi » Unassigned

The last submitted patch, 8: libraries-override-wrong-override-order-3073053-8.testonly.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

pivica’s picture

pivica CreditAttribution: pivica at MD Systems GmbH commented
Component: asset library system » theme system
FileSize
interdiff-3073053-8-12.txt3.15 KB
libraries-override-wrong-override-order-3073053-12.patch3.15 KB

I've found one more problem with reversed call order. For the same themes inherit example, as in issue description, the Drupal\Core\Theme\ThemeManager::alterForTheme() will call functions in the wrong/reversed order.
So, for example, if all themes are defining hook_form_alter() ThemeManager::alterForTheme will call hooks in this order:

[
  'client_theme_form_alter',
  'theme_3_form_alter',
  'theme_2_form_alter',
  'theme_1_form_alter',
]

and the correct order is:

[
  'theme_1_form_alter',
  'theme_2_form_alter',
  'theme_3_form_alter',
  'client_theme_form_alter',
]

The problem is inherited from the same place, in ThemeInitialization::getActiveTheme() we are doing:

$values['base_theme_extensions'] = $base_active_themes;

which has themes in reversed order, and then later we are using this value to initialize ActiveTheme::baseThemeExtensions variable which will be returned by ActiveTheme::getBaseThemeExtensions() which is used in ThemeManager::alterForTheme().

As an experiment, I've tried to reverse $base_themes for all method code in ThemeInitialization::getActiveTheme() but that created other unexpected problems. So we can't do that.

Then I've checked can we just array_reverse for `$values['base_theme_extensions']` but that is also not an option because some places are already using array_reverse() approach, like Drupal\Core\ThemeRegistry::build() and in the same class postProcessExtension(), and webform WebformThemeManager::getActiveThemeNames().

So I guess the only thing we can do currently is to find all places where getBaseThemeExtensions() call is used and make sure that array_reverse is applied to them. Not an ideal solution at all but I can't think a better approach for now.

Here is a patch that is adding array_reverse where needed and also adds explanation to getBaseThemeExtensions() comment about this.

If new patch makes sense should we add additional tests that are covering other cases?

pivica’s picture

pivica CreditAttribution: pivica at MD Systems GmbH commented
FileSize
libraries-override-wrong-override-order-3073053-13.patch5.79 KB

Sorry, the last patch is a wrong diff, this one is correct. Interdiff is fine.

pivica’s picture

pivica CreditAttribution: pivica at MD Systems GmbH commented

> If new patch makes sense should we add additional tests that are covering other cases?

Quickly checking new changes it looks to me that we can't really create tests for other changes because those places are only calling other functions.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
@@ -212,7 +212,8 @@ public function getStyleSheetsRemove() {
    *
    * The order starts with the base theme of $this and ends with the root of
-   * the dependency chain.
+   * the dependency chain. Because of this you will probably want to apply
+   * array_reverse() call to the returned array.
    *

    I guess the wording could be improved a bit?

    "For most use cases, parent themes are expected to be called first, so this order needs to be reversed with array_reverse()."?

    This is not on an interface, so a possible option could also be to add an argument (or a separate method) to let it to the array_reverse() internally, e.g. function getBaseThemeExtensions($root_first = FALSE). But don't make that change just based on my feedback, lets see what others think.

  2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -427,7 +427,7 @@ public function alterForTheme(ActiveTheme $theme, $type, &$data, &$context1 = NU
 
-    $theme_keys = array_keys($theme->getBaseThemeExtensions());
+    $theme_keys = array_reverse(array_keys($theme->getBaseThemeExtensions()));
     $theme_keys[] = $theme->getName();

    We can test this indirectly. Add a form alter to the example themes that we already use in the other test, then similar to block_form_form_test_alter_form_alter(). Then we can extend \Drupal\Tests\system\Functional\Form\AlterTest with the additional messages. I think it's OK to extend that existing test for that.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

pivica’s picture

pivica CreditAttribution: pivica at MD Systems GmbH commented

This issue is maybe partially related (alter call order fixing) to #2118743: Twig debug output does not display all suggestions when an array of theme hooks is passed to #theme.

unstatu made their first commit to this issue’s fork.

unstatu opened merge request !2256

unstatu’s picture

unstatu CreditAttribution: unstatu commented

Hi all,

@pivica thanks for the patch and for reporting the bug. When I discovered the problem I was not sure if it was a bug or it wasn't and I was not very comfortable creating an issue for it since it has a quite a big impact in the core. Then, I found your ticket and saw that it has been well received. That made me happy :)

Well, I'll go straight to the point:

- I have tried to apply the patch on 9.3.12 without success so I have created a MR with a reroll
- Besides to the reroll I have applied the @Berdir suggestion to improve the wording.

Related to the changes in the method. I don't think it does worth to create a new method for changing the order. IMO, it will add some confusion about it. However, I think it is a good idea to add the $root_first parameter with the default value to it. If there is some consensus, I can do it.

Also, I think it's a good idea to extend the \Drupal\Tests\system\Functional\Form\AlterTest for testing the new behavior.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

pooja saraah’s picture

pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commented
FileSize
3073053-23.patch5.94 KB

Fixed failed commands on #20
Attached patch against Drupal 9.5.x

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tonibarbera’s picture

tonibarbera CreditAttribution: tonibarbera as a volunteer commented
Version: 11.x-dev » 10.1.x-dev
Status: Needs work » Needs review
FileSize
3073053-26.patch5.67 KB
23-26.txt1.01 KB

Rerolled patch for Drupal 10.1.5 and the interdiff

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Version: 10.1.x-dev » 11.x-dev

11.x is the current development branch, running #26 against 11.x

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs tests +Needs Review Queue Initiative
FileSize
Screenshot 2023-10-27 at 11.24.48 AM.png33.19 KB

test failure

Ran the tests without the fix and got expected failure. Removing tests tag.

Moving to NW though for the open remaining task

Figure should we fix some other definitions also like 'libraries' because they are also in the wrong order. Maybe this affects the loading of resources in the wrong order?

Has this been figured out?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review

I'm not sure that's required to fix bug that this issue is focused on, although it could be another related bug. I was thinking of a follow-up but I'm not sure if we need that unless the bug exists. Regardless, I think this could use a review.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Reviewed & tested by the community

In that case then patch #26 addresses the problem at hand.

FYI should try to use MRs vs patches as patches are slowly phased out. MRs can be much faster and easier to review.

  • longwave committed 528ac574 on 11.x 
    Issue #3073053 by pivica, Primsi, unstatu, pooja saraah, smustgrave,...
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Fixed

Committed 528ac57 and pushed to 11.x. Thanks!

Decided not to backport to 10.2.x as this seems quite an obscure bug that doesn't affect many users, but I'm concerned that making this change in a patch release might have an unintended side effect in case anyone is currently somehow relying on the existing broken behaviour without realising it.

Status: Fixed » Closed (fixed)

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