Problem/Motivation

A philosopher once said:

Don't it always seem to go
That you don't know what you've got 'til it's gone

In this case, the thing we lost that we didn't know we had is the ability to theme the installer. It is not loading the theme, or themes, it needs to. Drupal CMS is severely affected by this because of its extensive installer modifications, but vanilla core is too.

The problem is that theme hooks, which can now be object-oriented, are not executed because they are not discovered by the theme hook collector. They are not discovered by the hook collector because the hook collector depends on the container.themes container parameter being accurate, based on what's in DrupalKernel::$themeList, and...it's not. It's an empty array.

But that's not actually true; the installer does indeed load the necessary chain of themes, in drupal_maintenance_theme(). But by that point, it's too late: the installer kernel has been fully initialized, and the hooks have been (not) collected. Result: one very sad-looking installer.

Steps to reproduce

Just spin up Drupal 11.3.0-rc1 and visit the installer to weep gently as you behold this sorry excuse for a page:

Proposed resolution

1. Add the profiles earlier to install state so we can use this info during container booting
2. Copy the logic from _install_select_profile() so we can work out the profile and get info while booting the container
3. Resolve the installer theme and add to the container if necessary.

Remaining tasks

Fix it with a test.

User interface changes

None, unless you count "restoring the decent UX that we previously had" as a change.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

None necessary, this is squarely a bug fix.

CommentFileSizeAuthor
11.3.0-installer-sad-trombone.png70.06 KBphenaproxima

Issue fork drupal-3562319

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

phenaproxima opened merge request !14073

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: Unassigned » nicxvan
Issue summary: View changes
Status: Active » Needs work

Wrote a regression test here, but I think @nicxvan is better suited than I to fix this. Unlike me, he understands this stuff.

Adding this snippet in DrupalKernel::compileContainer() will make the test pass:

    $this->themeList = [
      'custom_installer_theme' => [
        'type' => 'theme',
        'pathname' => 'core/profiles/tests/testing_installer_theme/theme/custom_installer_theme.info.yml',
        'filename' => 'custom_installer_theme.theme',
      ],
    ];
   /// ADDED FOR CONTEXT
    $container->setParameter('container.themes', $this->getExtensionsParameter(
      $this->themeList ?? [], [$this, 'themeExtensions'])
    );

nicxvan opened merge request !14076

nicxvan opened merge request !14077

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes

alexpott made their first commit to this issue’s fork.

alexpott opened merge request !14080

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

This blocks the release of 11.3.0 too!

I've added a new MR that includes and improves on the test case. Plus the approach works. It almost certainly results in more directory scanning and yaml parsing during the install but that might be something we have to take on the chin for 11.3.0 and then work out how to fix in a bigger refactor of the installer.

catch’s picture

catch
he/him
Primary language English
commented

Manually tested !14080

Claro loads on the first page with stock head.

If you hack standard.info.yml to add this courtesy of @alexpott:

distribution:
  install:
    theme: olivero

Then the first page of the installer loads in Olivero.

I hit a weird issue with Olivero as the distribution theme where I then got a fatal error the first time the front page of standard loaded (second page was OK). I think this is... not the fault of the MR but the hacky way of testing it. With stark as the installation theme, first page of standard was fine.

Didn't profile but install was still pretty snappy, we can live with the extra YAML parsing here, and try to use this issue as motivation to modernise some of this stuff sooner than later.

alexpott changed the visibility of the branch 3562319-parse-profile-for-install-theme to hidden.

alexpott changed the visibility of the branch 3562319-minimal-change-claro-default to hidden.

alexpott changed the visibility of the branch 3562319-regression-the-installer to hidden.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: nicxvan » Unassigned
Status: Needs review » Reviewed & tested by the community

This fixes Drupal CMS. Ship it.

nicxvan’s picture

nicxvan commented
Status: Reviewed & tested by the community » Needs work

Sorry, a couple questions on the MR, one is important I think.

nicxvan’s picture

nicxvan commented
Status: Needs work » Reviewed & tested by the community

Questions answered, this looks great!

nicxvan closed merge request !14073

nicxvan closed merge request !14076

nicxvan closed merge request !14077

nicxvan’s picture

nicxvan commented

Reviewed the two commits since RTBC, still RTBC.

  • catch committed 451e2dfc on 11.3.x 
    fix: #3562319 [regression] The installer does not properly load the...

  • catch committed b6f6b221 on 11.x 
    fix: #3562319 [regression] The installer does not properly load the...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks! The fact we found at least two undiscovered bugs while fixing this one shows how arcane this stuff is.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14080

Status: Fixed » Closed (fixed)

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