YAML discovery does not take theme inheritance into account

Provided a MR with a failing test to demonstrate the problem.

https://git.drupalcode.org/issue/drupal-3457863/-/jobs/1984798#L4263

---- Drupal\KernelTests\Core\Layout\LayoutPluginManagerTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-361.xml      0 Drupal\KernelTests\Core\Layout\Layo
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.20 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.8
    Configuration: /builds/issue/drupal-3457863/core/phpunit.xml.dist
    
    F                                                                   1 / 1
    (100%)
    
    Time: 00:01.315, Memory: 8.00 MB
    
    There was 1 failure:
    
    1)
    Drupal\KernelTests\Core\Layout\LayoutPluginManagerTest::testPluginOverride
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
    -'Child'
    +'Parent'
    
    /builds/issue/drupal-3457863/core/tests/Drupal/KernelTests/Core/Layout/LayoutPluginManagerTest.php:40

Copy pasting works from @maboy in ✨ UX: Add other widgets than select Active , MR 47.

/**
   * Get list of theme directories sorted by inheritance.
   *
   * @return array
   *   The sorted directories.
   */
  protected function orderTheme(): array {
    // Order theme directories to allow overriding in subtheme.
    $themes = $this->themeHandler->listInfo();
    $ordered_theme_directories = [];
    $base_theme = [];
    $theme_directories = $this->themeHandler->getThemeDirectories();

    // Create a list which includes the current theme and all its base themes.
    foreach ($themes as $key => $theme) {
      // Base theme for current theme.
      // We need to place current theme AFTER the base theme in array.
      if (isset($theme->base_themes)) {
        // Get the last base_theme.
        // To get the key after we want to insert the current theme.
        $after_key = \array_key_last($theme->base_themes);
        $index = \array_search($after_key, \array_keys($ordered_theme_directories), TRUE);

        // The current theme is a base theme.
        $before_key = NULL;
        if (\array_key_exists($key, $base_theme)) {
          $before_key = $base_theme[$key];
        }

        // The base theme is not existing yet in the array.
        if ($index == 0) {
          // Current theme is also a base theme.
          if ($before_key) {
            // Insert the current theme before its children in the array.
            $index_before = \array_search($before_key, \array_keys($ordered_theme_directories), TRUE);
            $ordered_theme_directories = \array_slice($ordered_theme_directories, 0, $index_before - 1) + [$key => $theme_directories[$key]] + $ordered_theme_directories;
          }
          else {
            // Insert the current theme at the end of the array.
            $ordered_theme_directories[$key] = $theme_directories[$key];
          }
          $base_theme = [$after_key => [$key]];
        }
        // The base theme exist in the array.
        else {
          // Insert the current theme after the base_theme.
          $ordered_theme_directories = \array_slice($ordered_theme_directories, 0, $index + 1) + [$key => $theme_directories[$key]] + $ordered_theme_directories;
        }
      }
      // No base theme,
      // Can be added first.
      else {
        $ordered_theme_directories = [$key => $theme_directories[$key]] + $ordered_theme_directories;
      }
    }

    return $ordered_theme_directories;
  }

I will try to use that for a generic fix.

pdureau → credited maboy → .

I have just put code from comment 6 into the getThemeDirectories method of the themeHandler directly.

I have changed the logic to not rely on itself anymore to avoid infinite loop. And so I had to change some variables name. I hope the logic is still ok.

At the beginning I tried a "simplified" approach based on:

    $theme_config = $this->configFactory->get('system.theme');
    $default_theme = $theme_config->get('default');
    $admin_theme = $theme_config->get('admin');

And only handle those themes but the problem is that it can be null, at least in the tests.

Test are green now. Thanks @maboy!

So, testing with UI Suite modules, I started with UI Styles.

No problem on the default theme which is a sub theme of ui_suite_bootstrap generated using its starterkit.

But if I go on a page which should be contextualized on ui_suite_bootstrap, it does not detect the styles overridden by its sub theme.

See attached screenshots. Also I attached the styles of the sub theme.

So maybe the envisaged solution is not ok and all plugins provided by YAML should follow SDC logic to have the provider automatically prefixed in the plugin machine name to avoid overlaps.

Nice work everyone,

Believe to be very close just a couple nitpicky stuff.

Review comments addressed.

Feedback appears to be addressed

Thanks for the RTBC but I don't know if it's not too soon. See comment 12.

At least in RTBC this will more attract attention from maintainers to get feedbacks.

About theme hierarchy (A is the base theme of B which is the base theme of a C which is the active theme):

• it is important to get the plugin override right: a plugin from C is overriding a plugin from A or B if same plugin ID, a plugin from B is overriding a plugin from A if same plugin ID.
• merging and overriding theme settings with the same rules would be nice but it is not as mandatory and not in the scope of this issue, IMHO

Adding the parent theme in the dependencies list of the child theme has no effect.

The pipeline failed on one test: Drupal\Tests\ckeditor5\FunctionalJavascript\ImageUrlProvider, I don't think it is related to the change in this issue.

Existing kernel test updated.

Pipeline still gets some random failure from other unrelated tests.

Talked with @grimreaper and @longwave about this issue at Drupal Barcelona. Existing test coverage looked great, found a way to move the ordering logic up a layer (actually removing it in the end which was unexpected but even better). RTBC for me now.

Given we're touching this code to add a foreach() I think we can make it even more readable?

Suggestion approved, thanks!

Yes that looks much better, foreach is now an overall simplification of the code compared to how it was.

Backported down to 10.3.x as this seems like a low risk bug fix and unblocks some problems in contrib.

Committed and pushed 5e0ff5a872 to 11.x and 62027b5d4d to 11.0.x and 152f951114 to 10.4.x and bd6acd2ffb to 10.3.x. Thanks!

Thanks a lot for the merge and the backports!

longwave → closed merge request !8577

There is a possible regression: 🐛 Fatal error: Uncaught TypeError: Drupal\Core\Extension\ThemeHandler::addTheme() Active

I was in process of upgrading a Drupal 8.x website to 10.3.x and everything was going well until yesterday when I upgraded to 10.3.6. I started getting the WSOD and this error popped up:

Uncaught PHP Exception TypeError: "Drupal\Core\Extension\ThemeHandler::addTheme(): Argument #1 ($theme) must be of type Drupal\Core\Extension\Extension, null given, called in /core/lib/Drupal/Core/Extension/ThemeHandler.php on line 74" at /core/lib/Drupal/Core/Extension/ThemeHandler.php line 84

There's an issue open for that error, and I do think it's a regression from here: 🐛 Fatal error: Uncaught TypeError: Drupal\Core\Extension\ThemeHandler::addTheme() Active .

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