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
Install head with English and when installing modules:
When setting up configuration:
When installing in a foreign language:
After installing in a foreign language (until you clear the cache):
Proposed resolution
Figure out what causes the visual problems. Fix.
Remaining tasks
Figure it out. Fix it.
User interface changes
API changes
TBD.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#60 | 2614014-2-60.patch | 4.97 KB | alexpott |
| |||
#60 | 50-60-interdiff.txt | 670 bytes | alexpott |
#50 | interdiff.txt | 5.31 KB | dawehner |
#50 | 2614014-50.patch | 4.97 KB | dawehner |
| |||
#44 | interdiff.txt | 2.87 KB | star-szr |
Comments
Comment #2
LewisNymanIt looks like Classy's progress.css isn't being loaded. The library should be attached in classy/progress-bar.html.twig
Comment #3
Gábor HojtsyAssigning to classy then(?)
Comment #4
davidhernandezWe should check to see if it is loading Stable or core's progress-bar template since those don't load a library.
Comment #5
davidhernandezYup
All the installer's templates are coming from Stable, which does not have a progress bar library. I'm not sure why, but the installer is loading both Classy and Stable, but only acts like its template suggestions go to Stable.
It doesn't look to be a Classy issue. It certainly was unearthed when we added all the Stable stuff, which was a recent commit. I think though this is an installer special snowflake issue?
Comment #6
joelpittetWondering if this is also related: The fieldset on installer looks a bit wrong and doesn't have the CSS to toggle it: EDIT: has JS toggling just doesn't look good or work.
Comment #7
joelpittetSeems to be loading all the CSS on that page just for that details template it's loading stable instead of classy's template, bizarre.
Comment #8
dawehnerThe same thing can be also observed when running a test via the UI.Rebuilding the cache helped, sorryComment #9
LittleCodingthe templates use of Stable theme templates may have to do with the changes made by Make Classy extend from the new Stable base theme
Comment #10
LewisNyman@joelpittet It could be, because no templates are being loaded from Classy. Templates are being loaded from Seven though, so it could be that templates are only being loaded from the highest base theme on the installer only.
This patch fixed the reported symptom by copying the Classy template over to Seven. There's something else going on here but if we run out of time to fix this before release it would be great if the installer did ship looking like crap.
Comment #11
joelpittetWorking through this:
core/includes/theme.maintenance.inc:96
Still trying to see what's at the root of it but it looks like it's only dealing with one of the base themes at that line.
Also this breakpoint is weird saying that classy is the baseTheme for Stable and Classy.
Comment #12
joelpittetStill not perfect because Stable is the basetheme for stable for some reason but at least things seem to be loading correctly. It just looks like it was loading the base themes in reverse.
I could be completely wrong, please try it out. And bumping to major
Comment #13
joelpittetThings look good here:
Comment #14
joelpittetAs this affects the product, I'm tagging for Angie to have a look as I really thing this needs to get in for release.
Comment #15
dawehnerDo you mind making it a bit easier to read, so just use
$ancestor = ...; $base_themes[] = $themes[$ancestor];
This is the right fix, as its the same as we did in
\Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName
itself.Comment #16
xjmSo this is actually a pretty significant behavior change, no? When did this bug get introduced?
Defnitely would need tests. The inline comment also links this other issue:
#2322619: Remove workarounds in the theme system for the maintenance page and the installer
I agree that it would be extremely unsightly to ship 8.0.0 with this regression and we should fix it before release if possible. However, the patch points to a much deeper problem and who knows what side effects it would have, which makes me nervous. Tests would help.
Comment #17
xjmAlso, technically these changes aren't in scope, but I'm okay with it.
Comment #18
dawehner#2389735: Core and base theme CSS files in libraries override theme CSS files with the same name fixed the same bug on total different subsystems.
Comment #19
dawehnerSo yeah IMHO the code should as similar as possible to
\Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName
and then maybe in 8.1 we could get rid of that sadness.
Comment #20
Wim Leers#19++
Comment #21
Gábor HojtsyUpdated title / summary with fieldset problem.
Comment #22
Gábor HojtsyComment #23
webchickDefinitely would love to see this in before release. Not sure what other feedback you need from a PM point of view.
Comment #24
joelpittetFeel free to work on tests , I won't be able to until later and don't want to hold this up.
It looks like it never worked quite right and the second base theme just exposed it
Comment #25
xjmComment #26
dawehnerI'll try to come up with some regression tests.
Comment #27
joelpittetTalked to @dawehner and found some time so I'll take the tests on.
@xjm can you look at 'blessing' this with "rc target"?
Comment #28
catchI discussed this with xjm a bit, and I think it was enough discussion to tag it.
Comment #29
joelpittetThank you for discussing it @xjm and @catch.
I'm adding a new Test. The approach I'm planning is to check the ActiveTheme on the language page and Assert on the base themes if possible. As I don't think targeting specific CSS/markup is a decent test unless I write 2-3 installer sub themes and check their inheritance.
Hope that will suffice.
Comment #30
Gábor HojtsyMessages also look messed up (lacking styling):
Comment #31
Gábor HojtsyI found yet one more bug that this patch fixes. If you install HEAD in a foreign language (in this case Hungarian), then your breadcrumbs and your seven shortcut star shows up all broken. (Until you clear your cache). I think the set of problems at this point is worthy of the critical label.
The proposed patch fixed the messages in the installer and this problem after a foreign language install as well.
Comment #32
dawehnerSo we could do something like this ...
Comment #33
joelpittetHere's my attempt, need to merge in #32
Comment #34
joelpittetAdded 32 tests and here is #32 tests only patch as well.
Comment #38
LittleCodingThe patch from #34is working for a clean install with RC4, PHP 5.5.16 and MySQL 5.6.24
Comment #40
joelpittetI've been trying to get the test only patch from #32 to fail but locally I can't get it to fail either. The idea was neat but I don't know how to ensure that condition will evaluate on the batch page. (Was testing through the UI)
Comment #41
alexpottHere's a failing patch based on @dawehner's idea.
Comment #43
joelpittetNice I'd say this looks like it has enough test coverage. Thanks for helping get both types of regression tests working @alexpott.
Comment #44
star-szrNits/documentation things, don't mind me. Everything else is looking good to me and I'd say this is ready to go.
If we're going to chagne it might as well [].
Nit: Extra blank line here.
I think there needs to be a summary line and then a blank line per https://www.drupal.org/node/1354#general.
Do we have an active… theme?
s/theme's/themes/, but this comment is hard to read IMO, I will try to reword it.
Nit: s/css/CSS/.
Comment #46
joelpittetTestbot is ornery today
Comment #50
dawehnerFixed a couple of minor things here and there.
Comment #52
alexpottDrupalCI you are lying.
Comment #58
Wim LeersWe state that this code block is basically a duplicate of
\Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName()
. Yet we omit one important part that we see there:Shouldn't we then at least document why this part is different, why it's only a partial duplicate?
This is the most ugly, the most hacky test I've ever seen I think… but I guess it's justified here.
I'd almost say: let's add a @todo pointing to the visual regression testing issue, because once we have that, we don't need this hack anymore; we'll be able to test it much more elegantly/sanely.
Also: why is the other test alone insufficient?
This reads kinda strange.
Comment #59
dawehnerWell yeah, the thing is the block after that ... the exception when a theme is not installed yet ... at install time this is always the case, so the ThemeInitialization though we just have a workaround in place for some update path issue.
Comment #60
alexpottThanks @Wim Leers
Comment #61
alexpottGiven my contributions are all about test coverage and documentation I think it is okay if I RTBC this patch. It fixes the serious visual regression and adds complete test coverage.
Comment #62
dawehnerRemoving the regression tag given that all bugs are more or less a regression. There are better ways to add attention to an issue.
Comment #63
catchCommitted/pushed to 8.0.x, thanks!
Comment #65
Gábor HojtsyYay, amazing, thanks all!
Comment #76
Gábor Hojtsy