Problem/Motivation

Once Classy extends from Stable it should not reference assets/templates/etc. from core modules.

Why this should be an RC target

Once we have all the templates and CSS library assets in Stable, anyone using Stable or Classy as a base theme should use the Stable or Classy namespace (depending) when they are extending templates or overriding libraries.

If we don't do this, then Classy and people working from Classy or Stable will be much more likely to extend core module templates and set themselves up for things breaking when we change those core templates later.

Bad:

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

Good:

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

The changes required should be non-disruptive and are part of proper completion of the parent issue, which is tagged as rc target.

Proposed resolution

Update templates and .info.yml files. We can work on a combined patch while waiting for the parent.

We will need to ensure that a child theme can override library assets that a parent theme has overridden via libraries-override.

Remaining tasks

Patch
Review

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Comments

Cottser created an issue. See original summary.

joelpittet’s picture

Issue tags: +rc target triage

Tagged from trello board for @Cottser. Remove if not needed.

xjm’s picture

Issue tags: -rc target triage

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary. If you're not sure if it should be an RC target, please wait to tag it so we can find the relevant trees in the forest. :)

mandclu’s picture

@slefevre1 and I are currently evaluating this issue.

mandclu’s picture

This ticket does not meet the criteria of rc eligible, so a core committer should decide if it can be tagged as rc target, like its parent issue.

mandclu’s picture

I had a conversation with @mradcliffe about whether or not it should be classified as rc eligible based on the fact that its parent issue has been labelled rc target. Documentation about the rc eligible tag does not currently include this as part of the criteria, so for now we will not tag this issue as rc eligible.

Cottser’s picture

It just needs some justification/explanation, I will add it as soon as I can. Thanks @xjm @mandclu et al :)

mandclu’s picture

Issue summary: View changes
Cottser’s picture

Issue summary: View changes
Issue tags: +rc target triage

Adding some more explanation and rationale to the issue summary, thanks @mandclu :)

Cottser’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.33 KB

Here's a start anyway. This is the interdiff from #2581443-10: Make Classy extend from the new Stable base theme that was later rolled back there because it didn't really fit.

Status: Needs review » Needs work

The last submitted patch, 10: update_classy_s_twig-2588289-10.patch, failed testing.

Cottser’s picture

Title: Update Classy's Twig extends, library overrides etc. now that it extends from Stable » Update Classy's Twig extends now that it extends from Stable
Status: Needs work » Needs review
FileSize
186.5 KB
926 bytes

I looked at all the library stuff in .info.yml files, attach_library in templates, extends/import/include/embed in Twig templates. Rolling back the Seven change because that's not as urgent.

Attached is a combined patch with #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS as well as a separate patch with just the relevant changes for this issue. I think this should be all that we need to change in Classy.

The last submitted patch, 12: update_classy_s_twig-2588289-12-combined.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: update_classy_s_twig-2588289-12.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
194.36 KB

Oops of course I need to also add #2581443: Make Classy extend from the new Stable base theme :)

This is just a new combined patch, I know the partial patch won't pass anyway.

Status: Needs review » Needs work

The last submitted patch, 15: update_classy_s_twig-2588289-15-combined.patch, failed testing.

Cottser’s picture

So when we start stacking libraries-override it looks like we might want to allow syntax like this (separate issue to be created):

Before:

  core/drupal.dropbutton:
    css:
      component:
        /core/themes/stable/css/core/dropbutton/dropbutton.css: /themes/my_theme/css/dropbutton.css

After:

  core/drupal.dropbutton:
    css:
      component:
        '@stable/css/core/dropbutton/dropbutton.css': /themes/my_theme/css/dropbutton.css

Status: Needs review » Needs work

The last submitted patch, 17: update_classy_s_twig-2588289-17.patch, failed testing.

Cottser’s picture

Title: Update Classy's Twig extends now that it extends from Stable » Update Classy's Twig extends and library overrides now that it extends from Stable
Cottser’s picture

Title: Update Classy's Twig extends and library overrides now that it extends from Stable » Update Classy's Twig extends now that it extends from Stable
Status: Needs work » Needs review
FileSize
188.17 KB
898 bytes
2.29 KB

The libraries stuff needed to be rolled into #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS anyway so reverting it from here.

Also just using the registry loader for the local actions and tasks blocks per discussion on #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS and on IRC. We landed on being consistent over special casing certain blocks/templates.

The interdiff is not particularly useful here, better just to review the tiny (non combined) patch. It won't pass until #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS is in, but the combined patch includes the latest patch from there and should pass.

Cottser’s picture

Actually never mind what I said about it not passing, since this is now using the registry loader it can be reviewed committed with or without the other issue going in :D

davidhernandez’s picture

The Stable stuff is in now so we can do this.

Cottser’s picture

The non-combined patch should work and apply, might be easier if I just repost it but on my phone atm.

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
898 bytes

Re-uploading Cottser's patch to make it easier.

I checked Classy and these are the only extends, includes, macros, or anything else that needed updating. In prior IRC discussions we agreed to go with using the loader over explicitly specifying the location of the template.

Cottser’s picture

Thanks!

xjm’s picture

Issue tags: -rc target triage +rc target

@alexpott, @effulgentsia, and I agreed that this is an RC target.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • xjm committed 82829f8 on 8.1.x
    Issue #2588289 by Cottser, davidhernandez, mandclu: Update Classy's Twig...

Status: Fixed » Closed (fixed)

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