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
We want to make Classy depend on Stable.
Currently, fatal errors occur when you try to visit update.php after we remove base theme: false
from Classy's .info.yml. For update.php to work the theme displaying it needs to work.
Proposed resolution
If Stable is missing in theme initialization, ignore it.
Remaining tasks
None
User interface changes
n/a
API changes
n/a
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff.txt | 1.78 KB | star-szr |
#41 | make_classy_extend_from-2581443-41.patch | 7.74 KB | star-szr |
#40 | make_classy_extend_from-2581443-40.patch | 7.86 KB | star-szr |
#36 | 2581443-36.patch | 7.86 KB | alexpott |
#36 | 35-36-interdiff.txt | 798 bytes | alexpott |
Comments
Comment #2
star-szrWe can maybe make Classy extend from Stable in this issue as well. Let's see how it goes, we can split later if needed.
Comment #3
star-szrI think we need something better than this because as soon as someone calls something like getActiveTheme() on ThemeManager or ThemeInitialization during updates things will probably go wrong. But here is something @lauriii and myself have been kicking around, he did the fancy things.
Originally I wanted to just put this stuff right into ThemeInitialization but I guess that's not good.
Comment #4
alexpottDiscussed with @catch and we agreed that this is an RC target since if we don't fix this then we'll have to copy all templates to Classy and Stable.
Comment #7
lauriiiComment #8
star-szr@lauriii mentioned in the frontend meeting today maybe doing something that's hanging off of UpdateKernel, that seemed sensible to me.
Comment #9
alexpottThis is the first proper use case for #2547507: Consider adding hook_update_early_N() support. I'm not sure that it means we should do the issue though.
There are a number of ways to go here... one possibility is to use a theme other than seven as the maintenance theme - but I think that this is a lot of work. Since we're in favour of doing the smallest possible changes during RC I think doing something special in the update controller might be a good idea.
Comment #10
star-szrJust want to make sure we don't forget this interdiff, moving from #2575737-21: Copy templates, CSS, and related assets to Stable and override core libraries' CSS with an additional change I caught. Any themes extending from Classy or Stable should not reference core libraries/templates/assets/etc. because they may change.
Comment #11
star-szrAlthough that type of change may need to be follow-up to both of these issues unless #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS gets in first. So maybe roll that back for now because it will make tests fail.
Comment #13
lauriiiLets see what happens
Comment #14
lauriiiRemoved system.module.orig
Comment #15
lauriiiGarbage collection
Comment #16
star-szrThanks @lauriii!
Minor: I think this
use
is no longer needed.Update @see to point to the relevant spot.
Minor: Missing @param for $theme_installer.
How can we test this code?
Comment #18
lauriiiComment #21
star-szrCreated #2588289: Update Classy's Twig extends now that it extends from Stable for things like #10.
Comment #22
star-szrComment #25
alexpottSo in order to get this to work we need to clear both the system list in state and the system list in cache - if you only clear the cache it uses state to rebuild the cache and if you only clear state then it'll read from the cache.
Comment #28
alexpottFixing the remaining fails.
Comment #30
alexpottHmmm...
This behaviour of copying blocks around is completely incorrect for base themes. We need #2574975: Allow (base-)themes to be excluded from the UI (e.g. blocks, Appearance). and then adjust
block_theme_initialize()
to not copy blocks for hidden themes.Comment #31
alexpottComment #32
alexpottA different approach and one that is not dependent at all on doing early update stuff. Basically it just ignores the missing stable base theme if it is not present because it is very special situation. In this way the update can proceed and enable stable as required.
Also I'm not that sure that the original update actually worked because of the way theme data is cached in cache and state (the system list is persisted across cache rebuilds here). We should have used
ThemeHandler::refreshInfo()
and notThemeHandler::rebuildThemeData()
.Comment #33
star-szrManual testing seems to check out, bot is happy, and this approach seems sensible to me. I could only find these minor points on review:
Minor: I think this line can be removed now since we don't throw the exception there any more.
Minor: Missing period at the end.
Comment #34
lauriiiThanks a lot for figuring this out @alexpott. I tried figure out this personally for a long time but didn't make any progress. This looks pretty much RTBC beside this:
Is these docs copy paste from tests?
Comment #35
alexpott@lauriii nope. But looking at these docs again makes me think we should just remove system_update_8012() and use the same one-liner. The
// Ensure we have fresh info.
is just unnecessary documentation.Comment #36
alexpottHere's a patch to show why we need to postpone this on #2574975: Allow (base-)themes to be excluded from the UI (e.g. blocks, Appearance).. Once that has landed installing stable will no longer automatically place blocks for it.
Comment #37
alexpottDoing the postponing for realz.
Comment #39
alexpottYep it fails without #2574975: Allow (base-)themes to be excluded from the UI (e.g. blocks, Appearance). :)
Comment #40
star-szrJust a reroll after #2574975: Allow (base-)themes to be excluded from the UI (e.g. blocks, Appearance). got committed, no changes.
Comment #41
star-szrOkay so the only things I can see that need work are #33 (and removing the reference in that chunk of docs to
system_update_8012()
which is being removed now).Since it's docs only, rolling in here and RTBC'ing – based on my previous reviews and @lauriii's in #34. I think @alexpott, @lauriii, and me are all happy with this now :)
Comment #42
lauriiiRTBC++
Comment #43
catchCommitted/pushed to 8.0.x, thanks!