Problem/Motivation

To make sure we unblock deprecating Classy from Drupal 10 on time, we should update remaining tests to use Starterkit. We can continue working on #3083275: [meta] Update tests that rely on Classy to not rely on it anymore after this has been done.

Issue fork drupal-3304731

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

phenaproxima’s picture

Version: 9.5.x-dev » 10.1.x-dev
Assigned: Unassigned » phenaproxima

Self-assigning to work on this. I'll start with 10.1.x and we'll figure out how to backport it as needed.

phenaproxima’s picture

Okay. The only remaining core test which contains defaultTheme = 'classy' is \Drupal\FunctionalTests\Theme\ClassyTest. That should clearly be moved into the contrib project, so I'll update the meta-issue.

phenaproxima’s picture

Assigned: phenaproxima » xjm
Status: Active » Needs review

Nice.

Assigning to @xjm for review per prior out-of-band discussion.

bnjmnm’s picture

Status: Needs review » Needs work

There are several test themes that have base theme: classy. Those uses need to be refactored as well as they're just as much a barrier to removing Classy.

phenaproxima’s picture

Assigned: xjm » phenaproxima

Re-assigning to myself to make that change and see what breaks. If it turns out to cause a tsunami of complexity, I'd vote we do that in another stable-blocking follow-up.

bnjmnm’s picture

Issue tags: +Drupal 10, +beta blocker
phenaproxima’s picture

Assigned: phenaproxima » bnjmnm
Status: Needs work » Needs review

Those remaining failures are fixed now (at least on my machine). Reassigning to @bnjmnm for review.

phenaproxima’s picture

Assigned: bnjmnm » phenaproxima
Status: Needs review » Needs work

Boo, I forgot to deal with tests that install Classy using the theme installer. Maddening. Reassigning to work on that.

phenaproxima’s picture

Assigned: phenaproxima » bnjmnm
Status: Needs work » Needs review

Okay, over to you now, @bnjmnm. I've addressed every test that mentions Classy in its code, and deleted a few cases/tests which will no longer be relevant when Classy is removed from core.

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

There's an open thread in Gitlab that I'm fine with as the test still checks for the same thing: is-active status. @phenaproxima spent a while looking for an explanation for the difference, but I'm going to RTBC this as I don't believe the need to change the test reflects anything beyond the fact that Classy and Starterkit aren't identical, and the change is very similar to the hundreds of changes made to tests switching from Classy to Stark as their default theme. (I would point out the two tests where that is happening, but Gitlab is currently down)

A committer is certainly welcome to push back and insist on full justification for the change, so I wanted to be sure this RTBC comes with confirmation that @phenaproxima and myself aren't 100% why those changes work, but do not feel they introduce any problems.

bnjmnm’s picture

Alright, Gitlab is back
The two tests with changes that needed to be made to work with Starterkit and seem fine, but there isn't a conclusive explanation as what makes Classy & Starterkit different are:
core/modules/system/tests/src/Functional/Menu/LocalTasksTest.php
core/modules/views/tests/src/Functional/Plugin/DisplayPageWebTest.php

Small changes compare to switching to Stark but wanna make that disclosure full.

lauriii’s picture

Posting the MR as a patch to test across multiple branches.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Posted few nitpicks on the MR and looks like 9.5.x needs some work still.

ravi.shankar made their first commit to this issue’s fork.

phenaproxima’s picture

Status: Needs work » Needs review
bnjmnm’s picture

Status: Needs review » Needs work

    A few more nits + one more MR thread.

  1. There are test dependencies on Classy in testing_install_profile_dependencies.info.yml testing_install_profile_all_dependencies.info.yml that should work fine switched to starterkit.
  2. core/modules/ckeditor/tests/src/FunctionalJavascript/MediaTest.php references a Classy template that I assume it no longer Classy
        if ($media_embed_enabled) {
            // The preview rendering, which in this test will use Classy's
            // media.html.twig template, will fail without the CSRF token/header.
            // @see ::testEmbeddedMediaPreviewWithCsrfToken()
    
phenaproxima’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Hiding out-of-date patch from #14.

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

Alright I've read through this many times lets switch this to RTBC 🍿

  • lauriii committed 4024e3c on 10.1.x
    Issue #3304731 by phenaproxima, bnjmnm: Update remaining tests using...

  • lauriii committed 305554c on 10.0.x
    Issue #3304731 by phenaproxima, bnjmnm: Update remaining tests using...

  • lauriii committed 689225c on 9.5.x
    Issue #3304731 by phenaproxima, bnjmnm: Update remaining tests using...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
5.81 MB

Removing credit from @ravi.shankar given that the commit that received automatic credit only addressed nitpicks and ignored the fact that the MR needed changes for 9.5.x compatibility. Removing credit from myself since I simply reposted the MR as a patch for triggering test bot.

Discovered couple of funky things as I was reviewing this:

  1. There's a testing theme called views_test_classy_subtheme which definitely does not use Classy but Stark 😅
  2. There's \Drupal\Tests\system\Functional\System\ThemeTest::testUninstallingThemes which is ensuring that Classy cannot be uninstalled when Olivero and Claro are installed. However, it's not testing what it should because Classy is not even enabled on the test 🤪
  3. There are few remaining references to Classy in Starterkit Theme 🤦‍♂️
  4. It seems like HAL will continue to use Classy for testing 🤠

Thank you @bnjmnm and @phenaproxima! It seems like this turned out being quite a battle.

Committed 10.1.x MR to 10.1.x. and cherry-picked to 10.0.x. Also committed 9.5.x MR to 9.5.x.

lauriii’s picture

Updating credits since they were swallowed by d.o.

smustgrave’s picture

Can someone move credit over from https://www.drupal.org/project/drupal/issues/3112548 where work was done to change it to stark.

smustgrave’s picture

Status: Fixed » Closed (fixed)

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

quietone’s picture

Issue tags: +Drupal 10 beta blocker