Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#28 | battle.gif | 5.81 MB | lauriii |
Issue fork drupal-3304731
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:
- 3304731-9.5.x changes, plain diff MR !2686
- 3304731-10.1.x changes, plain diff MR !2648
Comments
Comment #2
phenaproximaSelf-assigning to work on this. I'll start with 10.1.x and we'll figure out how to backport it as needed.
Comment #4
phenaproximaOkay. 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.Comment #5
phenaproximaNice.
Assigning to @xjm for review per prior out-of-band discussion.
Comment #6
bnjmnmThere 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.Comment #7
phenaproximaRe-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.
Comment #8
bnjmnmComment #9
phenaproximaThose remaining failures are fixed now (at least on my machine). Reassigning to @bnjmnm for review.
Comment #10
phenaproximaBoo, I forgot to deal with tests that install Classy using the theme installer. Maddening. Reassigning to work on that.
Comment #11
phenaproximaOkay, 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.
Comment #12
bnjmnmThere'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.
Comment #13
bnjmnmAlright, 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.
Comment #14
lauriiiPosting the MR as a patch to test across multiple branches.
Comment #15
lauriiiPosted few nitpicks on the MR and looks like 9.5.x needs some work still.
Comment #18
phenaproximaComment #19
bnjmnmA few more nits + one more MR thread.
testing_install_profile_dependencies.info.yml
testing_install_profile_all_dependencies.info.yml
that should work fine switched to starterkit.core/modules/ckeditor/tests/src/FunctionalJavascript/MediaTest.php
references a Classy template that I assume it no longer ClassyComment #20
phenaproximaComment #21
phenaproximaHiding out-of-date patch from #14.
Comment #22
bnjmnmAlright I've read through this many times lets switch this to RTBC 🍿
Comment #28
lauriiiRemoving 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:
views_test_classy_subtheme
which definitely does not use Classy but Stark 😅\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 🤪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.
Comment #29
lauriiiUpdating credits since they were swallowed by d.o.
Comment #30
smustgrave CreditAttribution: smustgrave at Mobomo commentedCan someone move credit over from https://www.drupal.org/project/drupal/issues/3112548 where work was done to change it to stark.
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #34
quietone CreditAttribution: quietone at PreviousNext commented