Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | update_classy_s_twig-2588289-20.patch | 898 bytes | davidhernandez |
| #20 | interdiff.txt | 2.29 KB | star-szr |
| #20 | update_classy_s_twig-2588289-20.patch | 898 bytes | star-szr |
| #20 | update_classy_s_twig-2588289-20-combined.patch | 188.17 KB | star-szr |
| #17 | interdiff.txt | 1.4 KB | star-szr |
Comments
Comment #2
joelpittetTagged from trello board for @Cottser. Remove if not needed.
Comment #3
xjmThanks 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. :)Comment #4
mandclu CreditAttribution: mandclu commented@slefevre1 and I are currently evaluating this issue.
Comment #5
mandclu CreditAttribution: mandclu commentedThis 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.
Comment #6
mandclu CreditAttribution: mandclu commentedI 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.
Comment #7
star-szrIt just needs some justification/explanation, I will add it as soon as I can. Thanks @xjm @mandclu et al :)
Comment #8
mandclu CreditAttribution: mandclu commentedComment #9
star-szrAdding some more explanation and rationale to the issue summary, thanks @mandclu :)
Comment #10
star-szrHere'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.
Comment #12
star-szrI 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.
Comment #15
star-szrOops 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.
Comment #17
star-szrSo when we start stacking libraries-override it looks like we might want to allow syntax like this (separate issue to be created):
Before:
After:
Comment #19
star-szrComment #20
star-szrThe 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.
Comment #21
star-szrActually 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
Comment #22
davidhernandezThe Stable stuff is in now so we can do this.
Comment #23
star-szrThe non-combined patch should work and apply, might be easier if I just repost it but on my phone atm.
Comment #24
davidhernandezRe-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.
Comment #25
star-szrThanks!
Comment #26
xjm@alexpott, @effulgentsia, and I agreed that this is an RC target.
Comment #27
xjmCommitted and pushed to 8.0.x. Thanks!