Extend post update system to provide themes a way to install newly-required dependencies [#3259188]

Problem/Motivation

In #474684: Allow themes to declare dependencies on modules we enabled themes to have module dependencies but we provided no way for an existing theme to add a module as a dependency without breaking.

Steps to reproduce

Add a dependency to a theme that is already installed on a site. It will break if the theme relies on code from the module and the module is not installed.

Proposed resolution

Gives themes the ability to have post update hooks. This would mean that they can install modules and also fix their config if they've changed their theme setting config structure.

Remaining tasks

Fix drush: https://github.com/drush-ops/drush/pull/5120
Harden tests per #16?

User interface changes

API changes

Themes can implement HOOK_post_update_NAME() and HOOK_removed_post_updates()
\Drupal\Core\Update\UpdateRegistry::getModuleUpdateFunctions() is deprecated in favour of \Drupal\Core\Update\UpdateRegistry::getUpdateFunctions()
\Drupal\Core\Update\UpdateRegistry::filterOutInvokedUpdatesByModule() is deprecated in favour of \Drupal\Core\Update\UpdateRegistry::filterOutInvokedUpdatesByExtension()

Data model changes

Release notes snippet

Issue fork drupal-3259188

Show commands

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

Add & fetch this issue fork’s repository

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

Add & fetch this issue fork’s repository

3259188 changes, plain diff MR !1676
Check out this branch for the first time

Check out existing branch, if you already have it locally
3259188-allow-themes-are compare
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

18 January 2022 at 16:55

alexpott created an issue. See original summary.

Comment #2

cilefen commented 18 January 2022 at 17:23

Title:

Allow themes are way to install newly dependencies

» Allow themes are way to install newly-required dependencies

Comment #3

bbrala

Netherlands

commented 18 January 2022 at 17:27

Title:

Allow themes are way to install newly-required dependencies

» Allow themes a way to install newly-required dependencies

Comment #4

18 January 2022 at 18:12

alexpott opened merge request !1676

Comment #5

alexpott

he/they

English

🇪🇺🌍

commented 18 January 2022 at 18:58

Status:

Active

» Needs review

Comment #6

lauriii

he/him

Finnish

Finland

commented 19 January 2022 at 09:02

Issue tags:

+Needs change record

Comment #7

alexpott

he/they

English

🇪🇺🌍

commented 19 January 2022 at 09:39

Title:

Allow themes a way to install newly-required dependencies

» Extend post update system to provide themes a way to install newly-required dependencies

Comment #8

alexpott

he/they

English

🇪🇺🌍

commented 24 January 2022 at 15:28

Issue summary:

View changes

Comment #9

mherchel

he/him

English

Gainesville, FL, US

commented 7 March 2022 at 22:38

This is a blocker for #3257274: Implement color changing theme settings for Olivero

Comment #10

bbrala

Netherlands

commented 11 March 2022 at 09:00

Issue tags:

+blocker

Comment #11

chr.fritsch

he/him

German

🇩🇪🇪🇺🌍

commented 15 March 2022 at 14:24

Status:	Needs review	» Reviewed & tested by the community
Issue tags:	-Needs change record

This looks pretty good. I tested it in the gin theme and all my updates ran. Since this is needed by Olivero and would also be lovely for gin, I would like to this going.

One sidenote: I first tried to run the update with drush, but that didn't work. It discovered the new update hook, but was not able to run it. After that I used update.php and everything worked well. So I think we have to open a follow-up in the drush issue queue.

Comment #12

18 March 2022 at 03:50

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

Comment #13

dww

we/he/they

commented 18 March 2022 at 03:52

"Merge blocked: the source branch must be rebased onto the target branch." -> rebased.
I opened an MR thread about deprecation in 9.4.x and immediate removal in 10.0.x. I didn't think that was allowed. 😉
I closely reviewed all the code, but ran out of time at the tests. The above MR thread is all I noticed.

Leaving at RTBC, but would love clarification on the deprecation strategy.

Thanks!
-Derek

Comment #14

dww

we/he/they

commented 18 March 2022 at 03:59

p.s. I was going to ask what's up with the 2 seemingly duplicate CRs, but the commits on the MR say:

Split out CRs so the more important theme supporting post update functions does not contain superfluous info

More closely reading the actual CRs and it's clear why they're different. Posting here in case anyone else was confused by this like I was. 😅

Comment #15

catch

he/him

English

commented 8 April 2022 at 08:44

Issue tags:

+Needs follow-up

MR looks ready. The one remaining issue is we currently don't currently have any answer to hook_removed_post_updates() endlessly growing, and once we do, we'll need to apply that to themes too, and one of the possible ways we came up with for that was a hook_update_N(), and nooooo. But... we will just need to find a way that does not involve hook_update_N(), there are other problems with that anyway. Can't find the issue that discusses this, but original is here: #3066801: Add hook_removed_post_updates().

Looks like we still need to open an issue against drush to add support per #3259188-11: Extend post update system to provide themes a way to install newly-required dependencies.

Comment #16

alexpott

he/they

English

🇪🇺🌍

commented 8 April 2022 at 12:17

Status:

Reviewed & tested by the community

» Needs review

While working on the Drush fix I noticed that we needed some more changes in update.inc. Specifically...

diff --git a/core/includes/update.inc b/core/includes/update.inc
index 22caf29242..2f0f298e99 100644
--- a/core/includes/update.inc
+++ b/core/includes/update.inc
@@ -226,8 +226,10 @@ function update_invoke_post_update($function, &$context) {
     return;
   }
 
-  [$module, $name] = explode('_post_update_', $function, 2);
-  \Drupal::moduleHandler()->loadInclude($module, 'php', $module . '.post_update');
+  // Ensure extension post update code is loaded.
+  [$extension, $name] = explode('_post_update_', $function, 2);
+  \Drupal::service('update.post_update_registry')->getUpdateFunctions($extension);
+
   if (function_exists($function)) {
     try {
       $ret['results']['query'] = $function($context['sandbox']);
@@ -257,17 +259,17 @@ function update_invoke_post_update($function, &$context) {
     $context['finished'] = $context['sandbox']['#finished'];
     unset($context['sandbox']['#finished']);
   }
-  if (!isset($context['results'][$module][$name])) {
-    $context['results'][$module][$name] = [];
+  if (!isset($context['results'][$extension][$name])) {
+    $context['results'][$extension][$name] = [];
   }
-  $context['results'][$module][$name] = array_merge($context['results'][$module][$name], $ret);
+  $context['results'][$extension][$name] = array_merge($context['results'][$extension][$name], $ret);
 
   if (!empty($ret['#abort'])) {
     // Record this function in the list of updates that were aborted.
     $context['results']['#abort'][] = $function;
   }
 
-  $context['message'] = t('Post updating @module', ['@module' => $module]);
+  $context['message'] = t('Post updating @extension', ['@extension' => $extension]);
 }
 
 /**

This code is copied in Drush. Nothing was broken in core because the theme post update function was already loaded. By changing \Drupal::moduleHandler()->loadInclude($module, 'php', $module . '.post_update'); to \Drupal::service('update.post_update_registry')->getUpdateFunctions($extension); ensure the post update function file is loaded. Tests were not failing because the batch is immediately processed in the same request that has create the batch by listing all the available update functions.

Comment #17

dww

we/he/they

commented 9 April 2022 at 04:35

@alexpott re: #16: Thanks for catching that! It's unfortunate that you squashed all the commits in the MR branch as part of your rebase, since now it's a little harder to review what you actually changed. Thankfully, Gitlab remembers it as commit 78c7ca08, and that all looks great.

It sounds like you're already working on the drush issue. Would you be willing to add the link here so we can remove the "Needs follow-up" tag?

I almost moved this back to RTBC, but I'm also wondering if we want to try to harden the tests any more, per the end of your comment.

I'm also not sure if I should attempt that myself, or retain my reviewer status in here. 😉 Please advise.

Thanks!
-Derek

Comment #18

dww

we/he/they

commented 9 April 2022 at 04:51

Issue summary:	View changes
Status:	Needs review	» Reviewed & tested by the community
Issue tags:	-Needs follow-up

Found https://github.com/drush-ops/drush/pull/5120 and added to the summary.

At this point, I think RTBC is safe. The committer can decide to push-back for more thorough tests if they deem it necessary and worth holding up this blocker for.

Thanks again!
-Derek

Comment #19

alexpott

he/they

English

🇪🇺🌍

commented 9 April 2022 at 05:06

I think it is quite hard to test this because we'd need an update that has a sleep in it to force batching.

Re the squash - sorry but for whatever reason the MR would no longer merge 10.x cleanly at all. There were conflicts in pieces of code completely untouched by the change so I just obliterated everything and started again. Hence pasting what I changed. Thanks for posting the link to the Drush work.

Comment #20

dww

we/he/they

commented 9 April 2022 at 06:09

Sounds great, thanks. Then RTBC it is! 🎉

Comment #21

9 April 2022 at 17:12

catch committed 5f5c811 on 10.0.x

Issue #3259188 by alexpott, dww, chr.fritsch: Extend post update system...

Comment #22

9 April 2022 at 17:12

catch committed 305d120 on 9.4.x

Issue #3259188 by alexpott, dww, chr.fritsch: Extend post update system...

Comment #23

catch

he/him

English

commented 9 April 2022 at 17:13

Status:

Reviewed & tested by the community

» Fixed

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

It'd be good to have the more expansive test coverage however:

1. Agreed it seems hard and potentially unreliably to trigger the batch
2. The easiest way to trigger the batch will be to have a real post update and real updates giving us implicit test coverage - we'll probably get that with Olivero + a few core updates over the next months.