Deprecate base theme fallback to Stable

So, in a nutshell, we need to make this work. I'll take this on.

Let's see what fails 🤓 Hopefully a lot.

3540 failures, excellent! (I guess the sheer number of failures causes DrupalCI to not report this back for some reason?)

First, working to understand the intent and scope of the issue here. This requires me to analyze the problem/motivation section, which currently reads like this:

If a theme hasn't provided base theme setting, the theme will currently fallback to extending stable. This works when there's only one version of stable. However, in Drupal 9, there will be multiple stable versions (Drupal 8 stable and Drupal 9 stable). This will either be so that Stable is moved to contrib, and a new renamed Stable theme takes place in core, or so that there will be multiple versions of stable in core. Both of these scenarios need themes to explicitly define which version of stable it wants to extend.

Let's break this down to help us get on a path to consensus.

I think some of what is said here is impossible because Drupal currently does not support having multiple versions of a theme in a single Drupal installation. In other words: you can't have two foo themes with different versions.

Therefore this option is currently impossible:

However, in Drupal 9, there will be multiple stable versions (Drupal 8 stable and Drupal 9 stable).

Unless that means that "Drupal 9 stable" would be a new stable9 theme.

This will either be so that Stable is moved to contrib, and a new renamed Stable theme takes place in core, […]

This would be theoretically possible … except that a theme declaring base theme: stable now has no way to indicate which of the following is intended:

• Drupal 8 core's stable
• Drupal 9 core's stable
• Drupal contrib's stable

[…]or so that there will be multiple versions of stable in core.

This I think is impossible.

Both of these scenarios need themes to explicitly define which version of stable it wants to extend.

Kind of: if by "which version" you mean "a renamed stable for Drupal 9".

Based on this, my proposal would be:

• Introduce a stable9 theme.
• stable will always be "the Drupal 8 stable theme"'s extension name. We could alias this to stable8 to have consistency.
• For the scope of this issue, it then doesn't matter whether stable aka stable8 remains in core or is moved into contrib; the extension name remains the same.
• This solution will also work when Drupal 10 comes around: we'll just introduce stable10 at that point.

👍 Thanks for confirming!

• Created change record: https://www.drupal.org/node/3066038
• Updated issue summary
• Attached patch should have fewer than #4's 3540 failures 🤓 It triggers a deprecation error, adds test coverage for this, and updates all of core's themes except for test_stable since the entire purpose of that theme is to test the current "default to stable if no base theme specified" behavior.

except for test_stable since the entire purpose of that theme is to test the current "default to stable if no base theme specified" behavior

Hm … this is what's causing most if not all of the remaining 1636 failures.

So I need to make the test_stable theme be discovered only by \Drupal\KernelTests\Core\Theme\StableThemeTest::testStableIsDefault(). This turns out to be surprisingly hard.

The only way to do it is by using file_scan_ignore_directories to make extension discovery ignore test_stable in all tests except for this one. But that leads to the next challenge: \Drupal\Core\Extension\ExtensionDiscovery::$files is cached statically, across all ExtensionDiscovery instances, with no way of clearing this cache 😨I attempted various tricks, all without success.

After spending a lot of time digging, I discovered SettableDiscoveryExtensionListTrait and TestThemeExtensionList, all out of reach in a unit test class unfortunately.

I settled on something that works although I definitely do not think it's elegant. OTOH … this is just going to be around until Drupal 9 ships, so does elegance truly matter? I think this is good enough.

I see that #10's interdiff and patch were incomplete. Here are the missing bits.

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -268,6 +268,8 @@ protected function bootEnvironment() {
+      // Ignore the `test_stable` theme because it triggers deprecation errors.
+      'file_scan_ignore_directories' => ['test_stable'],

This fixed it for kernel tests. Now repeating that for functional tests.

Before I dig in deeper, I'd like confirmation from @lauriii and perhaps a framework manager that they approve of this approach. I'm down two ~150 failures, but the patch still is doing pretty exceptional things. Before I spend more time on it to bring it down to zero failures, I'd like a +1 on the approach.

1. 👍 Will do.
2. I think it's important that when developers start to see these deprecation errors for how it used to work that they can also read about how it will work.
3. IMHO that's not necessary, because in Drupal 9 we'll have to remove all 9.0.0 deprecation errors anyway, so there's no risk of forgetting :)

Overall the approach seems fine to me. 👍

Okay, noted! @xjm also indicated she wanted to review the approach, so now assigning to her.

Related to #17.2: #2659890-32: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility.

Sorry, to clarify: I'm interested in reviewing the new approach and will when I have time, but this does not need to be blocked on me. We could also get feedback from one of the framework managers. I'll see if anyone is interested in giving feedback here.

This is still just a proposal. Also, we haven't decided on naming. Is this something we have to decide in the scope of this issue? Any thoughts on trying to document this without going into specifics?

I think we can just say 'you must now specify 'stable' as a base theme specifically to use it, instead of it being added by default, see @change_record', and then document what Drupal 9/10 recommendations look like in the change record from a follow-up issue - i.e. I don't think we should block the issue on the specific plan, it also works self-contained just to get people to add the explicit dependency

I should've been more clear in #17: I am looking for sign-off on the approach of the test coverage.

1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableThemeTest.php
   @@ -42,11 +43,35 @@ protected function setUp() {
   +    $correct_deprecation_error_was_triggered = FALSE;
   +    $previous_error_handler = set_error_handler(function ($severity, $message, $file, $line, $context) use (&$previous_error_handler, &$correct_deprecation_error_was_triggered) {
   +      // Track whether the expected deprecation error was triggered and then
   +      // continue executing code as if nothing happened, to allow the rest of
   +      // the test to still make its assertions.
   +      if ($severity === E_USER_DEPRECATED && $message === 'There is no `base theme` property specified in the test_stable.info.yml file. The optionality of the `base theme` property is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. All Drupal 8 themes must add `base theme: stable` to their *.info.yml file for them to continue to work as-is in future versions of Drupal. See https://www.drupal.org/node/3066038') {
   +        $correct_deprecation_error_was_triggered = TRUE;
   +        return;
   +      }
   +      if ($previous_error_handler) {
   +        return $previous_error_handler($severity, $message, $file, $line, $context);
   +      }
   +    });
   

   If we're removing this in Drupal 9 why are not doing regular legacy testing with @expectedDeprecation.

2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/StableThemeTest.php
   @@ -58,6 +83,16 @@ public function testStableIsDefault() {
   +    $previous_error_handler = set_error_handler(function ($severity, $message, $file, $line, $context) use (&$previous_error_handler) {
   +      // @see ::testStableIsDefault()
   +      if ($severity === E_USER_DEPRECATED && $message === 'There is no `base theme` property specified in the test_stable.info.yml file. The optionality of the `base theme` property is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. All Drupal 8 themes must add `base theme: stable` to their *.info.yml file for them to continue to work as-is in future versions of Drupal. See https://www.drupal.org/node/3066038') {
   +        return;
   +      }
   +      if ($previous_error_handler) {
   +        return $previous_error_handler($severity, $message, $file, $line, $context);
   +      }
   +    });
   +
   

   I think we should move this test into a new class. The WildWest theme shouldn't trigger this deprecation and having this code here is confusing.

As for a better way of testing. Let's remove test_stable found themes and move it to a place where we can create a virtual filesystem to inject into \Drupal\Core\Extension\ThemeExtensionList as Drupal root. That way we can get rid of the much of the awkwardness.

#23:

1. Because merely installing the theme triggers the deprecation error, which means that the rest of the test's method assertions won't run. I though this was how it worked, and I'd swear I manually debugged this to check this, but apparently I was wrong! TIL! 😊
2. +1 (but won't be necessary anymore with #24)

#24: Thanks, that's the feedback I was looking for! Your proposal removes the awkwardness but it makes this one test super convoluted. I think that's the right trade-off.

Or at least, that's what I thought it would do. Unfortunately, \Drupal\Core\Extension\ExtensionDiscovery::scanDirectory() does this:

    if (!is_dir($absolute_dir)) {
      return $files;
    }

I can work around that by ensuring ExtensionDiscovery is constructed with $root = 'vfs://root'. But then the next problem hits me, \Drupal\Core\Extension\ExtensionDiscovery::scan($type, …) does this, despite being limited to a particular extension $type:

    foreach ($searchdirs as $dir) {
      // Discover all extensions in the directory, unless we did already.
      if (!isset(static::$files[$this->root][$dir][$include_tests])) {
        static::$files[$this->root][$dir][$include_tests] = $this->scanDirectory($dir, $include_tests);
      }
      // Only return extensions of the requested type.
      if (isset(static::$files[$this->root][$dir][$include_tests][$type])) {
        $files += static::$files[$this->root][$dir][$include_tests][$type];
      }
    }

It first scans a given search directory for all search types, then it extracts only the requested type. So it's impossible for me to override only ThemeExtensionList. We'll need to override ExtensionDiscovery just for ThemeExtensionList.

However, after doing that, I ran into a new problem: \Drupal\Core\Extension\ExtensionList::createExtensionInfo() does:

  protected function createExtensionInfo(Extension $extension) {
    $info = $this->infoParser->parse($extension->getPathname());

    // Add the info file modification time, so it becomes available for
    // contributed extensions to use for ordering extension lists.
    $info['mtime'] = $extension->getMTime();

    // Merge extension type-specific defaults.
    $info += $this->defaults;

    return $info;
  }

Since InfoParser is unaware of the root and assumes everything it is given is relative to the Drupal root on disk (and not some virtual file system), it fails to find the file in the virtual file system.

No problem, I can work around this too! Just subclass InfoParser::parse() and prefix all incoming paths with vfs://root/ and then inject this custom info parser into ThemeExtensionList. Yay, parsing now works.

Alas … another problem arises. $extension->getMTime() now fails because \Drupal\Core\Extension\Extension::__call() omits the root. I was able to fix this in Extension itself.

Great, now everything seems to be working. Except theme installation is failing because test_stable lives in the virtual file system, yet it depends on stable which lives inside the real file system! 🤣We can pretend it also lives in the virtual file system. Doing that gets us a working solution.

There was at least one test relying on the test_stable theme. It looks like that was not truly necessary, but for now just simply pointing to the new location.

Now if I'm lucky, the only remaining failing test is \Drupal\Tests\system\Functional\Update\StableBaseThemeUpdateTest. We'll have to see what to do about that test. I think we might be able to remove it altogether. It's testing system_update_8012(), which was added before 8.0.0!

Also: figuring out #26 took a very long time. It reaches into some areas with hard-coded assumptions. I spent several hours of my vacation trying to finish this up, then gave up when it seemed to take too long. Turns out this was the right call, since I spent another several hours on it today!

#32 fixed the failures in ExtensionListTest. That test was passing invalid data to Extension objects to work around a bug that I solved in #26:

$extension->getMTime() now fails because \Drupal\Core\Extension\Extension::__call() omits the root. I was able to fix this in Extension itself.

This reroll makes similar fixes in ThemeExtensionListTest.

\Drupal\KernelTests\Core\Theme\BaseThemeRequiredTest::testWildWest() can be simplified now 🥳

Interesting twist for StableBaseThemeUpdateTest: system_update_8012() was removed in favor of system_update_8014() by @alexpott in #2581443-35: Make Classy extend from the new Stable base theme.

I think we may be able to use a pattern similar to what I used in \Drupal\KernelTests\Core\Theme\BaseThemeDefaultDeprecationTest, but I'm still seeing one unrelated update path test failure locally. Let's see what testbot says.

I don't like this at all, but the only alternative I see is to simply remove this update path test.

Feedback would be much appreciated!

Just noticed #36's interdiff was wrong.

+++ b/core/lib/Drupal/Core/Extension/Extension.php
@@ -155,7 +155,7 @@ public function load() {
-      $this->splFileInfo = new \SplFileInfo($this->pathname);
+      $this->splFileInfo = new \SplFileInfo($this->root . '/' . $this->pathname);

+++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionListTest.php
@@ -142,7 +142,7 @@ public function testGetPathnames() {
-      'test_name' => 'vfs://drupal_root/example/test_name/test_name.info.yml',
+      'test_name' => 'example/test_name/test_name.info.yml',
     ], $filenames);

+++ b/core/tests/Drupal/Tests/Core/Extension/ThemeExtensionListTest.php
@@ -31,29 +31,30 @@ public function testRebuildThemeDataWithThemeParents() {
-        'test_subtheme'  => new Extension($this->root, 'theme', $this->root . '/core/modules/system/tests/themes/test_subtheme/test_subtheme.info.yml', 'test_subtheme.info.yml'),
-        'test_basetheme' => new Extension($this->root, 'theme', $this->root . '/core/modules/system/tests/themes/test_basetheme/test_basetheme.info.yml', 'test_basetheme.info.yml'),
+        'test_subtheme'  => new Extension($this->root, 'theme', 'core/modules/system/tests/themes/test_subtheme/test_subtheme.info.yml', 'test_subtheme.info.yml'),
+        'test_basetheme' => new Extension($this->root, 'theme', 'core/modules/system/tests/themes/test_basetheme/test_basetheme.info.yml', 'test_basetheme.info.yml'),

All these changes make up for about half of the changes, these could be moved to a separate (blocking) issue if we like. Thoughts?

I think it might be saner to just remove StableBaseThemeUpdateTest. And if people disagree, it'd be great if they could provide proposals of how to keep it working while also complying with all other requirements.

Issue summary updated, change record already exists. #38 and #41 are both green. #38 keeps BaseThemeDefaultDeprecationTest but makes it use the virtual file system, #41 removes the test.

This needs review on a few aspects:

1. overall approach introduced in #26 based on @alexpott's proposal in #24
2. #38 vs #41: how do we want to deal with BaseThemeDefaultDeprecationTest?
3. #40: do we want to extract a portion of this patch to a separate issue?

WRT patch details: I know \Drupal\KernelTests\Core\Theme\BaseThemeDefaultDeprecationTest shouldn't use FooSomething classes, but without consensus about the approach, renaming that is just busywork.

@xjm mentioned in a meeting the other that we definitely want to avoid removing test coverage, even if it means making tests more complex. So please ignore #41 and instead review #38 :)

Like @lauriii mentioned, we discussed #44 last night. I clarified #44.2 (which indeed was very poorly worded — sorry about that!): I was referring to the fact that ideally we would keep StableBaseThemeUpdateTest, use vfs:// (per @alexpott in #24), without complex code and without the need for tricky changes in existing tests. (Which is also what #44.3 is about, more about that in my next comment.)

Per #44.1, we're going with the approach in #38 (which updates StableBaseThemeUpdateTest in pretty complex ways to not reduce test coverage), not #41 (which drops that test coverage). So, rerolling #38.

This reroll also addresses point #44.4. Next up: #44.3.

WRT #44.3. The tricky thing is that those changes all are due to bugs in tests and code paths exercised only by tests. So we can't write a failing test, because that'd be a test testing a test 🙃

I could extract that into a separate issue, and I did: #3076797: \Drupal\Core\Extension\Extension's absence of validation has allowed multiple incorrect tests to be added

Since even this Major issue is not getting a lot of attention, I doubt that Normal issue that only touches internals is going to get more attention. So my fear is that if we split it out into a separate issue, that it will not get committed. I'd be happy to be wrong though! Hopefully see you in #3076797: \Drupal\Core\Extension\Extension's absence of validation has allowed multiple incorrect tests to be added, which would reduce the size of this patch.

That being said, I have good news: I found a way to split it off to a separate issue after all! Because, as it turns out, there’s a LOT of broken test code in core around this. 🙃This is good, because it justifies fixing this separately!

Rebased #46 on top of #3076797-15: \Drupal\Core\Extension\Extension's absence of validation has allowed multiple incorrect tests to be added.

#3076797: \Drupal\Core\Extension\Extension's absence of validation has allowed multiple incorrect tests to be added landed! Rebased #49.

Fixed the sole coding standard violation. 😬

But that did not cause the test failures. What did, were the test themes that were added to Drupal core between #49 and #50. Those now need to be updated too. Easy enough.

Done & done. :) That second point is a really good catch 😅 (Not that it would've mattered a lot, but still.)

Committed 5448b07 and pushed to 8.8.x. Thanks!

Updated the RN snippet to describe the disruption.

Per a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing tag cleanup here based on that discussion.

Will Drupal 11 also use stable9, as Drupal 10 does currently? Or will there eventually be stable11 for Drupal 11, stable12 for Drupal 12, etc.?