Copy templates, CSS, and related assets to Stable and override core libraries' CSS

I think we can soon use this issue to do all the template copying and such.

Yeah, we got pretty far on this so I'm hesitant to actually postpone but we certainly wouldn't want to commit anything until RC1. We should be able to check to make sure all our assets are up to date by using git log.

Thanks @webchick just came to do that :)

Updated the IS. Making Classy inherit from Stable is happening separately at #2581443: Make Classy extend from the new Stable base theme so this issue should only be concerned about Stable itself if at all possible.

Hi Kevin, thanks for the keen eye. We are aware and the plan is to hide these core base themes from the UI: #2574975: Allow (base-)themes to be excluded from the UI (e.g. blocks, Appearance).

Working on this today.

Here's a new initial patch but this is based on commit 158d434 and many assets have been updated since then. The next patch will bring things at least more in sync and I'm going to try to come up with an automated way of verifying we have a complete set of assets.

Here's the missing libraries-override bits from stable.info.yml.

At one point in #2575421: Add a Stable base theme to core and make it the default if a base theme is not specified I rebased/rerolled on top of commit 0e4c0f3 so that is the reference used here. Not much difference in terms of assets from 158d434, just 2 less JS file changes, it was a reroll for #2576037: "Sorry" in user-facing errors violates the User Interface Standards.

I'm attaching additional three "interdiffstat" files if anyone wants to compare the changes I'm making here with what actually happened in core. The only differences should be for test files and the one Twig template difference noted below.

Twig templates updated based on:

git diff --stat 0e4c0f3 core/modules/*.html.twig
 core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig                        |  2 +-
 core/modules/config_translation/templates/config_translation_manage_form_element.html.twig |  4 ++--
 core/modules/statistics/tests/themes/statistics_test_attached/node.html.twig               | 24 ++++++++++++++++++++++++
 core/modules/system/templates/admin-page.html.twig                                         |  3 ++-
 core/modules/system/templates/field-multiple-value-form.html.twig                          |  8 +++++---
 core/modules/system/templates/indentation.html.twig                                        |  6 ------
 core/modules/system/templates/item-list.html.twig                                          |  5 +++--
 core/modules/system/templates/mark.html.twig                                               |  4 ++--
 core/modules/system/templates/page.html.twig                                               |  2 +-
 core/modules/system/templates/pager.html.twig                                              | 12 ++++++------
 core/modules/system/templates/system-modules-details.html.twig                             | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 core/modules/system/templates/system-modules-uninstall.html.twig                           | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 core/modules/update/templates/update-version.html.twig                                     |  6 +++---
 core/modules/views/templates/views-view-field.html.twig                                    |  6 +-----
 14 files changed, 201 insertions(+), 32 deletions(-)

Interdiffstat note: The whitespace operator had already been updated in views-view-field.html.twig, that explains the slight discrepancy there.

CSS updated based on:

git diff --stat 0e4c0f3 core/{misc,modules}/*.css
 core/modules/editor/css/editor.css                           | 16 ----------------
 core/modules/simpletest/css/simpletest.module.css            |  4 ++++
 core/modules/system/css/components/ajax-progress.module.css  |  6 +++++-
 core/modules/system/css/components/item-list.module.css      | 19 +++++++++++++++++++
 core/modules/system/css/components/tabledrag.module.css      |  5 ++++-
 core/modules/system/css/system.admin.css                     | 17 ++++++++++++-----
 core/modules/system/css/system.diff.css                      |  6 +++++-
 core/modules/system/css/system.maintenance.css               |  6 +++++-
 core/modules/system/tests/themes/test_theme/css/collapse.css |  4 ++++
 core/modules/views_ui/css/views_ui.admin.theme.css           |  8 ++++----
 10 files changed, 62 insertions(+), 29 deletions(-)

JS updated based on:

git diff --stat 0e4c0f3 core/{misc,modules}/*.js
 core/misc/drupal.js                                           | 17 -----------------
 core/misc/vertical-tabs.js                                    |  4 +++-
 core/modules/ckeditor/js/ckeditor.js                          |  7 ++++---
 core/modules/ckeditor/js/plugins/drupalimage/plugin.js        | 39 ++++++++++++++++++++++++++++++++-------
 core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js | 28 ++++++++++++++++++++++++----
 core/modules/ckeditor/js/plugins/drupallink/plugin.js         | 30 ++++++++++++++++++++++++++----
 core/modules/editor/js/editor.formattedTextEditor.js          |  2 +-
 core/modules/quickedit/js/editors/plainTextEditor.js          |  2 +-
 core/modules/quickedit/js/quickedit.js                        |  7 ++++---
 core/modules/system/tests/themes/test_theme/js/collapse.js    |  4 ++++
 core/modules/views_ui/js/views-admin.js                       | 76 +++++++++++++++++++++++++++++++++++-----------------------------------------
 11 files changed, 134 insertions(+), 82 deletions(-)

Libraries and library overrides updated based on:

git diff --stat 0e4c0f3 core/core.libraries.yml core/modules/*.libraries.yml
 core/core.libraries.yml                                                                                |  5 +++--
 core/modules/ckeditor/ckeditor.libraries.yml                                                           |  2 ++
 core/modules/system/system.libraries.yml                                                               |  1 +
 core/modules/system/tests/themes/test_basetheme/test_basetheme.libraries.yml                           |  1 +
 core/modules/system/tests/themes/test_subtheme/test_subtheme.libraries.yml                             |  1 +
 core/modules/system/tests/themes/test_theme/test_theme.libraries.yml                                   |  8 ++++++++
 core/modules/system/tests/themes/test_theme_libraries_extend/test_theme_libraries_extend.libraries.yml | 11 +++++++++++
 core/modules/views_ui/views_ui.libraries.yml                                                           |  1 +
 8 files changed, 28 insertions(+), 2 deletions(-)

Only system.libraries.yml needed to be updated for the addition of item-list.module.css.

Also, I removed the file.theme.css library and override because the CSS file isn't there. A separate issue was created earlier to deal with it: https://www.drupal.org/node/2580049

Next up, some forms of automatic verification, and probably looking into the testbot fails.

Reverting some changes for #2581443: Make Classy extend from the new Stable base theme to deal with.

And I think stable.libraries.yml is not needed at all with the approach we are taking but instead I need to pay more attention to the overrides!

Wondering out loud whether we should consider copying assets referenced from CSS into Stable, like icons. These mostly just live in core/misc right now.

This should fix the fail.

Just to give an update I've been working on an automated test to verify the library overrides, and I think we can do something similar for the template overrides and such.

Here's a work in progress, this makes sure core module JS assets are all overridden by Stable. This was tricky to get working as a kernel test, the key was to install Stable first before installing all the modules. I spent quite a chunk of time trying to get things to work the other way around.

Edit: I know some services are unused etc. :)

A couple new patches here, the first to demonstrate that the tests will catch problems. The second will not be green either but will have less fails. We can either special case #2580049: Removed CSS files not removed from library definitions and editor.css which needs an issue created (I'll do so shortly) or be blocked on those getting in.

If someone wants to review the tests now that would be great. Tagging for more tests and updating the IS because we could use some test coverage for making sure we have all the Twig templates.

Okay we combined those two issues into one, thanks for the idea @nod_!

Making things more expandable.

@davidhernandez thanks, I brought that up in #23, I'm leaning towards copying them although there are still likely cases where paths would need to be adjusted.

Are you sure about the CSS loading thing? The way we are using libraries-override shouldn't the positions of the assets be preserved and therefore it wouldn't matter that the assets are actually in a theme? Maybe that needs an automated test.

@mdrummond yup I checked pretty thoroughly in #20 when I updated all the changed/added/removed assets and we'll work on adding tests to verify that as well.

And yes, any new things that are added, bugs fixed etc. will need to happen in Stable. One thing we haven't done yet is in Stable's templates is change the top line to start with "Theme override…" and remove the @ingroup themeable.

I'm going to:

• Create an issue to update the Twig extends, Library overrides etc. in Classy postponed on this and #2581443: Make Classy extend from the new Stable base theme
• Copy the image assets into Stable
• Try to bulk update the docs in all of the Stable templates

#2588289: Update Classy's Twig extends now that it extends from Stable

There are two interdiffs here, one to show the image/CSS changes, the other to show the Twig template docblock changes. The tricky part is that in the diff now some templates will actually be copied from Classy because they are actually more similar now that the docblocks have been updated. So some parts of the diff look a bit weird (removing classes etc.) because of this. Unfortunately the patch is also quite a bit larger now.

I'm still open to the idea of splitting this because the CSS/JS/images/libraries + tests part needs review but the template part still needs tests, for example. But the committers might not want that split.

Notes:

I used this regex to search/replace the @file stuff:

(Default theme implementation|Default template|Default theme override|Default view template)

I opened #2588373: Remove unused image files from core.

Just some cleanup for the library tests.

More test futzing/fixing.

This should definitely be reviewed by a JS maintainer. Unfortunately it copies a lot of JS and the idea is that bugfixes would need to be copied to Stable. Luckily as far as I know no changes would need to be made to those files.

Maybe we don't need to copy JS, or we can be more selective? I don't know.

I'm fine with that, the JS part really hadn't been discussed that much.

Here's what that would look like. This also removes the is_file() check because that will be run by the test added to #2580049: Removed CSS files not removed from library definitions anyway.

We can also update the .info.yml of Stable to take out "JavaScript"…

Going to take a look at some tests for the template overrides.

Here are the template tests, they caught one missing template (status-report.html.twig). The first patch should show more fails.

I tried to get away with only enabling, not installing modules, but Views/Views UI does quite a lot in its hook_theme() implementations. So the test is a bit slow, but I don't know how else to do it. One other thing I tried was enabling all modules except Views ones and then only installing Views and Views UI but that didn't work out.

Despite the fact that this patch will still be red, it should turn green once #2580049: Removed CSS files not removed from library definitions is committed so reviews, please!

Better assertion message.

Catching up with other changes.

Now that we have 0 theme functions 2 more templates needed to be copied to Stable.

Reviews please! I think we have quite a reasonable amount of test coverage but of course those need review as well.

Worth considering, thanks @davidhernandez. For now here is the patch split by library/CSS/image changes and template changes (and their accompanying tests). If we get the go-ahead from a core committer then we can split this issue into two.

For now we can review the patches separately, they don't conflict at all so it's not a big deal to maintain it like this, whether on this issue temporarily or on separate issues to be committed separately.

No changes, just splitting.

Breaking down the patch in the issue summary.

Thanks! Steps to reproduce the theme initialization error would be helpful, because when installing the sub theme we *should* be installing any dependencies as well. If that doesn't work that problem is bigger than Stable I think.

I don't remember what views mapping test is.

Thank you @davidhernandez!

Checking to see if anything needs updating since 15d0695. Commit hash based on #67. I also dug through the logs in a similar manner and didn't find any surprises. The patches don't need to be updated based on core changes.

git diff --stat 15d0695 core/modules/*.html.twig

 core/modules/block/templates/block.html.twig                     | 3 +--
 core/modules/system/templates/block--system-menu-block.html.twig | 3 +--
 core/modules/system/templates/status-report.html.twig            | 1 +
 3 files changed, 3 insertions(+), 4 deletions(-)

No changes needed here, the latest patch(es) copy these files correctly, all is well.

git diff --stat 15d0695 core/{misc,modules}/*.css

<no output>

Nothing to do.

git diff --stat 15d0695 core/core.libraries.yml core/modules/*.libraries.yml

 core/core.libraries.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 
The CKEditor library version was updated, no change needed.

Edit: Oops did a Markdown thing. Fixed.

So this does need a slight reroll after #2574975: Allow (base-)themes to be excluded from the UI (e.g. blocks, Appearance). got committed. The template patch doesn't need to change so please see #72 for that.

Yeah I'm not sure on that, we could check back to when that was added. Maybe that could be followup. Thanks!

For #85 we may need to think about using dependencies more with CSS libraries, right now a lot of these dependencies are not declared - if Seven's quickedit CSS does not rely on being loaded after the core stuff then it's fine, but if it does arguably it should be explicit about that.

As for JS yes that is intentional, see the discussion from around #51 to #56.

Ah yes very good point :)

Regarding #91 part of the reason the directory structure was kept uniform was so that we could have automated tests. If we start moving things around then we need to special case the tests for asset overrides. Otherwise I'd agree that things could be improved.

Attached should fix #82/#83 and #92, no other changes. Thank you very much @davidhernandez.

#2581443: Make Classy extend from the new Stable base theme is in now, so bringing over some changes from there to make this greener.

Agreed on #96, I meant to check that. Fixed here.

Calm down bots :)

I filed an issue for the color module weirdness.

Thank you very much for reviewing this @davidhernandez!

This issue is to create a snapshot of our templates and other frontend assets for backwards compatibility. If we don't copy the templates, CSS, and such, there's not much point to having Stable in core IMO. Around the time of Barcelona we had discussed some potential ways of handling backwards compatibility without copying everything and explored some solutions there. However, because of the way that Twig templates can reference each other by namespace (a similar thing applies to libraries), only copying templates and assets to Stable when we change them in core is not viable, so we need to shift things over to Stable to actually make Stable stable and work as a BC layer.

For example, if someone does this in a Twig template in a theme extending only from Stable and we didn't have a block template in Stable:

{% extends "block.html.twig" %}

They would be extending the core block module's block template. Then if/when we later make a change to that template, we can potentially break their site unless we first copy the template into Stable. That might sound like what we should do to handle BC, however templates can also be referenced by namespace:

{% extends "@block/block.html.twig" %}
{% extends "@stable/block/block.html.twig" %}

…and referencing templates via namespaces is needed in some cases to prevent infinite loops when extending. So without being able to reference @stable, people would in some cases be forced to couple themselves with core's wild west markup, knowing (or not knowing) that it's going to change from under them and break their site at some point in the future.

So with Stable being fully populated, people can reference the templates and assets from Stable rather than core and we are free to later update the block module's block template. People using Stable or Classy as a base theme won't see any of those changes unless they specifically reference the core template or copy the modified template into their theme.

Hope that makes sense :)

For some other takes on this see:

• #2575421-46: Add a Stable base theme to core and make it the default if a base theme is not specified (@davidhernandez)
• #2575421-55: Add a Stable base theme to core and make it the default if a base theme is not specified (@catch)
• #2575421-56: Add a Stable base theme to core and make it the default if a base theme is not specified (@mdrummond)

Edit: Fixed a technical inaccuracy, it would be @stable/block/block.html.twig, not just @stable/block.html.twig. The path is relative from the /templates directory and stable has a similar folder structure to Classy.

Tiny conflict with #2025707: Remove unused #theme "file_widget", straightforward reroll. Interdiff shows we no longer copy that file because it's gone.

Edit: Note that in my excitement I forgot to change the patch filename.

Thanks @catch! I think this could probably benefit from a change record telling people how to update their Twig/library extends etc.

If nobody beats me to it I can probably work on that about ~4h from now and put a draft up.

As discussed (on IRC) I think we can just edit that to add more info and maybe have an admin bump/republish it.

I updated https://www.drupal.org/node/2580687, see https://www.drupal.org/node/2580687/revisions/view/9101472/9118256. Maybe it can be republished.