Problem/Motivation

Drupal 9 will come with a new version of Stable. Initially, the plan was to replace the old Stable with a new Stable theme, which essentially would force all themes to upgrade to the next Stable theme as part of the major version upgrade. Upgrading Stable can be cumbersome. Given that upgrading to the next Stable theme comes with very little benefits, it seems unlikely that themes would be interested in upgrading unless they are forced to do so.

Proposed resolution

  1. Make base theme a require property in themes: #3065545: Deprecate base theme fallback to Stable
  2. Copy current Stable to contrib project so that it can be used with Drupal 9. There's already a namespace reserved for this.
  3. Create a new Stable theme with a new name for Drupal 9, following almost the same steps used to create Drupal 8 Stable in #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS
    1. Automated test for library overrides to ensure completeness
    2. Automated test for template overrides to ensure completeness
    3. Copy all the CSS, changing image paths as necessary
    4. Copy all the images, no changes No benefit to this step, they'll get copied when it becomes contrib, though.
    5. Add all the libraries-override needed to stable's .info.yml
    6. Copy all the templates, with appropriate doc changes
  4. Deprecate the Drupal 8 Stable theme in Drupal 9 to be removed in Drupal 10.

Remaining tasks

  • Agree on maintainers of the contrib project
  • Copy code from Stable to the new project
  • Decide what happens to Classy which depends on Drupal 8 Stable
  • Agree on a name for the Drupal 9 Stable: #3094440: [policy, no patch] Agree on name for Drupal 9's Stable theme
  • Create the Drupal 9 Stable theme and copy all the templates and assets to the new theme
  • Write change record
  • Remove dependency on Stable theme from core themes and tests not specifically testing Stable
  • Deprecate Drupal 8 Stable (needs follow-up)

User interface changes

API changes

Data model changes

Release notes snippet

The Stable 9 theme has been added to provide backwards compatible markup and CSS in Drupal 9. More information can be found in the change record introducing Stable 9.

CommentFileSizeAuthor
#83 3050374-83.patch125.97 KBalexpott
#83 79-83-interdiff.txt1006 bytesalexpott
#79 interdiff_77-79.txt586 bytesbnjmnm
#79 3050374-79.patch126.71 KBbnjmnm
#77 interdiff__73-77.txt7.19 KBbnjmnm
#77 3050374--77.patch126.13 KBbnjmnm
#74 3050374-73-copies.patch133.35 KBlauriii
#73 interdiff.txt1.87 KBlauriii
#73 3050374-73.patch496.33 KBlauriii
#71 3050374-71--reroll.patch126.67 KBbnjmnm
#68 3050374-61_assumes_normalize_801_landed.patch126.71 KBcatch
#64 Status-report-stable9.png114.07 KBDyanneNova
#64 Status-report-stable.png113.92 KBDyanneNova
#63 stable9-with-normalizecss801__test-them-together.patch547.87 KBbnjmnm
#61 3050374-61_assumes_normalize_801_landed.patch126.71 KBbnjmnm
#61 3050374-61_explicity_uses_normalize_303.patch490.01 KBbnjmnm
#57 interdiff_54-57.txt9.96 KBbnjmnm
#57 3050374-57.patch126.71 KBbnjmnm
#54 3050374--54.patch125.41 KBbnjmnm
#54 interdiff__50-54.txt21.54 KBbnjmnm
#50 3050374-50.patch122.26 KBbnjmnm
#50 interdiff_45-50.txt163.27 KBbnjmnm
#45 3050374-45-REROLL.patch166.91 KBbnjmnm
#44 3050374--44.patch167.03 KBbnjmnm
#44 interdiff_34-44.txt562 bytesbnjmnm
#38 3050374-37.patch169.98 KBbnjmnm
#38 interdiff_34-37.txt57.42 KBbnjmnm
#34 3050374-34.patch166.48 KBbnjmnm
#34 interdiff_30-34.txt510 bytesbnjmnm
#30 3050374-30.patch166.43 KBbnjmnm
#30 interdiff_27-30.txt163.01 KBbnjmnm
#27 3050374-27.patch170.44 KBbnjmnm
#27 interdiff_19-27.txt2.82 KBbnjmnm
#17 3050374-17.patch557.42 KBbnjmnm
#14 3050374-14.patch167.59 KBbnjmnm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

davidhernandez’s picture

Note that if we change the name it won't be as simple as downloading the contrib theme to stay compatible. If you are doing any overrides somewhere (or who knows what) the name "stable" may be in your code somewhere. The likelyhood is low, but I think still possible. We'll need to document it.

larowlan’s picture

Is there a link to the discussion that prompted this?

lauriii’s picture

@larowlan The parent issue has a summary of the conclusions made in the discussion: #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

bnjmnm’s picture

  • I can be a maintainer on the contrib
  • The name that comes to mind is stable8. Open to other bike storage options, of course.
  • #2 makes a good point and that will be taken into account when the contrib project is created.
lauriii’s picture

Title: Copy Stable to contrib project » Create Drupal 9 stable theme
Issue summary: View changes
Related issues: +#2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS

Updated the issue summary to reflect discussions on #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility.

I can be a maintainer on the contrib

👏 Would you be interested in being a maintainer of the Drupal 8 core Stable subsystem as well?

The name that comes to mind is stable8. Open to other bike storage options, of course.

After some discussions with the release and product managers, I think It would be better to keep the name of the existing theme the same to make upgrades easier.

xjm’s picture

One thing we discussed is that Drupal 8 stable can't be deprecated in core until we deprecate Classy. #3050378: [meta] Replace Classy with a starterkit theme is the proposed implementation for deprecating Classy and we'll want to have that work happening during the next two years in the Drupal 9 minor release cycle.

This also means we don't actually need to put code in the contrib project until later. We just need a plan for the upgrade path (for themes, for Composer, etc.).

bnjmnm’s picture

Would you be interested in being a maintainer of the Drupal 8 core Stable subsystem as well?

Yes, I can maintain that as well.

lauriii’s picture

Issue summary: View changes
lauriii’s picture

Issue summary: View changes

Reordering some steps so that they are in the order we most likely want to work on them.

bnjmnm’s picture

bnjmnm’s picture

Issue summary: View changes

Updated issue summary detailing the proposed process of creating the Stable theme for Drupal 9. This currently matches the process used for Drupal 8 Stable, which occurred in #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS. If there is interest in making changes to that process for Drupal 9, this is a good place to express that interest.

bnjmnm’s picture

This patch starts the process of creating the stable9 theme, which will replace stable. The theme is mostly unchanged at this point. It's been renamed to stable9, and other files such as tests and themes that subtheme it have been changed to reflect those changes.

The remaining test failures are all update tests to tests paths within 8.x. This appears to be due to fixtures & tests assuming the test begins and ends with a theme named "stable" being present. In general, I'm not sure of the role 8.x update tests have in 9.x and I'm working on getting that clarified. It's possible this is the first 9.x branch where 8.x update tests don't work, but it certainly won't be the last. Once I know more I'll update documentation where needed and get this patch passing all tests. Then it will be time to move templates and CSS..

bnjmnm’s picture

Version: 8.9.x-dev » 9.0.x-dev
lauriii’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
    @@ -112,7 +112,7 @@ public function getActiveThemeByName($theme_name) {
    -        if ($ancestor == 'stable') {
    +        if ($ancestor == 'stable9') {
               // Themes that depend on Stable will be fixed by system_update_8014().
               // There is no harm in not adding it as an ancestor since at worst
               // some people might experience slight visual regressions on
    

    Would we be able to remove this completely?

  2. +++ b/core/themes/classy/classy.info.yml
    @@ -1,6 +1,6 @@
    -base theme: stable
    +base theme: stable9
    

    Classy should continue to extend stable because changing this to stable9 would be a BC break.

  3. +++ b/core/themes/stable9/stable9.info.yml
    --- a/core/themes/stable/stable.theme
    +++ b/core/themes/stable9/stable9.theme
    

    We should remove everything from this file.

  4. We shouldn't remove stable yet. Drupal 9 will ship with both, stable and stable9.
bnjmnm’s picture

Version: 9.0.x-dev » 8.9.x-dev
Status: Needs work » Needs review
FileSize
557.42 KB

#14 was pretty much a misfire so an interdiff is going to be more noise than signal. The questions there can be ignored and this patch provides an attempt at the full theme.

lauriii’s picture

@bnjmnm could you document the steps taken to generate #17 in the issue summary? 😎

Given that we are copying lots of files around, I'd recommend that we use git diff -C -C for generating future patches to make reviewing easier. 😇

bnjmnm’s picture

Issue summary: View changes
FileSize
181.12 KB

I updated steps to create Stable 9 in item #3 of "Proposed Resolution" to be easier to identify by changing them from an <ol> to a <ul>, plus added some details about the process.

The patch is #17 , but using git diff -C -C so no interdiff needed. Note that when the stable and module files are identical, the diff tends to favor saying that the file was copied from Stable.

bnjmnm’s picture

Issue summary: View changes

I created/populated a 9.x-1.x branch (no release yet, though) at https://www.drupal.org/project/stable, but upon re-review of the issue summary it sounds like this should be a 10.x branch as Stable(8) will be deprecated in 9 and not removed until 10 - some clarification on this would be helpful for me. Once I'm more certain of the release compatibility, I can update the README and create a release.
With this contrib Stable, I manually tested how this would work on a Drupal 9 install without stable in core/themes, particularly since Stable is hidden and can't be explicitly installed. It worked well - themes that depended on Stable or on Stable-depending themes such as Classy were able to access the contrib Stable based solely on its presence in the filesystem - no enabling required. I also manually tested the (unlikely?) scenario of contrib and core versions of Stable both present, and in this instance core opts to the contrib version.

lauriii’s picture

I would create 8.x-1.x to make it clear that the snapshot is coming from Drupal 8. I'm not sure how versioning contrib projects will work in the future but we would essentially create a new major version for the Drupal 9 Stable. That would probably be 9.x-2.x or just 2.x depending on how versioning is done in the future.

bnjmnm’s picture

I've added an 8.x dev release to https://www.drupal.org/project/stable, but I'm not able to get it to install to 9.x via Composer. Tried a few combinations of core_version_requirement: and core: with no success. The resulting error is very similar to what happens with any contrib theme/module I attempt to install on 9.x, so it may be that Composer contrib requires aren't possible yet on this version? Will look into that further, but documenting here in case it's information someone has at the ready.

Here's the Composer error (which admittedly I have a hard time mentally parsing even after many reads of their documentation)

Problem 1
    - Conclusion: remove drupal/core 9.0.x-dev
    - Installation request for drupal/stable-stable 1.x-dev -> satisfiable by drupal/stable-stable[1.x-dev].
    - Conclusion: don't install drupal/core 9.0.x-dev
    - Conclusion: don't install drupal/core 9.0.x-dev
    - drupal/stable-stable 1.x-dev requires drupal/core ~8.0 -> satisfiable by drupal/core[8.0.x-dev, 8.1.x-dev, 8.2.x-dev, 8.3.x-dev, 8.4.x-dev, 8.5.x-dev, 8.7.x-dev, 8.8.x-dev, 8.9.x-dev].
    - Can only install one of: drupal/core[8.0.x-dev, 9.0.x-dev].
    - Can only install one of: drupal/core[8.1.x-dev, 9.0.x-dev].
    - Can only install one of: drupal/core[8.2.x-dev, 9.0.x-dev].
    - Can only install one of: drupal/core[8.3.x-dev, 9.0.x-dev].
    - Can only install one of: drupal/core[8.4.x-dev, 9.0.x-dev].
    - Can only install one of: drupal/core[8.5.x-dev, 9.0.x-dev].
    - Can only install one of: drupal/core[8.7.x-dev, 9.0.x-dev].
    - Can only install one of: drupal/core[8.8.x-dev, 9.0.x-dev].
    - Can only install one of: drupal/core[8.9.x-dev, 9.0.x-dev].
    - Installation request for drupal/core 9.0.x-dev -> satisfiable by drupal/core[9.0.x-dev].
bnjmnm’s picture

@tedbow and @mixologic were able to identify why the Drupal 9 Stable could not be installed via composer. Before that is possible, it requires this issue to be completed: #3084063: Use information in info.yml files to determine project requirements, which is to integrate the core_version_requirement key.

lauriii’s picture

We only need the contrib project once we deprecate Stable. Stable can be deprecated when Classy has been deprecated. Deprecating Classy can only happen after we have a replacement for it, which means we will have to wait for some Drupal 9 minor release which will ship a replacement. It's great that we've been able to make some progress on the contrib project already, but I just wanted to call out that it shouldn't be a hard blocker for this issue. Maybe we should open a separate issue as a follow-up to discuss that?

lauriii’s picture

Version: 8.9.x-dev » 9.0.x-dev
lauriii’s picture

Status: Needs review » Needs work
Related issues: +#3084816: Determine how to deprecate themes
  1. +++ b/core/themes/stable9/README.txt
    @@ -1,21 +1,21 @@
    +Stable is the default base theme for Drupal 9; it provides minimal markup and
    ...
    +In Drupal 9, Stable 9 will be used as the base theme if no base theme is set in
    +a theme's .info.yml file. To opt out of Stable you can set the base theme to
    +false in your theme's .info.yml file (see the warning below before doing this):
       base theme: false
    
    +++ b/core/themes/stable9/stable9.info.yml
    @@ -0,0 +1,300 @@
    +description: A default base theme using Drupal 9.0.0's core markup and CSS.
    

    We should update these because this is not the default base theme in Drupal 9 anymore. We probably should apply these updates to the Drupal 8 Stable theme too.

  2. Was the patch created using markup from Drupal 8.9.0? We should use markup from Drupal 9 since there's few small changes to the markup in that branch and the patch doesn't apply as a result. I don't think we need a 8.9.x version of the patch since this should be only committed to the Drupal 9.0.x branch.
bnjmnm’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
170.44 KB

Addresses both items from #26

lauriii’s picture

Status: Needs review » Needs work
Related issues: +#3101787: Follow-up to #2849628: Copy change to Views UI module
  1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/Stable9LibraryOverrideTest.php
    @@ -44,7 +44,9 @@ class StableLibraryOverrideTest extends KernelTestBase {
    +  protected $librariesToSkip = [
    +    'simpletest/drupal.simpletest',
    +  ];
    
    +++ b/core/tests/Drupal/KernelTests/Core/Theme/Stable9TemplateOverrideTest.php
    @@ -23,6 +23,7 @@ class StableTemplateOverrideTest extends KernelTestBase {
       protected $templatesToSkip = [
    +    'simpletest-result-summary',
         'views-form-views-form',
       ];
    

    Should we document why we're skipping these?

  2. +++ b/core/themes/stable9/README.txt
    @@ -1,21 +1,13 @@
    +Warning: Themes that opt out of using Stable 9 as a base theme will need
    

    Nit: I guess it isn't opting out anymore since Stable isn't the default anymore. Maybe Themes that decide not to use Stable 9... would work?

  3. I'm wondering if we actually should be copying the images over to Stable. The problem with that is the fact that after they're moved to Stable, other extensions could become dependant on the location of the Stable theme. This would make it harder for us to move the theme to contrib in the future.
  4. Opened #3101787: Follow-up to #2849628: Copy change to Views UI module which is a blocker to this issue.
  5. +++ b/core/themes/stable9/stable9.info.yml
    @@ -302,9 +298,3 @@ libraries-override:
    -libraries-extend:
    -  core/drupal.ajax:
    -    - stable/drupal.ajax
    -  user/drupal.user:
    -    - stable/drupal.user
    

    🗑️🎉

bnjmnm’s picture

I'm wondering if we actually should be copying the images over to Stable. The problem with that is the fact that after they're moved to Stable, other extensions could become dependent on the location of the Stable theme. This would make it harder for us to move the theme to contrib in the future.

I've been on the fence about this as well, although for slightly different reasons. I revisited #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS hoping to be reminded of what the justification was for copying the images but I didn't see anything there. Now I'm unsure where/if I saw such a thing.

My main "against" regarding the copying of image assets to Stable9 is that it's considerable overhead for files that are unlikely to have their contents changed. Based on Git history of images in core/modules and misc/icons there is no history of this. If an image needs to be changed, it's probably better if a new image with a different name is added anyway. The one exception that comes to mind is if a security issue is discovered in an SVG, in which case it would be changed in Stable anyways.

My main "for" for copying image assets is for when it becomes a contrib module, to avoid issues like the one in #3045216: Asset paths pointing to core/misc folder assume Claro is installed in core/themes . The location of a contrib theme in the filesystem can't be certain, so the only relative paths guaranteed to work are ones within the theme.

Based on my for/against assessment, my preference would be not copying image assets to the core Stable9 as there's little evidence it is necessary. Once moved to a contrib theme, the image assets should be copied so there aren't issues regarding the location of the theme in the filesystem. Perhaps that could be accompanied by a followup to look into other ways of addressing the issue of relative paths and uncertain locations of contrib themes.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
163.01 KB
166.43 KB

This item addresses the points in #28 including not copying image assets. It sounds like @lauriii and I don't yet entirely line up regarding what do to when the theme is moved to contrib, but we're in agreement that it's best to not have these assets copied into Stable 9. That's nicely in scope here.

lauriii’s picture

Good points @bnjmnm! I totally forgot that contrib was unable to use those images even though I worked on some related issues in Claro. 🤦Moving images to the theme would make sense from the perspective of making it easier to move the theme to contrib, but besides that, it seems to have no value. Maybe we should keep the icons in core for now, and copy them to Stable when it gets moved to contrib?

bnjmnm’s picture

Issue summary: View changes

#31

Maybe we should keep the icons in core for now, and copy them to Stable when it gets moved to contrib?

Yep, that's exactly what I was hoping for too. Updating the issue summary to reflect that.

lauriii’s picture

Issue tags: +Needs reroll

Let's reroll this now that #3101787: Follow-up to #2849628: Copy change to Views UI module has been committed.

bnjmnm’s picture

xjm’s picture

Issue tags: +Needs followup

Tagging for a followup to discuss the policy of copying assets to Stable themes.

bnjmnm’s picture

lauriii’s picture

Status: Needs review » Needs work

Let's update #34 to copy the assets to the Stable 9 theme until we have decision on #3105209: [policy, no patch] as of Stable 9, do not copy image assets unless they have changed.. Copying assets to the Stable theme shouldn't affect module or theme developers at all meaning that this change can be reverted later.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
57.42 KB
169.98 KB

Good point on #37, the image assets change would not be disruptive so it doesn't need to postpone this issue. This has all the changes up to #34 but with the images brought back + the paths to them in CSS.

An additional change was I remove stable9.theme as there's no need for it at the moment.

lauriii’s picture

I guess a follow-up question related to the icons is whether we should copy all icons to Stable regardless of if they are being used? I tried to scan through the icons and it seems like we are potentially missing some icons if the scope is to copy all the icons, and we also have icons in the folder that are not being used anywhere.

bnjmnm’s picture

I guess a follow-up question related to the icons is whether we should copy all icons to Stable regardless of if they are being used?

If images need to be copied I'd say yes to copying all icons largely because there's built-in uncertainty to copying only those in use and no easy way (that I'm aware of) to check with automated tests. Bringing them all over seems to be the most review-friendly and unwanted-surprise-preventing way to go about it. I think partial copying of icons may lead to the need for increased review time that could be better spent creating a test that would address the only concern preventing agreement on not copying images at all #3105209: [policy, no patch] as of Stable 9, do not copy image assets unless they have changed.

it seems like we are potentially missing some icons if the scope is to copy all the icons

The intent was to copy all the icons, and diff -rq core/misc/icons/ core/themes/stable9/images/core/icons/ reports no differences - perhaps there's an additional location I missed?

lauriii’s picture

Issue summary: View changes
Issue tags: +Needs followup, +Needs change record

Added change record as a step in the remaining task. Once #3105209: [policy, no patch] as of Stable 9, do not copy image assets unless they have changed. is done, we might have to make some additions to the CR based on the result.

We should also open a follow-up for deprecating Drupal 8 Stable since we can't do that until we've removed dependency to Stable from code depending on that (Classy and all the themes extending Classy).

DamienMcKenna’s picture

Issue tags: +Drupal 9.0-beta1 requirement

Tagging as a requirement for Drupal 9.0-beta1.

DamienMcKenna’s picture

Issue tags: -Drupal 9.0-beta1 requirement +Drupal 9.0.0-beta1 requirement
bnjmnm’s picture

Now that #3105209: [policy, no patch] as of Stable 9, do not copy image assets unless they have changed. is complete, this brings back #34, which does not copy image assets, but also adds an images directory with a README explaining what it's there for and the circumstances that would lead to it being populated. The interdiff comes from #34 for that reason.

bnjmnm’s picture

#44 didn't apply due to needing a reroll.

Also created an alpha release at drupal.org/project/stable/ to verify it was possible to install on D9 via composer. This is possible, but requires the command composer require 'drupal/stable-stable:^1.0' vs the expected composer require 'drupal/stable:^1.0'. @traviscarden is familiar with what causes this hyphenizing and will do a better job explaining it than me, so he'll be commenting with some details on why that is happening.

TravisCarden’s picture

The hyphenation is a feature of the Drupal.org Composer Facade for handling namespace conflicts caused by making submodules and subthemes available directly via Composer (e.g., composer required coder_sniffer, a submodule of Coder). I would expect there to already be something available as drupal/stable. Having tested that hypothesis, I see that composer require drupal/stable seems to know about it, but I can't get find a version that's installable or find any reference to it in the endpoint responses, so I can't determine where this conflict came from. But it seems to me a safe assumption that some kind of namespace conflict is at the root of this.

Of course, since @bnjmnm got the composer require string from the drupal.org UI, and it does seem to work, this detail is probably not important to explain decisively.

xjm’s picture

The followup for deprecating Stable 8 will need to be postponed on the work to deprecate Classy with the starterkit initiative.

There's a bullet I think is missing from the remaining tasks. Should we work to make core's themes extend Stable 9 instead of Stable 8? It could be a lot of ugly, tricky work, but OTOH it also might not be because:

  1. Decoupling them from Classy has probably already taken care of a lot of things
  2. Internal themes typically are patched to get the same changes as module-provided CSS/templates, etc. during the bugfix process so they might be closer to Stable 9 than Stable 8.
  3. We've established a strong process for analyzing and decoupling theme dependencies.

Furthermore, if we don't make them depend on Stable 9 instead (or on nothing, is the other option, but I've strong concerns about that), then they would also have to be deprecated before Stable 8 is removed, and we don't want that because at least two of them are modern themes under active development.

So, we need a plan for how Bartik, Seven, Umami, and Claro will relate to Stable 8 or Stable 9. (The answer might be different for Bartik and Seven since they are are on a path to be replaced with Claro and Olivero sometime in the future.)

This plan doesn't have to block commit for this issue, but it does need to have an issue filed.

lauriii’s picture

Issue summary: View changes

Based on previous discussions, the plan is to make core themes extend Stark to ensure that core themes are kept up-to-date in future. This is also documented in #2659890-35: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility. Yesterday, @bnjmnm opened an issue for this. We also just added it to the roadmap: #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility.

catch’s picture

Priority: Major » Critical

Bumping to critical since this is blocking the beta.

bnjmnm’s picture

This is a reroll plus:
- simplifies some url() paths to images
- corrects template comments that shouldn't state "default theme implementation"
- removes libraries-override that replace files that no longer exist in core
- added some missing assets from misc/

The new patch is smaller than #45 as some of these changes resulted in less overall changes to report when using git diff -C -C as there were more 100% matches with existing files.

For manual testing, some things to note
- Stable 9 must be enabled before changing an enabled theme's base theme to stable9. The easiest way to do this is set a base theme to stable9 on a disabled theme, then enable it. This enabling will enable stable9 as well, and then it is possible to change any base theme to stable9 without error.
- It shouldn't actually effect testing, but could be confusing if this issue lands while that happens: Core themes are very close to having base theme set to false. This will happen once this issue is complete #3115223: Remove Stable as a base theme of core themes

Status: Needs review » Needs work

The last submitted patch, 50: 3050374-50.patch, failed testing. View results

bnjmnm’s picture

Status: Needs work » Needs review

The test failure that kicked this back to "Needs work" was an unrelated intermittent failure of QuickEditIntegrationTest. Ran the test again to be certain and it's fine.

lauriii’s picture

Status: Needs review » Needs work
  1. +++ b/core/themes/stable9/css/color/color.admin.css
    @@ -39,12 +39,12 @@
    +  background: url(../../../../../core/modules/color/images/hook.png) no-repeat 100% 0; /* LTR */
    ...
    +  background: url(../../../../../core/modules/color/images/hook-rtl.png) no-repeat 0 0;
    
    @@ -75,7 +75,7 @@ button.color-palette__lock,
    +  background: url(../../../../../core/modules/color/images/lock.png) no-repeat 50% 0;
    
    +++ b/core/themes/stable9/css/core/dialog/off-canvas.base.css
    @@ -214,18 +214,18 @@
    +  background-image: url(../../../../../../core/misc/icons/73b355/check.svg);
    ...
    +  background-image: url(../../../../../../core/misc/icons/e29700/warning.svg);
    ...
    +  background-image: url(../../../../../../core/misc/icons/e32700/error.svg);
    
    +++ b/core/themes/stable9/css/core/dialog/off-canvas.dropbutton.css
    @@ -241,7 +241,7 @@
    +  background: transparent url(../../../../../../core/misc/icons/ffffff/pencil.svg) no-repeat center;
    
    +++ b/core/themes/stable9/css/core/dialog/off-canvas.tabledrag.css
    @@ -43,14 +43,14 @@
    +  background-image: url(../../../../../../core/misc/icons/bebebe/move.svg);
    ...
    +  background-image: url(../../../../../../core/misc/icons/787878/move.svg);
    
    +++ b/core/themes/stable9/css/core/dialog/off-canvas.theme.css
    @@ -40,14 +40,14 @@
    +  background-image: url(../../../../../../core/misc/icons/bebebe/ex.svg);
    ...
    +  background-image: url(../../../../../../core/misc/icons/ffffff/ex.svg);
    
    @@ -80,7 +80,7 @@
    +  background: transparent url(../../../../../../core/misc/icons/ffffff/pencil.svg) no-repeat scroll center center;
    
    +++ b/core/themes/stable9/css/quickedit/quickedit.icons.theme.css
    @@ -58,17 +58,17 @@
    +  background-image: url(../../../../../core/modules/quickedit/images/icon-throbber.gif);
    
    +++ b/core/themes/stable9/css/shortcut/shortcut.icons.theme.css
    @@ -22,10 +22,10 @@
    +  background: transparent url(../../../../../core/modules/shortcut/images/favstar.svg) no-repeat left top;
    ...
    +  background-image: url(../../../../../core/modules/shortcut/images/favstar-rtl.svg);
    
    +++ b/core/themes/stable9/css/views_ui/views_ui.admin.theme.css
    @@ -24,7 +24,7 @@
    +  background-image: url(../../../../../core/modules/views_ui/images/sprites.png);
    

    Should we use the shorter path here too?

  2. +++ b/core/themes/stable9/templates/admin/system-modules-uninstall.html.twig
    --- a/core/modules/views_ui/templates/views-ui-style-plugin-table.html.twig
    +++ b/core/themes/stable9/templates/admin/views-ui-style-plugin-table.html.twig
    
    +++ b/core/themes/stable9/templates/views/views-view-opml.html.twig
    @@ -9,8 +9,6 @@
    diff --git a/core/themes/classy/templates/views/views-view-row-opml.html.twig b/core/themes/stable9/templates/views/views-view-row-opml.html.twig
    
    copy to core/themes/stable9/templates/views/views-view-row-opml.html.twig
    diff --git a/core/themes/stable/templates/views/views-view-row-rss.html.twig b/core/themes/stable9/templates/views/views-view-row-rss.html.twig
    

    Let's update these files with the theme override documentation.

  3. Should we override the CSS files from layout_discover module since we have overridden the templates from there too?
  4. Has ./core/misc/normalize-fixes.css been excluded on purpose?
  5. Has ./core/misc/print.css been excluded on purpose?
  6. --- a/core/tests/Drupal/KernelTests/Core/Theme/StableLibraryOverrideTest.php
    +++ b/core/tests/Drupal/KernelTests/Core/Theme/Stable9LibraryOverrideTest.php
    

    Any thoughts on expanding the test coverage to include tests that ensure that referenced files are pointing to existing files and that we are not overriding non-existing assets?

bnjmnm’s picture

#53.1-2 addressed those neglected files
#53.3

Should we override the CSS files from layout_discover module since we have overridden the templates from there too?

It looks like this is an issue with both layout_builder and layout_discovery. I reviewed the make-stable issues for both and neither suggest that the omission of CSS was intentional.
Layout discovery stable #2834025: Mark Layout Discovery as a stable module
Layout builder stable #3041053: Mark Layout Builder as a stable module
I suspect this was overlooked because the CSS is not in the /css directory. I added libraries for both module's layouts.

#4

Has ./core/misc/normalize-fixes.css been excluded on purpose?

Yes, as extends the normalize.css library, which isn't currently included in Stable. My thought was to match the approach from #2821525: Update normalize.css to the most recent version and only copy normalize.css (and normalize-fixes.css) to stable if the library is updated.

#5 Has ./core/misc/print.css been excluded on purpose? It was, but this made me reconsider. The file is not included in any library, but it is added directly in book-export-html.html.twig

    <link type="text/css" rel="stylesheet" href="misc/print.css" />

Stable's version of that template just links to misc/print.css, which I initially thought was intentional but now I'm thinking it was overlooked. I updated Stable 9's template to link to the Stable 9 CSS.

#6

Any thoughts on expanding the test coverage to include tests that ensure that referenced files are pointing to existing files and that we are not overriding non-existing assets?

Great idea and easy to implement! Had to update the test anyway to include the new layout library overrides. This will prevent the stale overrides we ran into as part of decoupling!

lauriii’s picture

#54.4 Since this issue is still open #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides, I'm wondering if we should ship normalize.css as well in Stable, at least for the Drupal 9 version. This way if we want to update normalize.css in a minor release of Drupal 9, we don't have worry about any potential disruption.

xjm’s picture

Assigned: Unassigned » lauriii

@lauriii is going to work on the change record for this issue.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
126.71 KB
9.96 KB

#55 is a good idea. I implemented that here. That the version specified in stable/normalize is intentionally set to "8.0.1" since that will be the version in Drupal 9. The patch will work with either version of normalize since it uses git diff -C -C, but it's probably important to state. This should not get committed until #2821525: Update normalize.css to the most recent version is committed. That issue does not prevent work from happening here, however, so I'm not changing the status to postponed.

lauriii’s picture

Title: Create Drupal 9 stable theme » [PP-1] Create Drupal 9 stable theme
Assigned: lauriii » Unassigned
Issue tags: -Needs change record

I'm finished editing the change record.

lauriii’s picture

If it seems like it will take long for us to land #2821525: Update normalize.css to the most recent version, we could consider adding normalize.css 3.0.3 to Stable 9 and update it to 8.0.1 as part of #2821525: Update normalize.css to the most recent version.

xjm’s picture

Assigned: Unassigned » lauriii

Assinginf to @lauriii for review.

bnjmnm’s picture

As a precaution in case #2821525: Update normalize.css to the most recent version does not land, I was asked to provide a version of the patch that provides normalize.css 3.0.3. This approach results in a larger patch, because it's not possible to use git diff -C -C as the diff needs to explicitly provide normalize.css 3.0.3, as opposed to copying whatever file is in that path. The patch that does this is named 3050374-61_explicity_uses_normalize_303.patch.

If #2821525: Update normalize.css to the most recent version lands, use the 3050374-61_assumes_normalize_801_landed.patch instead. This is just the patch from #57 but named in a way to make it evident how it differs from the other patch. Even if it's necessary to go with the normalize 3.0.3 patch, reviewers should do their reviewing on this patch as it's much easier to navigate, and the only difference between the two is the normalize.css version being loaded.

xjm’s picture

It's not as a precaution against the issue not landing; it's a combined patch so we can test both together while working on this issue and until such time as it does land. :)

bnjmnm’s picture

Ah, #62 makes sense. This patch combines the changes for this issue with the ones in #2821525: Update normalize.css to the most recent version

DyanneNova’s picture

Status: Needs review » Needs work
FileSize
113.92 KB
114.07 KB

I've tested the patch in #63 using Bartik and Seven. I was able to enable Stable 9 as the base theme for both successfully. I found a few bugs on the Status Report page when Stable 9 is enabled as the base theme for Seven, and have attached screenshots.

I'm going to set up a Wraith test to run overnight to see if anything else pops up.

Status Report using Stable
Stable using patch #63

Status Report using Stable 9
Stable 9 using patch #63

It looks like the counter issues are coming in here:

.system-status-report-counters__item {
width: 100%;
margin-bottom: 0.5em;
padding: 0.5em 0;
text-align: center;
white-space: nowrap;
background-color: rgba(0, 0, 0, 0.063);
}

The h3 borders are coming in here:

.system-status-general-info__item-title {
border-bottom: 1px solid #ccc;
}

And the extra box border and margins are coming in here:

.system-status-general-info__item {
margin-top: 1em;
padding: 0 1em 1em;
border: 1px solid #ccc;
}

catch’s picture

On #64 it's my understanding that the current patch doesn't make Seven depend on Stable 9, and rather than update it to be based on Stable 9, the direction is to remove the dependency on Stable altogether in issues like #3117217: Decouple core theme dependency on functions in stable.theme. Either way that seems like good information to know for a possible future patch, but not blocking here.

catch’s picture

Status: Needs work » Needs review

Double checked with @lauriii and this testing is indeed out of scope for this issue. We should make sure there are follow-ups to update the various core themes and it is useful advance warning for those.

lauriii’s picture

Status: Needs review » Needs work

It seems like there are some changes to Umami, Bartik, Seven and Claro that seem out of scope.

catch’s picture

Status: Needs work » Needs review
FileSize
126.71 KB

The most recent patch here is the combined patch with normalize.css. Re-uploading the second patch from #61 which is the one that actually needs review. (no credit please)

bnjmnm’s picture

Title: [PP-1] Create Drupal 9 stable theme » Create Drupal 9 stable theme

No longer postponed: #2821525: Update normalize.css to the most recent version landed. The patch in #68 is the current patch for review.

Status: Needs review » Needs work

The last submitted patch, 68: 3050374-61_assumes_normalize_801_landed.patch, failed testing. View results

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
126.67 KB

Reroll

alexpott’s picture

  1. +++ b/core/tests/Drupal/KernelTests/Core/Theme/Stable9LibraryOverrideTest.php
    @@ -44,7 +44,10 @@ class StableLibraryOverrideTest extends KernelTestBase {
    +  protected $librariesToSkip = [
    +    // Simpletest is removed in Drupal 9.
    +    'simpletest/drupal.simpletest',
    +  ];
    

    As far as I can see this is not necessary. Can be an empty array.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Theme/Stable9TemplateOverrideTest.php
    @@ -23,6 +23,10 @@ class StableTemplateOverrideTest extends KernelTestBase {
    +    // Simpletest is removed in Drupal 9.
    +    'simpletest-result-summary',
    

    This does not appear to be necessary either.

lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
496.33 KB
1.87 KB

Good catch @alexpott! These are indeed not needed since #3110862: Remove simpletest module from core has been committed. I fixed these and one indentation mistake I noticed in the patch.

I reviewed patch with following steps:

  • Ensured that all copied files are the latest version from modules. Reviewed by removing all themes from core and generating a new diff with git diff -C -C which will create comparison to core module files.
  • Ensured that all templates have been copied and no unnecessary templates have been copied
    • gfind ./core/modules -name *.html.twig ! -path "*/tests/*" ! -path "./core/modules/help_topics/*" ! -path "./core/modules/migrate_drupal_multilingual/*" ! -path "./core/modules/workspaces/*" ! -path "./core/modules/config_environment/*" -printf "%f\n" | sort > module-templates.txt
    • gfind ./core/themes/stable9 -name *.html.twig -printf "%f\n" | sort > stable-templates.txt
    • diff module-templates.txt stable-templates.txt -y | less
  • Ensured that all CSS files have been copied and no unnecessary CSS files have been copied
    • gfind ./core/modules ./core/misc ./core/assets/vendor/normalize-css -name *.css ! -path "*/tests/*" ! -path "./core/modules/help_topics/*" ! -path "./core/modules/migrate_drupal_multilingual/*" ! -path "./core/modules/workspaces/*" ! -path "./core/modules/config_environment/*" -printf "%f\n" | sort > module-css.txt
    • gfind ./core/themes/stable9 -name *.css -printf "%f\n" | sort > stable-css.txt
    • diff module-css.txt stable-css.txt -y | less
  • Enabled Stable 9 and manually tested that there aren’t regressions in the following UIs:
    • Table drags
    • Contextual links
    • Color editor
    • Off-canvas / Settings tray
    • Database log
    • Status messages
    • Update status
    • Status report
    • Toolbar
    • Views UI
    • Modal dialog
    • Book export
  • Reviewed Stable 9 tests and manually tested that the tests are working properly
    • Removed template and ensured that Drupal\KernelTests\Core\Theme\Stable9TemplateOverrideTest fails
    • Removed CSS file and ensured that Drupal\KernelTests\Core\Theme\Stable9LibraryOverrideTest fails
    • Removed CSS file override from stable9.info.yml and ensured that Drupal\KernelTests\Core\Theme\Stable9LibraryOverrideTest fails
lauriii’s picture

Here's #73 but generated with git diff -C -C./

The last submitted patch, 73: 3050374-73.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 74: 3050374-73-copies.patch, failed testing. View results

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
126.13 KB
7.19 KB

This removes the files that snuck into #73 while keeping the intended changes

lauriii’s picture

Status: Needs review » Needs work
$ yarn run lint:css
yarn run v1.21.1
$ stylelint "**/*.css"

themes/stable9/css/media/plugins/drupalmedia/ckeditor.drupalmedia.css
 12:1  ✖  Unexpected unknown type selector "drupal-media"   selector-type-no-unknown
 28:1  ✖  Unexpected unknown type selector "drupal-media"   selector-type-no-unknown
 35:1  ✖  Unexpected unknown type selector "drupal-media"   selector-type-no-unknown
 36:1  ✖  Unexpected unknown type selector "drupal-media"   selector-type-no-unknown
 39:1  ✖  Unexpected unknown type selector "drupal-media"   selector-type-no-unknown

themes/stable9/css/core/assets/vendor/normalize-css/normalize.css
  42:3   ✖  Expected "margin" to come before "font-size"                order/properties-order
  56:3   ✖  Expected "overflow" to come before "height"                 order/properties-order
  65:27  ✖  Unexpected duplicate name monospace                         font-family-no-duplicate-names
  87:3   ✖  Expected "text-decoration" to come before "border-bottom"   order/properties-order
 108:27  ✖  Unexpected duplicate name monospace                         font-family-no-duplicate-names
 129:3   ✖  Expected "position" to come before "line-height"            order/properties-order
 168:3   ✖  Expected "margin" to come before "line-height"              order/properties-order
 211:3   ✖  Expected "padding" to come before "border-style"            order/properties-order
 243:3   ✖  Expected "display" to come before "color"                   order/properties-order

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

It seems like we should add these files to the .stylelintrc.json as ignored files. 😇

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
126.71 KB
586 bytes

LINT!

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

#77 and #79 addresses the test failures and linting errors. I think this is ready to go back to RTBC 🚣‍♀️

lauriii’s picture

Issue summary: View changes

@alexpott mentioned on Slack that it would be a good idea to have release notes on this so I went ahead and added that.

bnjmnm’s picture

Issue summary: View changes
alexpott’s picture

This is all looking good. I've got some very minor nits that the patch attached fixes.

I've checked all the urls are pointing to real files and the correct ones. And read through the entire patch :)

  1. +++ b/core/.stylelintrc.json
    @@ -442,6 +442,8 @@
    +    "themes/stable9/css/media/plugins/drupalmedia/ckeditor.drupalmedia.css"
    

    Let's add /* stylelint-disable selector-type-no-unknown */ to the file instead of excluding it. More future proof. Or in fact let's do that in a follow-up because then we can do the core file too.

  2. +++ b/core/themes/stable9/templates/content/node.html.twig
    copy from core/themes/stable/templates/field/field.html.twig
    copy to core/themes/stable9/templates/field/field.html.twig
    
    --- a/core/themes/stable/templates/field/file-audio.html.twig
    +++ b/core/themes/stable9/templates/field/file-audio.html.twig
    
    +++ b/core/themes/stable9/templates/field/file-audio.html.twig
    +++ b/core/themes/stable9/templates/field/file-audio.html.twig
    @@ -11,6 +11,7 @@
    
    @@ -11,6 +11,7 @@
     *   - file: The full file object.
     *   - source_attributes: An array of HTML attributes for to be added to the
     *     source tag.
    +*
     */
     #}
    

    The extra line added here feels unnecessary. There are plenty of files that don't have this space. Even some added here.

  3. +++ b/core/themes/stable9/templates/media-library/status-messages.html.twig
    index 64bd791637..2d6a592644 100644
    --- a/core/modules/views/templates/views-view-fields.html.twig
    
    --- a/core/modules/views/templates/views-view-fields.html.twig
    +++ b/core/themes/stable9/templates/views/views-view-fields.html.twig
    
    +++ b/core/themes/stable9/templates/views/views-view-fields.html.twig
    +++ b/core/themes/stable9/templates/views/views-view-fields.html.twig
    @@ -27,8 +27,6 @@
    
    @@ -27,8 +27,6 @@
      * - row: The raw result from the query, with all data it fetched.
      *
      * @see template_preprocess_views_view_fields()
    - *
    - * @ingroup themeable
    

    In Stable the first line of this theme has been changed to Theme override to display all the fields in a row.

lauriii’s picture

The interdiff in #83 looks good so +1 for keeping this as RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +9.0.0 release notes

Committed 2505346 and pushed to 9.0.x. Thanks!

  • alexpott committed 2505346 on 9.0.x
    Issue #3050374 by bnjmnm, lauriii, alexpott, DyanneNova, xjm: Create...

Status: Fixed » Closed (fixed)

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

quietone’s picture

Issue tags: -Needs followup

A follow up was asked for in #41, for deprecating Drupal 8 Stable. That work has since happened, in #3308985: [Meta] Tasks to deprecate Stable. Therefore, I am removing the tag.