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 Umami.
- Create new libraries for every copied library, named with the convention "classy.
".
- Override the Classy libraries so they are not loaded by Umami.
- Any Classy libraries-extend: should now be taken care of in Umami.
The end result is no Classy library assets should load, and Umami experience should be unchanged.

Also: add tests that check to see if any of the copied assets from Classy have changed. If they've changed, there should be a failure that provides instructions on moving the altered file out of the /classy subdirectory.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3106600-11.patch38 KBbnjmnm
#11 interdiff_9-11.txt2.78 KBbnjmnm
#9 3106600-5-copies.patch37.94 KBlauriii
#5 3106600--5.patch62.4 KBbnjmnm
#5 interdiff__3-5.txt2.8 KBbnjmnm
#5 fail_patch_interdiff.txt2.83 KBbnjmnm
#5 3106600-5-changefile-FAIL.patch64.74 KBbnjmnm
#3 umamiCSSmanualCheck.zip4.29 MBbnjmnm
#3 JS-manual__media_embed_ckeditor--second-loads-from-media-module.png160.69 KBbnjmnm
#3 interdiff__2-3.txt3.99 KBbnjmnm
#3 3106600_3.patch57.17 KBbnjmnm
#2 3106600-2.patch56.94 KBbnjmnm
#2 3106600-2-altered-one-css-file-FAIL.patch57.25 KBbnjmnm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue tags: +Needs screenshots
FileSize
3106600-2-altered-one-css-file-FAIL.patch57.25 KB
3106600-2.patch56.94 KB

Postponed on #3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton (this is currently built on patch #40)
This also adds tests that check if any Classy-copied file has been changed. When changed, the test will fail and provide instructions to move the changed file out of the classy subdirectory and update the test to reflect that. A fail test has been added that makes a minor change to action-links.css, which means it no longer matches the Classy file and should be moved to another directory.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue summary: View changes
Issue tags: -Needs screenshots
FileSize
3106600_3.patch57.17 KB
interdiff__2-3.txt3.99 KB
JS-manual__media_embed_ckeditor--second-loads-from-media-module.png160.69 KB
umamiCSSmanualCheck.zip4.29 MB

Patch fixes a typo in the media library extends. It will also need to be rerolled when #3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton lands.
Screenshots attached that verify every asset copied from Classy loads from Umami and not Classy. If other css files sharing the same name are loaded, the filename explains the non-Classy location they come from.

Two notes

  1. Did not include image-widget.css in this as Umami does not actually use this file anywhere
    Info from Classy's image-widget.css: 
    * This CSS file is not used in this theme (Classy). It was intended to be used,
 * but due to a bug, Drupal 8 shipped with it not being used. To not break
 * backwards compatibility, we continue to not load it in Classy. Every
 * subtheme of Classy is encouraged to use it, by attaching the
 * classy/image-widget asset library in their image-widget.html.twig file.
  2. media-embed-error,css is added via the ckeditor_stylesheets: key in classy.info.yml
    This has also been added to Umami in that manner, but I'm not sure how to stop it from loading twice.
    ckeditor_css_alter() is called in umami.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.
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: [PP-1] Decouple Classy libraries from Umami » Decouple Classy libraries from Umami
Status: Active » Needs work

The blocker issue is in.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3106600-5-changefile-FAIL.patch64.74 KB
fail_patch_interdiff.txt2.83 KB
interdiff__3-5.txt2.8 KB
3106600--5.patch62.4 KB

Test was failing due to the wrong namespace, so the screenshots from #3 are still applicable.
This should address the test fail, and I provided another fail patch to show what happens when a change is made to a file copied from Classy.

The last submitted patch, 5: 3106600-5-changefile-FAIL.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue summary: View changes

Cleaning up some formatting in the issue summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Assigned: bnjmnm » Unassigned
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
FileSize
3106600-5-copies.patch37.94 KB

I regenerated the patch with git diff -C -C for easier manual reviewing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Reviewed changes manually using patch in #9. Changes in the patch are related to changing paths, tests or library definitions.

I tested Umami manually to make sure there isn't any obvious regressions. I didn't confirm manually that all libraries have been copied since we have tests for that which I have reviewed earlier as part of #3096203: Create Classy library dependency tests that can be used for all themes, and verify by providing an Umami-specific classy/dropbutton .

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
@@ -0,0 +1,194 @@
+    foreach (['images', 'css', 'js'] as $type) {

Nit: how about $sub_folder instead of $type?

Besides this, I think this is ready for RTBC.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
interdiff_9-11.txt2.78 KB
3106600-11.patch38 KB

Changed $type to $sub_folder per #11

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

This looks ready to go.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented 
  1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
@@ -0,0 +1,194 @@
+            'media-library.css' => 'bb405519d30970c721405452dfb7b38e',
+            'action-links.css' => '6abb88c2b3b6884c1a64fa5ca4853d45',
+            'file.css' => 'b644547e5e8eb6aa23505b307dc69c32',
+            'dropbutton.css' => 'f8e4b0b81ff60206b27f622e85a6a0ee',
+            'book-navigation.css' => 'e8219368d360bd4a10763610ada85a1c',
+            'tableselect.css' => '8e966ac85a0cc60f470717410640c8fe',
+            'ui-dialog.css' => '4a3d036007ba8c8c80f4a21a369c72cc',
+            'user.css' => '0ec6acc22567a7c9c228f04b5a97c711',

    So I definitely did not go through and verify all these hashses. ;)

    What's the plan for this going forward? Is it to confirm that no one makes changes to the files copied from Classy?

    How do we keep people from just updating the hashes instead of understanding that either (a) they should override it and no longer have the Classy version of it in the Classy folders or (b) If they're updating the CSS in the Classy theme under one of the less likely situations where we allow that and knowing that these hashes should be updated as well at that point? 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeNotUsingClassyLibraryTest.php
@@ -413,22 +413,7 @@ public function providerTestThemeNotUsingClassyLibraries() {
         'theme-name' => 'umami',
-        'to-skip' => [
-          'base',
-          'book-navigation',
-          'dialog',
-          'file',
-          'forum',
-          'image-widget',
-          'indented',
-          'media_library',
-          'node',
-          'progress',
-          'search-results',
-          'user',
-          'media_embed_error',
-          'media_embed_ckeditor_theme',
-        ],
+        'to-skip' => [],

@@ -496,15 +481,7 @@ public function providerTestThemeAccountsForClassyExtensions() {
-        'to-skip' => [
-          'user/drupal.user',
-          'core/drupal.dialog',
-          'file/drupal.file',
-          'core/drupal.progress',
-          'media/media_embed_ckeditor_theme',
-          'media_library/view',
-          'media_library/widget',
-        ],
+        'to-skip' => [],

    That's what I was looking for, yay!

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented

Is it to confirm that no one makes changes to the files copied from Classy?

How do we keep people from just updating the hashes instead of understanding that either (a) they should override it and no longer have the Classy version of it in the Classy folders or (b) If they're updating the CSS in the Classy theme under one of the less likely situations where we allow that and knowing that these hashes should be updated as well at that point?

The intent is to communicate this via the fail messages:

          $this->assertEquals($file_hashes[$sub_folder][$filename], $hash, "$filename is in the theme's /classy subdirectory, but the file contents no longer match the original file from Classy. This should be moved to a new directory and libraries should be updated. The file can be removed from the data provider.");

      $this->assertCount($filecount, $file_hashes[$sub_folder], "Different count for $sub_folder files in the /classy subdirectory. If a file was added to /classy, it shouldn't have been. If it was intentionally removed, it should also be removed from this test's data provider.");

And I'm always open to improved phrasing if that would help things.

  • xjm committed d712cec on 9.0.x 
    Issue #3106600 by bnjmnm, lauriii: Decouple Classy libraries from Umami

  • xjm committed a3a0b24 on 8.9.x 
    Issue #3106600 by bnjmnm, lauriii: Decouple Classy libraries from Umami...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

OK, that looks great to me, thanks! Committed and pushed to 9.0.x and cherry-picked to 8.9.x.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Retroactively checked that all the url() uses in the umami/css/classy/ directory have the relative filepaths updated correctly. 25 total calls in the directory, 25 changed in the patch. I also counted that there were the right number of directory-up for each (7 to core/misc/ or core/modules/; 3 to Umami's own comparable images folder).

Status: Fixed » Closed (fixed)

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