Problem/Motivation

Stable will be moved to contrib during Drupal 9. After we have ensured that core themes (Bartik, Claro, Seven, Umami) are not using Stable templates or libraries, we can remove Stable as a base theme of these themes.

Proposed resolution

Set base theme: false for Bartik, Claro, Seven, and Umami
Fix tests that break as a result.
Run regression tests to confirm only expected changes appear. If any regression test results are fuzzy due to differences caused by #2821525: Update normalize.css to the most recent version, it's worth fine-tuning CSS so there are fewer differences due to the updated normalize.css - this will ensure that changes resulting from removing stable as a base theme are identifiable.
Refactor libraries-override that use paths from Stable.

Remaining tasks

All the steps above in "Proposed resolution"
Confirm all @todo's referencing this issue are addressed.

User interface changes

API changes

Data model changes

Release notes snippet

Drupal core themes, Bartik, Claro, Seven and Umami no longer depend on Stable. Prior to this change, these themes were also refactored to no longer depend on Classy. Now, all these themes set base theme: false. All templates and CSS not overridden in these themes will be inherited directly from core.

CommentFileSizeAuthor
#27 3115223-27.patch53.79 KBbnjmnm
#27 interdiff_23-27.txt1.19 KBbnjmnm
#23 3115223-23.patch53.22 KBbnjmnm
#23 interdiff_21-23.txt741 bytesbnjmnm
#21 interdiff_19-21.txt4.16 KBbnjmnm
#21 3115223-21.patch53.19 KBbnjmnm
#20 Screen Shot 2020-03-19 at 15.41.33.png7.67 KBlauriii
#20 Screen Shot 2020-03-19 at 17.06.03.png7.17 KBlauriii
#19 3115223-19.patch49.21 KBbnjmnm
#19 interdiff_17-19.txt755 bytesbnjmnm
#17 interdiff_15-17.txt1 KBbnjmnm
#17 3115223-17.patch50.36 KBbnjmnm
#15 3115223--15.patch49.36 KBbnjmnm
#15 interdiff__13-15.txt14.56 KBbnjmnm
#13 HEAD-vs-patch-differences.zip23.9 MBbnjmnm
#13 3115223-13-reroll.patch33.4 KBbnjmnm
#13 umami-difference.png102.26 KBbnjmnm
#13 chrome-difference.jpg137.41 KBbnjmnm
#13 seven-difference.jpg129.81 KBbnjmnm
#11 3115223-11-reroll.patch31.8 KBbnjmnm
#8 umami-admin_structure_views_diff.png209.83 KBbnjmnm
#8 seven-admin_structure_views_diff.png174.7 KBbnjmnm
#8 bartik-admin_structure_views_diff.png212.22 KBbnjmnm
#8 paths.yml22.33 KBbnjmnm
#5 interdiff_3-5.txt17.91 KBbnjmnm
#5 3115223-5.patch30.54 KBbnjmnm
#3 3115223-3.patch13.48 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 summary: View changes
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
3115223-3.patch13.48 KB
bnjmnm’s picture

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

Corrected postpone list in issue summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
3115223-5.patch30.54 KB
interdiff_3-5.txt17.91 KB

This adds several changes that could not happen until #3111468: Decouple Stable libraries from Bartik/Seven/Claro/Umami was committed.

Something significant to note in the diff that the @todo items pointing to node/3111468 (where those @todos were actually created) were supposed to point to node/3115223. This would likely have been caught were it not part of code that is intentionally temporary. The changes were added to allow core themes to function as if they were already not loading Stable libraries. This was implemented to help surface issues related to the library change before this issue lands and core themes stop declaring Stable as a base theme.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Title: [PP-3] Remove Stable as a base theme of core themes » [PP-1] Remove Stable as a base theme of core themes
Issue summary: View changes
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Title: [PP-1] Remove Stable as a base theme of core themes » [PP-2] Remove Stable as a base theme of core themes
Issue summary: View changes

Added #3117217: Decouple core theme dependency on functions in stable.theme to postponed-on

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
paths.yml22.33 KB
bartik-admin_structure_views_diff.png212.22 KB
seven-admin_structure_views_diff.png174.7 KB
umami-admin_structure_views_diff.png209.83 KB

I performed Wraith regression testing on a patch that combines #5 and the patches from
#3113211: Create test for theme asset decoupling from Stable and address any regressions that decoupling may cause
#3117217: Decouple core theme dependency on functions in stable.theme
These issues are not yet committed but probably won't change too much, particularly in light of the results here. Were those not included it would be very difficult to distinguish issues that stem only from those patches not being added.
I used the URL list @dyannenova provided in #2821525: Update normalize.css to the most recent version and added an edit URL that includes an image widget and filter dropdown. This list is attached.

All four core themes were tested, and the only regression found was expected, occurring in admin/structure/views in Bartik, Seven and Umami. Screenshots of those diffs are attached. The change is due to #3059872: View names on view list table shouldn't be headings, which is a desired change (view names in that table were wrapped in <h3> tags but shouldn't have been).

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +beta target

As with #3113211: Create test for theme asset decoupling from Stable and address any regressions that decoupling may cause, this could theoretically be given a policy exception to be a beta target, although there is moderate risk to that.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Title: [PP-2] Remove Stable as a base theme of core themes » [PP-1] Remove Stable as a base theme of core themes

#3117217: Decouple core theme dependency on functions in stable.theme has been committed 🎉

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
3115223-11-reroll.patch31.8 KB

Reroll on top of #3117217: Decouple core theme dependency on functions in stable.theme

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Title: [PP-1] Remove Stable as a base theme of core themes » Remove Stable as a base theme of core themes
Status: Postponed » Needs review

#3113211: Create test for theme asset decoupling from Stable and address any regressions that decoupling may cause has been resolved 🎉

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
seven-difference.jpg129.81 KB
chrome-difference.jpg137.41 KB
umami-difference.png102.26 KB
3115223-13-reroll.patch33.4 KB
HEAD-vs-patch-differences.zip23.9 MB

This is a reroll on top of all the now-landed issues this was postponed on.
The patch includes

  • Setting core base themes to false
  • Refactored tests that assumed core themes depended on Stable
  • Refactored library overrides using Stable paths
  • Addressed @todo items (including several mistakenly specifying node/3111468)
  • Made a few CSS changes to make diff testing easier - this was discussed with @lauriii and considered in acceptable scope of this issue. It addresses a few more minor differences that would occur between versions of normalize.css, which was updated #2821525: Update normalize.css to the most recent version. All of these were acceptable differences, but had the potential to obfuscate legitimate issues during regression testing.

Regression testing findings

Regression testing was done with the path list that can be found in #8. Tests were performed at these widths: 360, 768 and 1280. A collection of all regression before/after/diffs that were found are zipped and attached.

Bartik

No regressions.

Seven

The only regression found was in admin/structure/views, which is expected and desired. This was a change implemented in #3059872: View names on view list table shouldn't be headings,view names in that table were wrapped in <h3> tags but shouldn't have been. Without Stable, Seven loads the template with this change. (this change did not result in a distinct visual difference in the other themes or the theme already overrode the template)

Claro

On the text format config pages, the list of available CKEditor buttons will wrap to a new row line a max width is hit. The width where buttons begin to wrap is slightly different. For example: with this patch, one button wraps to the next line at a width of 1280, where it previously didn't. I confirmed that media queries still happen where expected, but in this particular case - 26 adjacent buttons result in a few-pixel difference that causes a wrap to happen slightly sooner. As the viewport narrows, more buttons wrap to the next line, so there's no issue of a single button residing on it's own line.

Umami

The vast majority of the differences in all-differences are due to this: The file upload widget looks slightly different.


This difference really seems to irk the regression tests. The file widget is present on every Umami page where regressions were spotted, and the regressions appear more extreme on pages with more file widgets, but all appear to be due to those small height differences. Examine the zip for more details.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work
  1. Should we add a post update hook to uninstall Stable if it doesn't have to be installed after it was removed as the base theme of these themes? This is what we did with Classy on #3115088: Remove Classy as a base theme of core themes.
  2. It seems like there are still files referencing to images in Stable:
    • ./core/themes/seven/css/components/system-status-counter.css
    • ./core/themes/seven/css/components/system-status-report.css
    • ./core/themes/claro/css/theme/views_ui.admin.theme.pcss.css
  3. claro_preprocess_image_widget() has a comment mentioning that its overriding Stable. We should be able to remove the comment and the code adjacent to the comment.
  4. file-link.html.twig has @see reference to stable_preprocess_image_widget() which isn't relevant anymore. However, this file is in the Classy folder in Seven, Bartik and Umami. I think we should still fix the comment because it's inaccurate now.
  5. ./core/themes/claro/templates/content-edit/image-widget.html.twig has @see reference to stable_preprocess_image_widget()
  6. ./core/themes/claro/css/components/tabledrag.pcss.css mentions Stable in a comment. We should update the comment.
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff__13-15.txt14.56 KB
3115223--15.patch49.36 KB

While working on the feedback I noticed some Classy artifacts still present, so I created #3120285: Post Classy decoupling clean up to take care of those, which I plan to address after this lands to avoid reroll hassles.

Addressed all the points in #14. A few notes:

#14.1

Should we add a post update hook to uninstall Stable if it doesn't have to be installed after it was removed as the base theme of these themes?

the Stable update hook tests got their own class even though it seemed like something that could be added via dataProvider to ClassyUninstallUpdateTest. It looks like extending UpdatePathTestBase results in not having dataProvider functionality available. I'm unsure if this is intentional, but it's probably not in-scope here unless there's a known way to get around this within the class.

#14.4

file-link.html.twig has @see reference to stable_preprocess_image_widget() which isn't relevant anymore.

. This required moving these templates out of the /classy subdirectories within their themes as they no longer match Classy. If needed, it is possible to alter ConfirmClassyCopiesTest to account for this use case, but I'm hesitant to do that since 1) The template is technically different, even if the markup is the same 2) The use case would hurt the readability of the test, and this is among the tests where readability is the most important as it's likely to surpise theme contributors with fails when classy-copied templates are changed.

Status: Needs review » Needs work

The last submitted patch, 15: 3115223--15.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3115223-17.patch50.36 KB
interdiff_15-17.txt1 KB

ClassyUninstallUpdateTest needed some slight changes to assertions now that there's a post update hook uninstalling Stable.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/config/tests/src/Functional/ConfigImportUITest.php
@@ -493,7 +493,7 @@ public function testEntityBundleDelete() {
+    \Drupal::service('theme_installer')->install(['test_subtheme']);

    👍 for using theme that will always have a base theme. 

  2. +++ b/core/modules/system/system.post_update.php
@@ -125,3 +126,18 @@ function system_post_update_uninstall_classy() {
+/**
+ * Uninstall Stable if it is no longer needed.
+ */
+function system_post_update_uninstall_stable() {

    Can we document that this needs to run after system_post_update_uninstall_classy()? Luckily this seems to be the case already since the functions are run in alphabetical order based on Drupal\Core\Update\UpdateRegistry::getAvailableUpdateFunctions(). 

  3. +++ b/core/profiles/demo_umami/themes/umami/css/base.css
@@ -275,20 +275,21 @@ fieldset {
-input:not([type="file"]),
...
   line-height: 1.5rem;
...
+input:not([type="file"]) {
   line-height: normal;

+++ b/core/themes/bartik/css/components/form.css
@@ -20,7 +20,7 @@ form {
-input:not([type="file"]) {
+input {

    Why do we need these changes?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
interdiff_17-19.txt755 bytes
3115223-19.patch49.21 KB

This addresses #18.2.

Regarding #18.3

Why do we need these changes?

. These were a few additional changes to address differences caused by #2821525: Update normalize.css to the most recent version. these differences were small and acceptable, but had the potential to obscure other problems identified during regression testing. This was discussed outside-of-issue and mentioned in the list in #13, but it occurred to me this should also be part of the issue summary so I've added that.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue summary: View changes
Status: Needs review » Needs work
FileSize
Screen Shot 2020-03-19 at 17.06.03.png7.17 KB
Screen Shot 2020-03-19 at 15.41.33.png7.67 KB

I did bit of testing on UIs that aren't covered by the regression tests (media library, quickedit, settings tray, layout builder). I found regression on the edit button styles in Settings Tray:

Before:

After:

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3115223-21.patch53.19 KB
interdiff_19-21.txt4.16 KB

I'm glad you checked Settings Tray! I did make a point to manually test Quickedit, Layout Builder and Media Library, but I flat out overlooked that one.

Looks like the regression spotted in #20 is due to a preprocess function in stable.theme that we thought was safe to not move to core themes in [#3112717]. Although that preprocess function was marked deprecated, this is at least one place that still relies on that functionality. I opened the followup #3120962: Existing uses of #links should not rely on array keys to add classes to list items which goes into more detail.

This means that I had to copy the preprocess_links() to each core theme. I did look into fixing it higher up the chain, but it became apparent (and detailed in the followup) that this merited a separate issue.

I wasn't sure how to manually test this fix in Seven or Claro, as the only way I know how to reproduce the issue includes using contextual links to get to the Settings Tray for a menu, and this wasn't available in those themes (and would not be able to investigate further until at least tomorrow).

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

Looks great!. Just had 1 nit and a couple follow up ideas.

  1. +++ b/core/modules/system/system.post_update.php
@@ -125,3 +126,24 @@ function system_post_update_uninstall_classy() {
+ * the case since getAvailableUpdateFunctions() runs post_update hooks in

    Nit: getAvailableUpdateFunctions() just returns the update hooks. It doesn't run them.

    I wondered if we have test coverage for the order. It seems to do in \Drupal\Tests\Core\Update\UpdateRegistryTest() the docblocks don't mention the order. Could be nice novice level follow-up. 

  2. +++ b/core/modules/system/system.post_update.php
@@ -125,3 +126,24 @@ function system_post_update_uninstall_classy() {
+  catch (\InvalidArgumentException | UnknownExtensionException $exception) {
+    // Exception is thrown if Stable wasn't installed or if there are themes
+    // depending on it.
+  }

    We should create an issue to update the docblock on \Drupal\Core\Extension\ThemeInstallerInterface::uninstall()

       * @throws \InvalidArgumentException
   *   Thrown when trying to uninstall the default theme or the admin theme.

    it should mention this is also thrown when theme is depended on by others.

    we do have test coverage for that.

  3. I found it interesting that core/themes/stable/README.txt didn't change at all in this patch. Does stable have different fole now that needs any to be explained at all in this file?

    Also description in core/themes/stable/stable.info.yml is unchanged. that might not need any changes but just checking

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
interdiff_21-23.txt741 bytes
3115223-23.patch53.22 KB

This patch addresses the nit regarding the description of getAvailableUpdateFunctions() in #22.1

I will create the issues regarding coverage for alphabetical sorting of post_update hooks in \Drupal\Tests\Core\Update\UpdateRegistryTest() (#22.1) and the \Drupal\Core\Extension\ThemeInstallerInterface::uninstall() docblock (#22.2), but providing the patch now to keep the issue moving.

The questions in #22.3 about Stable being unchanged are ones that I thought might arise amongst users updating to 9, so I'm glad I have a chance to answer this early in the process. The scope of this issue and the ones that preceded it don't require any changes to Stable. This is the culmination of decoupling Bartik, Claro, Seven and Umami from Stable, but Stable has a purpose as a backwards compatible "fence" for contrib/custom themes, to ensure predictable functionality even if core templates or styling change.
In fact, the Stable theme is SO backwards compatible it's not even changing from Drupal 8 to Drupal 9. It's remaining as-is so contrib/custom themes built for Drupal 8 have the option of using the version of Stable they're accustomed to working with. Drupal 9 also includes the Stable 9 theme, which offers the same "BC fence" as Stable, but the fence is with Drupal's most up-to-date templates/css as of 8.9/9.0

bnjmnm’s picture

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

Added #3121155: Test coverage for the alphabetical sorting in getAvailableUpdateFunctions() and #3121157: Small correction needed in ThemeInstallerInterface::uninstall() docblock. based on the feedback in #22

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

My feedback on #20 has been addressed. I did another code review and a bit more manual testing on interactive UI's that aren't covered by the regression tests and I couldn't find any other regressions.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
@@ -636,7 +634,6 @@ public function providerTestClassyCopies() {
diff --git a/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php b/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php

diff --git a/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php b/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php
index 8f09bcd5a4..b04220b167 100644

index 8f09bcd5a4..b04220b167 100644
--- a/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php

--- a/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php

+++ b/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php
@@ -24,10 +24,6 @@ public function testMaintenanceTheme() {

@@ -24,10 +24,6 @@ public function testMaintenanceTheme() {
 
     $active_theme = \Drupal::theme()->getActiveTheme();
     $this->assertEquals('seven', $active_theme->getName());
-
-    $base_themes = $active_theme->getBaseThemeExtensions();
-    $base_theme_names = array_keys($base_themes);
-    $this->assertSame(['stable'], $base_theme_names);

We should change this test to use a test theme that has a base theme. Because base theme handling in _drupal_maintenance_theme() is pretty special.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff_23-27.txt1.19 KB
3115223-27.patch53.79 KB

Addresses #26

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

#27 addresses #26. Moving back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Committed and pushed b9fce45b5c to 9.1.x and ee4a13ddb0 to 9.0.x. Thanks!

@lauriii can you add a release note?

  • alexpott committed b9fce45 on 9.1.x 
    Issue #3115223 by bnjmnm, lauriii, tedbow, xjm, alexpott: Remove Stable...

  • alexpott committed ee4a13d on 9.0.x 
    Issue #3115223 by bnjmnm, lauriii, tedbow, xjm, alexpott: Remove Stable...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue summary: View changes

Added release note snippet

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +9.0.0 release notes

Published the CR.

Status: Fixed » Closed (fixed)

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