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

Comments

quietone created an issue. See original summary.

quietone’s picture

StatusFileSize
new6.54 KB

The changes in the patch are from the work done by phenaproxima over in #3308890: Deprecate Stable theme.

quietone’s picture

Issue summary: View changes
spokje’s picture

Decide what needs to be done for these.

If 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 legacy and add a @todo: remove in [stable-removal-issue] or try to move to the stable9 directory/namespace.

spokje’s picture

core/modules/rdf/tests/src/Functional/Node/NodeDisplayConfigurableTest.php

Already has @group legacy, since the RDF module is deprecated in 9.5.x, test is removed from 10.0.x upwards. I think we don't have to do anything else for this one.

core/modules/system/tests/src/Functional/Update/StableUninstallUpdateTest.php

Test is removed from 10.0.x upwards. 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 legacy and we're done.

core/modules/tour/tests/src/FunctionalJavascript/TourLegacyTest.php

Already has @group legacy in 9.5.x, test is removed from 10.0.x upwards. I think we don't have to do anything else for this one.

core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php

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.

core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php

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 $allThemes array. For better test coverage I also think we should be adding stable9. So basically: Swap out stable with stable9 and we're good.

core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTest.php

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 legacy in 9.5.x with 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\ConfirmClassyCopiesTest in #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\Stable9LibraryOverrideTest

core/tests/Drupal/KernelTests/Core/Theme/StableTemplateOverrideTest.php

Same story as \Drupal\KernelTests\Core\Theme\ConfirmClassyCopiesTest

core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php

The term "stable" is used here in the context of ExtensionLifecycle::STABLE. Has no links to the theme stable, nothing to see here, move along.

core/tests/Drupal/Tests/Core/Extension/ModuleRequiredByThemesUninstallValidatorTest.php

All mocks in there, but replacing stable with stable9 seems the right thing to do.

spokje’s picture

StatusFileSize
new3.38 KB
new10.98 KB

A patch with the above applied.

spokje’s picture

Status: Active » Needs review
lauriii’s picture

Title: Convert tests to stable9 instead of stable » Remove references to Stable
StatusFileSize
new23.26 KB
new11.38 KB

New patch based on what was needed on #3309176: Remove Stable theme from Drupal 10.

lauriii’s picture

StatusFileSize
new23.8 KB

Self-reviewing the interdiff:

  1. --- b/core/modules/system/tests/src/Functional/Update/StableUninstallUpdateTest.php
    +++ a/core/modules/system/tests/src/Functional/Update/StableUninstallUpdateTest.php
    
    @@ -9,8 +9,6 @@
    - * @group legacy
    - * @todo Remove in https://www.drupal.org/project/drupal/issues/3308998
    

    I don't think this needs to be marked as @legacy since this is a test base. ✅

  2. +++ a/core/modules/system/tests/src/Functional/Update/StableUninstallUpdateTest.php
    @@ -8,7 +8,6 @@
    - * @group legacy
    

    We need separate 9.5.x patch that includes this.

bnjmnm’s picture

Status: Needs review » Needs work

Spotted a few tiny things.

  1. Looks like where the test_theme_depending_on_stable hasn't been used since #3115223: Remove Stable as a base theme of core themes, so it can be removed as part of this
  2. test_wild_west mentions stable in its description, though doesn't directly use it
  3. BaseThemeRequiredTest (using test_wild_west) has an assertion message inaccurately referencing stable
  4. Do the test_stable references in \Drupal\Tests\Core\Theme\RegistryTest::testGetRegistryForModule need to be addressed? It's not immediately clear to me why that naming is used in the first place.
lauriii’s picture

Status: Needs work » Needs review
Issue tags: +Needs follow-up
StatusFileSize
new25.31 KB
new25.85 KB
new2.05 KB

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

lauriii’s picture

Issue tags: -Needs follow-up

Looked at the test again and starterkit_theme is extending stable9, so I think we actually don't need the follow-up.

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

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

lauriii’s picture

StatusFileSize
new25.49 KB
new633 bytes

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 3308987-14-d95.patch, failed testing. View results

lauriii’s picture

Status: Needs work » Needs review
StatusFileSize
new25 KB
new241 bytes
new25.31 KB

Looks like we need to bring back test_theme_depending_on_stable for 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.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Fix makes sense, tests are green.

lauriii’s picture

StatusFileSize
new24.89 KB

  • catch committed 2121966 on 10.0.x
    Issue #3308987 by lauriii, Spokje, quietone, bnjmnm: Remove references...
  • catch committed 6c087ce on 10.1.x
    Issue #3308987 by lauriii, Spokje, quietone, bnjmnm: Remove references...

  • catch committed effa1c6 on 9.5.x
    Issue #3308987 by lauriii, Spokje, quietone, bnjmnm: Remove references...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed/cherry-picked the respective patches to 10.1.x, 10.0.x and 9.5.x, thanks!

Status: Fixed » Closed (fixed)

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