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
As stated in #3050389: [META] Remove dependency to Classy from core themes, Classy will be moved to contrib before Drupal 9 and we have to remove dependencies on Classy from all core themes.
Part of this process includes creating theme-specific copies of all Classy libraries to core themes. This ensures Classy can be removed without impacting those themes.
Proposed resolution
- As needed, copy libraries and assets from Classy to Seven.
- Create new libraries for every copied library, named with the convention "classy.
".
- Override the Classy libraries so they are not loaded by Seven.
- Any Classy libraries-extend: should now be taken care of in Seven.
The end result is no Classy library assets should load, and Seven experience should be unchanged.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | image-widget.png | 127.27 KB | bnjmnm |
| #3 | 3107872-3-REROLL.patch | 32.21 KB | bnjmnm |
| #3 | 3107872-changed-css-FAIL.patch | 32.53 KB | bnjmnm |
| #2 | seven_media_embed_ckeditor_js__the-second-comes-from-modules.png | 130.71 KB | bnjmnm |
| #2 | seven_css.zip | 3.97 MB | bnjmnm |
Comments
Comment #2
bnjmnmPostponed on #3106600: Decouple Classy libraries from Umami as that will set the standard for the other theme library decoupling issues and should be rerolled on top of that.
This is largely completed other than that, patch attached along with a failing patch that proves tests will catch Classy-copied files that are altered.
Also attached are screenshots confirming every copied asset loads from Seven, not Classy.
Comment #3
bnjmnmUn-postponed and rerolled + a failing patch to show that altered files fail a test with a prompt to move them from the /classy subdirectory.
The screenshots in #2 are applicable to the reroll with this kept in mind (something we've already encountered in #3106600: Decouple Classy libraries from Umami)
Did not include image-widget.css in this as Seven does not actually use this file anywhereInfo from Classy's image-widget.css:
Image-widget is consumed by Seven, will provide another comment confirming the asset loads properly
This has also been added to Seven in that manner, but I'm not sure how to stop it from loading twice.
ckeditor_css_alter() is called in seven.theme before
the ckeditor stylesheets are added in a different instance of that hook... even when themeManager->alter() is called second. Quite possible I was doing something wrong, but it's also worth noting that the worst-case on this is one CSS file loading twice, and that will stop when Classy is removed.
Comment #5
bnjmnmHere's a screenshot of the missing image-widget proof that it is loading from Seven, not Classy.
Comment #6
lauriiiThere's probably isn't anything easy we could do about #3.2 given that it's not really using the libraries system. We could open a follow-up for that but it doesn't seem too high priority since in this use case it doesn't really have any serious consequences.
Besides that, the patch looks great. I manually confirmed again that all references to files are correct and did a bit of manual testing for any obvious regressions.
Comment #7
xjmCould we get the followup mentioned?
Comment #8
bnjmnmAgree with @lauriii in #6 that this isn't a pressing need, but the followup regarding
ckeditor_stylesheets:now exists #3108638: Make it possible to override a base theme's ckeditor_stylesheets settings.Comment #9
xjmI was really confused for a moment before I realized that of course Seven and Umami will end up copying many of the same Classy files to their
classy/subdirectories, and git doesn't know which actual file you copied. (It's just guessing based on similarity.)One thing I didn't think of doing for the Umami patch was double-checking that all relative image paths are correctly updated in the CSS files in the
classy/subdirectory, because that's something our tests don't cover I think. I can probably check this locally with the patch applied by grepping within the directory. I'll check that now for both this patch and Umami.This is the bit related to the aforementioned followup.
Comment #10
xjmApplied the patch locally and counted the
../. It's now five relative directories up fromseven/css/classy/componentstocore/misc/orcore/modules, and (as in #3106600: Decouple Classy libraries from Umami), three to the theme's own images folder. There are 27 totalurl()calls in the copied CSS. Only 12 of them are shown in the patch, but that's because the rest of them are hidden by git mistakenly(ish) thinking the files are copied from the Umami classy folder, where it's also three directories up to that theme's own images folder.Comment #11
xjmHere's the real diffstat of the patch, which I've checked against the list in
ConfirmClassyCopiesTest:Manually inspected and confirmed that all the correct CSS, JS, and images are there. A lot of these things sure seem to be related to the Media Library.
These two previously skipped libraries don't actually have any new library definitions being declared for them in
seven.info.yml. I asked @bnjmnm about this and he indicated that in HEAD currently Seven is already completely overriding it. I wasn't able to figure out if or when this changed between now and when they were added to Classy (and Seven at the same time) in the Media stable-maker patch. Fortunately, it doesn't actually hurt anything for them to have been skipped unnecessarily before; the important thing is that the array is empty now and the test passes.Comment #14
xjmCommitted to 9.0.x and cherry-picked to 8.9.x. Thanks!