Problem/Motivation

#3259188: Extend post update system to provide themes a way to install newly-required dependencies added the ability to add post updates to themes. The first issue to add an update to Olivero - #3257274: Implement color changing theme settings for Olivero - has run into an issue that if the theme is used during the installer then there is a problem as _drupal_maintenance_theme() will add the not installed theme to the theme handler. This results in the theme appearing to be installed when actually it is not.

This is only happening in tests because when install_drupal() is called by \Drupal\Core\Test\FunctionalTestSetupTrait::doInstall() the MAINTENANCE_MODE constant is not defined and set to install like it is if you do an interactive install via core/install.php or via Drush in \Drush\Commands\core\SiteInstallCommands::pre().

Steps to reproduce

See https://www.drupal.org/pift-ci-job/2377453

Proposed resolution

Use the list in configuration rather than the list in the module handler and the theme handler.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#3 3279850-3.patch5.36 KBalexpott
#3 3279850-3.test-only.patch3.53 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes

This is only happening in tests because when install_drupal() is called by \Drupal\Core\Test\FunctionalTestSetupTrait::doInstall() the MAINTENANCE_MODE constant is not defined and set to install like it is if you do an interactive install via core/install.php or via Drush in \Drush\Commands\core\SiteInstallCommands::pre().

How completely funky.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3279850-3.test-only.patch3.53 KB
3279850-3.patch5.36 KB

Here's the fix from #3257274: Implement color changing theme settings for Olivero with a test.

The last submitted patch, 3: 3279850-3.test-only.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented 
+++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
@@ -337,16 +337,23 @@ protected function includeThemes(): bool {
+      $old_extension_list = array_keys($config->getOriginal('module') ?? []);

It is a bit strange that we are getting the list of extensions from config instead of changing the factory to use source with correct data given that the method is updating ::enabledExtensions. This means that the class is initialized with incorrect list of extensions. I can't think of a concrete problem this would cause but I'm not sure I'm comfortable moving this to RTBC.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

Realized that the source of ::enabledExtensions is already changed by the existing code, what is changed here is how we do the comparison. Based on that, I think this is fine.

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented
Issue tags: +Bug Smash Initiative

Test-only fails in a very expected way:

There was 1 error:

1) Drupal\Tests\system\Functional\Theme\MaintenanceThemeUpdateRegistryTest::testMaintenanceThemeUpdateRegistration
Behat\Mink\Exception\ResponseTextException: The text "No pending updates." was not found anywhere in the text of the current page.

I closely reviewed the patch and couldn't find a single nit. Tagging to be smashed, and +1 RTBC.

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented

p.s. FWIW, I was part of #3259188: Extend post update system to provide themes a way to install newly-required dependencies, so I'm familiar with all this. Funny that none of us thought of this when we worked on that. 😅 That's how it goes.

p.p.s. Saving credit for reviews.

p.p.p.s. Given that #3259188 was part of 9.4.0-alpha1, do we need a CR for this?

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented

p.p.p.p.s.: Whoops. 😉 Both of the CRs for #3259188 are still draft:

Do we mass publish all draft CRs with Introduced in version set to 9.4.0 once 9.4.0 is shipped?

Or are we supposed to manually publish them now that alpha1 is out?

This is part of what confuses me about not being precise with when a change was introduced (alpha1, beta1, whatever) and always going with Y.X.0 in CRs for Introduced in version.

dww’s picture

dww
we/he/they
CreditAttribution: dww at TEN7 commented

I opened a thread about this in #bugsmash. @larowlan, @darvanen, @kimb0 and myself agreed that since the existing CRs for this haven't been published, better to add a note about this and mark it as a related issue than to open yet another CR about it.

@larowlan approved the changes, so I published those 2 CRs.

Therefore, no other CR changes needed.

I don't think this needs a release note snippet, either.

Thanks!
-Derek

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

FWIW I looked at changing \Drupal\Core\Update\UpdateRegistryFactory::create()

public function create() {
$extensions = array_merge(array_keys($this->container->get('module_handler')->getModuleList()), array_keys($this->container->get('theme_handler')->listInfo()));
return new UpdateRegistry($this->container->getParameter('app.root'), $this->container->getParameter('site.path'), $extensions, $this->container->get('keyvalue')->get('post_update'));
}

to use configuration but if the service is created during a configuration save it gets the yet to be saved configuration and not configuration as it was when the install began. Therefore I think we should leave this well alone. Both the module and theme handlers are guaranteed to be instantiate prior to the module or theme installer change configuration. This stuff is always a bit of a hornets nest and think the change here is the best we can do given the existence of _drupal_maintenance_theme() and it's ability to fake install a theme.

FWIW there is another issue that becomes apparent. I think we should file a follow-up to #474684: Allow themes to declare dependencies on modules to discuss what a module dependencies means for _drupal_maintenance_theme() :D

  • catch committed ed6684d on 9.4.x 
    Issue #3279850 by alexpott, dww, lauriii: Theme post updates are not...
  • catch committed 06f6126 on 9.5.x
    Issue #3279850 by alexpott, dww, lauriii: Theme post updates are not...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x, cherry-picked to 9.5.x and 9.4.x, thanks!

  • catch committed 144568e on 10.0.x 
    Issue #3279850 by alexpott, dww, lauriii: Theme post updates are not...

Status: Fixed » Closed (fixed)

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