Problem/Motivation
A step in the process to deprecate Stable theme.
Steps to reproduce
Proposed resolution
Find usages of stable theme in tests and change to stable9.
Remaining tasks
Decide what needs to be done for these.
$ git grep "'stable'" | grep Test | awk -F: '{print $1}' | sort -u | nl
1 core/modules/rdf/tests/src/Functional/Node/NodeDisplayConfigurableTest.php
2 core/modules/system/tests/src/Functional/Update/StableUninstallUpdateTest.php
3 core/modules/tour/tests/src/FunctionalJavascript/TourLegacyTest.php
4 core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php
5 core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
6 core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTest.php
7 core/tests/Drupal/KernelTests/Core/Theme/StableTemplateOverrideTest.php
8 core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
9 core/tests/Drupal/Tests/Core/Extension/ModuleRequiredByThemesUninstallValidatorTest.php
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | 3308987-17-d10.patch | 24.89 KB | lauriii |
| #16 | 3308987-11-d10.patch | 25.31 KB | lauriii |
| #16 | interdiff.txt | 241 bytes | lauriii |
| #16 | 3308987-16-d95.patch | 25 KB | lauriii |
| #14 | interdiff.txt | 633 bytes | lauriii |
Comments
Comment #2
quietone commentedThe changes in the patch are from the work done by phenaproxima over in #3308890: Deprecate Stable theme.
Comment #3
quietone commentedComment #4
spokjeIf we move along the same line as #3307454: Move classy related tests to the theme directory/namespace or handle them otherwise (which makes sense to me, since this also seems to be a Just-In-Time deprecation), we take the "pragmatic" approach, meaning that we move as much tests as possible (per usual) and relevant/eligible to be in the contrib incarnation of Stable to the stable directory/namespace.
All tests that aren't eligible to be in the contrib incarnation we mark as
@group legacyand add a@todo: remove in [stable-removal-issue]or try to move to the stable9 directory/namespace.Comment #5
spokjeAlready has
@group legacy, since the RDF module is deprecated in9.5.x, test is removed from10.0.xupwards. I think we don't have to do anything else for this one.Test is removed from
10.0.xupwards. Since it's not actually installing themes, there will be no deprecation warnings. Proof of this is that the seven theme is also used in this test, which is deprecated and still no test failures.If we follow the lines of #3307454: Move classy related tests to the theme directory/namespace or handle them otherwise we should add
@group legacyand we're done.Already has
@group legacyin9.5.x, test is removed from10.0.xupwards. I think we don't have to do anything else for this one.The term "stable" is used here in the context of composer dependencies stability. Has no links to the theme stable, nothing to see here, move along.
Looking at this commit for #3302800: Core tests need to filter out deprecated themes when looping over all themes, we "just" remove stable from the
$allThemesarray. For better test coverage I also think we should be adding stable9. So basically: Swap out stable with stable9 and we're good.Test is making sure that Stable overrides all relevant core library assets. Since contrib (which is what stable will be) can't rule core, this one should be
@group legacyin9.5.xwith a@todo Remove in https://www.drupal.org/project/drupal/issues/3308998, since it's also not eligible to move to the contrib theme. We did the same for the Classy specific test\Drupal\KernelTests\Core\Theme\ConfirmClassyCopiesTestin #3307454: Move classy related tests to the theme directory/namespace or handle them otherwise.I think we should do the same for its base-class
\Drupal\KernelTests\Core\Theme\StableLibraryOverrideTestBase.Stable9 will still have coverage with its
\Drupal\KernelTests\Core\Theme\Stable9LibraryOverrideTestSame story as
\Drupal\KernelTests\Core\Theme\ConfirmClassyCopiesTestThe term "stable" is used here in the context of
ExtensionLifecycle::STABLE. Has no links to the theme stable, nothing to see here, move along.All mocks in there, but replacing stable with stable9 seems the right thing to do.
Comment #6
spokjeA patch with the above applied.
Comment #7
spokjeComment #8
lauriiiNew patch based on what was needed on #3309176: Remove Stable theme from Drupal 10.
Comment #9
lauriiiSelf-reviewing the interdiff:
I don't think this needs to be marked as @legacy since this is a test base. ✅
We need separate 9.5.x patch that includes this.
Comment #10
bnjmnmSpotted a few tiny things.
test_theme_depending_on_stablehasn't been used since #3115223: Remove Stable as a base theme of core themes, so it can be removed as part of thistest_wild_westmentions stable in its description, though doesn't directly use itBaseThemeRequiredTest (using test_wild_west) has an assertion message inaccurately referencing stable\Drupal\Tests\Core\Theme\RegistryTest::testGetRegistryForModuleneed to be addressed? It's not immediately clear to me why that naming is used in the first place.Comment #11
lauriiiThank you for the review 🙏
#10.4: Looked at some diffs and I think it was added to test a bug long time ago where Stable was accidentally overriding Classy templates (I remember when myself from the past worked on this issue 😅). I believe this test is no longer testing this so we should probably revisit this test in a follow-up.
Comment #12
lauriiiLooked at the test again and
starterkit_themeis extendingstable9, so I think we actually don't need the follow-up.Comment #13
bnjmnm#11 addresses everything I spotted. I'll be AFK by the time the tests finish so I'm going to RTBC assuming it remains green.
Comment #14
lauriiiApparently a change that was required on Drupal 10 caused all Drupal 9.5 functional tests to fail 🤦♂️ Keeping RTBC since the change is kind of unrelated and silly.
Comment #16
lauriiiLooks like we need to bring back
test_theme_depending_on_stablefor 9.5.x because it's used by\Drupal\Tests\system\Functional\Update\StableUninstallUpdateTest. I kept it using Stable since the test has been marked as legacy and it explicitly tests Stable 8.Comment #17
nod_Fix makes sense, tests are green.
Comment #18
lauriiiReroll now that #3309247: Unused $theme_engine variable in ThemeInitialization.php has landed.
Comment #21
catchCommitted/pushed/cherry-picked the respective patches to 10.1.x, 10.0.x and 9.5.x, thanks!