Closed (fixed)
Project:
Drupal core
Version:
10.1.x-dev
Component:
Classy theme
Priority:
Major
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
19 Aug 2022 at 09:51 UTC
Updated:
3 Dec 2022 at 02:25 UTC
Jump to comment: Most recent, Most recent file
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.ymltesting_install_profile_all_dependencies.info.ymlthat should work fine switched to starterkit.core/modules/ckeditor/tests/src/FunctionalJavascript/MediaTest.phpreferences 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_subthemewhich definitely does not use Classy but Stark 😅\Drupal\Tests\system\Functional\System\ThemeTest::testUninstallingThemeswhich 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 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 commentedComment #34
quietone commented