Progress bar, fieldsets, messages broken in the installer due to theme ordering bug

It looks like Classy's progress.css isn't being loaded. The library should be attached in classy/progress-bar.html.twig

We should check to see if it is loading Stable or core's progress-bar template since those don't load a library.

Yup

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'progress_bar' -->
<!-- BEGIN OUTPUT from 'core/themes/stable/templates/misc/progress-bar.html.twig' -->
<div class="progress" data-drupal-progress>
    <div class="progress__track"><div class="progress__bar" style="width: 0%"></div></div>
  <div class="progress__percentage">0%</div>
  <div class="progress__description">Initializing.<br />&nbsp;</div>
</div>

<!-- END OUTPUT from 'core/themes/stable/templates/misc/progress-bar.html.twig' -->

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?

Wondering 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.

Seems to be loading all the CSS on that page just for that details template it's loading stable instead of classy's template, bizarre.

The same thing can be also observed when running a test via the UI.Rebuilding the cache helped, sorry

the templates use of Stable theme templates may have to do with the changes made by Make Classy extend from the new Stable base theme

@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.

Working 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.

Still 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

Things look good here:

As this affects the product, I'm tagging for Angie to have a look as I really thing this needs to get in for release.

1. +++ b/core/includes/theme.maintenance.inc
   @@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
   +    $base_themes[] = $themes[$themes[$ancestor]->base_theme];
        $ancestor = $themes[$ancestor]->base_theme;
   

   Do you mind making it a bit easier to read, so just use $ancestor = ...; $base_themes[] = $themes[$ancestor];

2. +++ b/core/includes/theme.maintenance.inc
   @@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
   -  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], array_reverse($base_theme)));
   +  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], $base_themes));
   

   This is the right fix, as its the same as we did in \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName itself.

+++ b/core/includes/theme.maintenance.inc
@@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
   // @todo This is just a workaround. Find a better way how to handle themes
   //   on maintenance pages, see https://www.drupal.org/node/2322619.
-  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], array_reverse($base_theme)));
+  \Drupal::theme()->setActiveTheme($theme_init->getActiveTheme($themes[$custom_theme], $base_themes));

+++ b/core/lib/Drupal/Core/Theme/ThemeInitializationInterface.php
@@ -57,8 +57,8 @@ public function loadActiveTheme(ActiveTheme $active_theme);
-   *    An array of extension objects of base theme and its bases. It is ordered
-   *    by 'oldest first', meaning the top level of the chain will be first.
+   *   An array of extension objects of base theme and its bases. It is ordered
+   *   by 'next parent first', meaning the top level of the chain will be first.

So 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.

+++ b/core/includes/theme.maintenance.inc
@@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
-  $base_theme = array();
+  $base_themes = array();
...
-    $base_theme[] = $themes[$themes[$ancestor]->base_theme];
+    $base_themes[] = $themes[$themes[$ancestor]->base_theme];

Also, technically these changes aren't in scope, but I'm okay with it.

#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.

So 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.

#19++

Definitely would love to see this in before release. Not sure what other feedback you need from a PM point of view.

Feel 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

I'll try to come up with some regression tests.

Talked to @dawehner and found some time so I'll take the tests on.

@xjm can you look at 'blessing' this with "rc target"?

I discussed this with xjm a bit, and I think it was enough discussion to tag it.

Thank 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.

So we could do something like this ...

Here's my attempt, need to merge in #32

Added 32 tests and here is #32 tests only patch as well.

The patch from #34is working for a clean install with RC4, PHP 5.5.16 and MySQL 5.6.24

I'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)

Here's a failing patch based on @dawehner's idea.

Nice I'd say this looks like it has enough test coverage. Thanks for helping get both types of regression tests working @alexpott.

Nits/documentation things, don't mind me. Everything else is looking good to me and I'd say this is ready to go.

1. +++ b/core/includes/theme.maintenance.inc
   @@ -82,19 +82,19 @@ function _drupal_maintenance_theme() {
   -  $base_theme = array();
   +  $base_themes = array();
   

   If we're going to chagne it might as well [].

2. +++ b/core/modules/system/src/Tests/Installer/InstallerThemeTest.php
   @@ -0,0 +1,45 @@
   +use Drupal\simpletest\KernelTestBase;
   +
   +
   +/**
   

   Nit: Extra blank line here.

3. +++ b/core/modules/system/src/Tests/Installer/InstallerThemeTest.php
   @@ -0,0 +1,45 @@
   +   * Tests themes and base themes are correctly loaded.
   +   * Regression test for: https://www.drupal.org/node/2614014
   

   I think there needs to be a summary line and then a blank line per https://www.drupal.org/node/1354#general.

4. +++ b/core/modules/system/src/Tests/Installer/InstallerThemeTest.php
   @@ -0,0 +1,45 @@
   +    // Do we have an active
   +    $this->assertTrue(\Drupal::theme()->hasActiveTheme());
   

   Do we have an active… theme?

5. +++ b/core/modules/system/src/Tests/Installer/InstallerThemeTest.php
   @@ -0,0 +1,45 @@
   +    // Ensure base theme's have proper base themes and the right count.
   

   s/theme's/themes/, but this comment is hard to read IMO, I will try to reword it.

6. +++ b/core/modules/system/src/Tests/Installer/StandardInstallerTest.php
   @@ -41,6 +41,17 @@ protected function setUpSite() {
   +    // Ensure that we see the classy progress css on the page.
   

   Nit: s/css/CSS/.

Testbot is ornery today

Fixed a couple of minor things here and there.

DrupalCI you are lying.

1. +++ b/core/includes/theme.maintenance.inc
   @@ -82,19 +82,21 @@ function _drupal_maintenance_theme() {
        if ($ancestor) {
   -      // Ensure that the base theme is added.
   +      // Ensure that the base theme is added and installed.
          $theme_handler->addTheme($themes[$ancestor]);
        }
   

   We 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:

           if ($ancestor == 'stable') {
             // Themes that depend on Stable will be fixed by system_update_8014().
             // There is no harm in not adding it as an ancestor since at worst
             // some people might experience slight visual regressions on
             // update.php.
             continue;
           }
   

   Shouldn't we then at least document why this part is different, why it's only a partial duplicate?

2. +++ b/core/modules/system/src/Tests/Installer/StandardInstallerTest.php
   @@ -41,6 +41,19 @@ protected function setUpSite() {
   +      $this->assertRaw('themes/classy/css/components/progress.css');
   

   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?

3. +++ b/core/tests/Drupal/KernelTests/Core/Theme/MaintenanceThemeTest.php
   @@ -0,0 +1,43 @@
   +   * Ensures that the maintenance theme initializes theme and their base themes.
   

   This reads kinda strange.

We 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:

Well 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.

Thanks @Wim Leers

1. I agree with @dawehner I don't think we need a comment about that it is a pretty special situation that only occurs during update.php and for sites before rc-4.
2. Yes it is ugly but also I don't think we should be adding @todo's without an issue to point to and visual regression testing is some way off afaik
3. Fixed

Given 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.

Removing the regression tag given that all bugs are more or less a regression. There are better ways to add attention to an issue.

Committed/pushed to 8.0.x, thanks!