Add hook_removed_post_updates()

Rerolled for latest 8.9.x.

... sigh ...

Rerolled for latest 8.9.x (this time!)

See #3097661: No hook_update_last_removed() equivalent for post updates for a different approach that actively removes registered post updates.

Marked #3097661: No hook_update_last_removed() equivalent for post updates as duplicate. Bumping this to critical.

Interdiff shows the main thing I think we're missing from here. We also need to add test coverage for that working.

Haven't done a line by line review of the patch yet, just looked for this specific detail and couldn't find it.

Errr array_merge().

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,34 @@ function hook_post_update_NAME(&$sandbox) {
   + * @see hook_post_update_NAME()
   + */
   +function hook_removed_post_updates() {
   +  // Example list of names to now disallow in hook_post_update_NAME() functions.
   +  return [
   +    'foo' => TRUE,
   +    'bar' => TRUE,
   +    'baz' => TRUE,
   +  ];
   +}
   

   This should either be an array of actual function names, or just an array of function suffixes. Having the name as key with values as TRUE does not seem to be adding anything.

2. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -86,6 +86,20 @@ public function __construct($root, $site_path, array $enabled_modules, KeyValueS
    
   +  /**
   +   * Gets all names removed from a module's post_update scope.
   +   *
   +   * @return string[]
   +   *   A list of names not allowed in module's post_update hooks.
   +   */
   +  protected function getRemovedPostUpdateFunctionsForModule($module) {
   +    $function = "{$module}_removed_post_updates";
   

   Regardless of what we do with the hook return value, this should be returning function names - as the method name implies.

Once we settle on an approach, this will need a UX review. The current message is not very clear.

Piecemeal but here's a bit more.

And a little bit more.

Updated the hook return value and docs.

Updated the module install/uninstall test to take into account update_test_postupdate_removed_post_updates() getting merged with the actual updates it provides.

Definitely still @todo:

Need to add a system_requirements() following the structure of the one added in #3098475: Add more strict checking of hook_update_last_removed() and better explanation (for contrib modules - don't think we need to special case core for this since that'll never actually get used). + test coverage for that.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -298,7 +298,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   -        $post_update_registry->registerInvokedUpdates($post_update_registry->getModuleUpdateFunctions($module));
   +        $post_update_registry->registerInvokedUpdates(array_merge($post_update_registry->getModuleUpdateFunctions($module), $post_update_registry->getRemovedPostUpdateFunctionsForModule($module)));
   

   This confused me at first, because I expected the removed ones to be removed from the list (via something like array_diff_key) but as @catch clarified in slack, the idea is to keep the list the same whether they were removed or not. While that makes sense, I think it deserves a comment inline

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,30 @@ function hook_post_update_NAME(&$sandbox) {
   + * This can be used to indicate post update functions that have existed in
   

   s/can/should, IMO

3. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,30 @@ function hook_post_update_NAME(&$sandbox) {
   +function hook_removed_post_updates() {
   ...
   +    'foo',
   +    'bar',
   +    'baz',
   

   Fine with push-back on this, but I think they should be the full function name here, and split out programmatically later, instead of combined later. This would help with devs searching the codebase to see where that one thing went.

4. +++ b/core/modules/system/tests/modules/update_test_postupdate/update_test_postupdate.post_update.php
   @@ -67,3 +67,14 @@ function update_test_postupdate_post_update_test_batch(&$sandbox = NULL) {
   +  return [
   +    'foo' => TRUE,
   +    'bar' => TRUE,
   +    'baz' => TRUE,
   +  ];
   

   These are still backwards from the last iteration of the patch
   EDIT: cross-post

5. +++ b/core/modules/system/tests/src/Functional/Update/UpdateRemovedPostUpdateTest.php
   @@ -0,0 +1,67 @@
   +    if (!$this->zlibInstalled) {
   +      $this->fail('Missing zlib requirement for update tests.');
   +      return FALSE;
   +    }
   

   Not seen this property used outside the base class, what's up?

Fine with push-back on this, but I think they should be the full function name here

I think that's a good idea too - it'll also make the docs much easier to read and write, I tried to adjust the wording already but just dealing with function names is a lot clearer.

#24.5 that is odd I think we should just remove the hunk.

Addressing #24 I think.

Haven't done a proper test run locally yet so this is a bit hit-and-hope.

Still need system_requirements() additions + test coverage.

I'm done for today so free for anyone to take over.

Implemented the system_requirements() and fixed up the test coverage a bit.

The system_requirements() directly follows hook_update_last_removed() checks, and decided for now to only show removed post update errors if there isn't an error for hook_update_last_removed() - this is on the basis that most modules will be removing updates and post updates together, and we only really want to show one error in that case, not two.

Just fixing some weird indentation issues, no interdiff.

Fixing the test fails in UpdatePostUpdateTest.

Apparently not, this one should do it.

Self-review:

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,30 @@ function hook_post_update_NAME(&$sandbox) {
    
   +/**
   + * Return an array of removed hook_post_update_NAME() names.
   + *
   + * This should be used to indicate post update functions that have existed in
   + * some previous version of the module, but are no longer available.
   + *
   + * This implementation has to be placed in a MODULE.post_update.php file.
   + *
   + * @return string[]
   + *   An array where the values are post update names that have been removed.
   + *
   

   s/name/function names/

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,30 @@ function hook_post_update_NAME(&$sandbox) {
   +  // Example list of names to now disallow in hook_post_update_NAME() functions.
   

   Remove this comment.

3. +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,43 @@
   + *
   + * Occurs when a module defines hook_post_update_NAME() that when NAME also
   + * exists in the array returned by hook_removed_post_updates().
   + */
   

   'Occurs when a module defines hook_post_update_NAME() that are also specified as having been removed.

4. +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,43 @@
   +   * List of invalid hook_post_update_NAME functions.
   

   Missing parentheses.

5. +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,43 @@
   +  /**
   +   * Retrieves a list of invalid hook_post_update_NAME functions.
   +   *
   +   * @return string[]
   +   *   List of invalid hook_post_update_NAME functions.
   +   */
   

   missing parentheses.

6. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -86,6 +86,21 @@ public function __construct($root, $site_path, array $enabled_modules, KeyValueS
   +   */
   +  public function getRemovedPostUpdateFunctionsForModule($module) {
   +    $this->scanExtensionsAndLoadUpdateFiles();
   

   Could this be renamed to getRemovedPostUpdates($module)?

EDIT: cross-post

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -295,10 +295,13 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        // Ensure that all post_update functions are registered already. This
   +        // should include existing post updates, as well as any specified as
   +        // having been previously removed, to ensure that newly installed and
   +        // updated sites have the same entries in the registry.
   

   +1, thanks

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,30 @@ function hook_post_update_NAME(&$sandbox) {
   +  // Example list of names to now disallow in hook_post_update_NAME() functions.
   

   Due to the switch from the NAME to the whole function name, this should be updated from
   "in hook_post_update_NAME() functions"
   to
   "as hook_post_update_NAME() function names"

3. +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,43 @@
   + * An exception thrown for removed post update errors.
   

   "removed post update errors" -> removed post update functions"?

4. +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,43 @@
   + * Occurs when a module defines hook_post_update_NAME() that when NAME also
   + * exists in the array returned by hook_removed_post_updates().
   

   Also needs to be updated now that we return full function names.

5. +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,43 @@
   +  public function __construct(array $invalid_updates, string $message = '') {
   +    parent::__construct($message);
   

   When extending \Exception, I usually include the optional constructor params and still pass them on (mostly only for $previous). See \Drupal\Core\ParamConverter\ParamNotConvertedException for an example. Not important, but figured I'd call it out anyway.

6. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -273,7 +274,19 @@ protected function selection(Request $request) {
   +              '#prefix' => '<div class="messages messages--warning">',
   

   I was going to call this out as odd, as it is only done in one other place in core. But that other place is this same class! So it's fine :)

Everything else looks good to me. Still needs a couple other signoffs from others, but IMO it's RTBC-able after the above fixes.

Should resolve #35 + #36.

Removing the subsystem maintainer review tag because:

Database Update API
- ?

We usually do this in the absence of a subsystem maintainer, especially for a low-level API.

...Although this API is simple enough that that might not really be needed, especially since a framework manager wrote the patch.

The tag was actually also added just to evaluate the original prototype, which @catch pretty much did by picking up the patch, so re-un-tagging.

Mostly this looks extremely solid. I started a thorough review. These are all just questions/concerns, nothing worthy of NW per se.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -295,10 +295,13 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        $post_update_registry->registerInvokedUpdates(array_merge($post_update_registry->getModuleUpdateFunctions($module), $post_update_registry->getRemovedPostUpdates($module)));
   

   I guess our official coding standards don't approve, but I'd rather this was wrapped onto multiple lines.

2. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -97,15 +112,26 @@ protected function getAvailableUpdateFunctions() {
   +          $removed = $this->updateType == 'post_update' ? $this->getRemovedPostUpdates($matches['module']) : [];
   

   Do we really want to call getRemovedPostUpdates() repeatedly inside a loop like this? It doesn't appear to do any caching of its own. Seems like a lot of churn that could be avoided, no?

3. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -273,7 +274,19 @@ protected function selection(Request $request) {
   +              '#title' => $this->t('Restricted hook_post_update_NAME usage'),
   

   I'm not totally clear where this title is appearing, but "Restricted hook_post_update_NAME usage" seems like a fairly cryptic error label to show to site admins. Can this be more human-understandable?

4. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -273,7 +274,19 @@ protected function selection(Request $request) {
   +              '#markup' => '<ul><li>' . implode('()</li><li>', $invalid_updates) . '()</li></ul>',
   

   Do we want to do this <ul> manually as #markup, or should we use #type => 'list' (or whatever it is)?

5. +++ b/core/modules/system/system.install
   @@ -1133,6 +1133,27 @@ function system_requirements($phase) {
   +        if ($missing_updates = array_diff($removed_post_updates, $existing_updates)) {
   

   I know there's a big (unresolved) debate about this somewhere, but assignments inside if generally scare me. Can we do this on 2 lines? Then it wouldn't exceed 80 chars, either.

6. +++ b/core/modules/system/system.install
   @@ -1133,6 +1133,27 @@ function system_requirements($phase) {
   +            'description' => t('The installed version of the %module module is too old to update. Update to an intermediate version first (missing_updates: @missing_updates).', [
   

   Also concerned about the UI of this. How is a site admin supposed to use the list of missing updates to find the right "intermediate version" to update to? I know we can't actually tell them. And d.o release nodes don't help, either. I doubt reading release notes (which "no one" does) would help, either. I know this is a (hopefully) rare error condition, but you have to basically do a git bisect to figure this out for yourself, which is probably something that <0.1% of site builders would know how to do. :/

7. +++ b/core/modules/system/system.install
   @@ -1133,6 +1133,27 @@ function system_requirements($phase) {
   +              '@missing_updates' => implode(', ', $missing_updates),
   

   Above we spent effort to append "()" to the function names. Do we want the same here?

8. +++ b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
   @@ -300,7 +300,7 @@ protected function assertInstallModuleUpdates($module) {
   +        $this->assertEmpty(array_diff(['update_test_postupdate_post_update_first', 'update_test_postupdate_post_update_second', 'update_test_postupdate_post_update_test1', 'update_test_postupdate_post_update_test0', 'update_test_postupdate_post_update_foo', 'update_test_postupdate_post_update_bar', 'update_test_postupdate_post_update_baz'], $existing_updates));
   

   Can we wrap this to multiple lines now that it's about 500 chars wide? ;)

9. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePostUpdateTest.php
   @@ -54,6 +54,18 @@ protected function setUp() {
   +    // Mimic the behaviour of ModuleInstaller::install() for removed post
   +    // updates. Don't include the actual post updates because we want them to
   +    // run.
   +    $key_value = \Drupal::service('keyvalue');
   +    $existing_updates = $key_value->get('post_updates')->get('existing_updates', []);
   +    $post_updates = [
   +      'update_test_postupdate_post_update_foo',
   +      'update_test_postupdate_post_update_bar',
   +      'update_test_postupdate_post_update_baz',
   +    ];
   

   To keep this list in sync with the test module, can we just invoke update_test_postupdate_removed_post_updates() directly here and use that?

10. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePostUpdateTest.php
    index 0000000000..f6e4f70df9
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateRemovedPostUpdateTest.php
    

    Ran out of time/steam for the review at this spot. Will try to resume tomorrow... Sorry I didn't make it to the end of the patch!

Thanks,
-Derek

I'm not totally clear where this title is appearing, but "Restricted hook_post_update_NAME usage" seems like a fairly cryptic error label to show to site admins.

This should only really show to a module developer (or in the case where a module developer has done literally zero testing). It only happens when someone adds a post update to the hook but also doesn't remove the post update, or adds a duplicate function name later by mistake. To be honest if I was working on this from scratch, I would not even have thought about adding this exception, but also not sure what a better wording would be.

One option would be to just not catch the exception at all, that would also remove some of the custom markup and make the patch a bit smaller.

Also concerned about the UI of this. How is a site admin supposed to use the list of missing updates to find the right "intermediate version" to update to? I know we can't actually tell them. And d.o release nodes don't help, either. I doubt reading release notes (which "no one" does) would help, either. I know this is a (hopefully) rare error condition, but you have to basically do a git bisect to figure this out for yourself, which is probably something that <0.1% of site builders would know how to do.

So this is exactly the same problem as with hook_update_last_removed() which we've had for years (with worse UX for the errors until recently).

You will never see this message for core post updates, because we're always going to be removing them alongside hook_update_N(), and because we special case core since #3098475: Add more strict checking of hook_update_last_removed() and better explanation to show a core-specific message where we know the exact versions we're dealing with.

This means it will only be shown when a contrib module implements the hook, and then someone tries to update from a very old version of that contrib module to the version where the post updates were removed. However, without this hook, we'd be allowing the update to proceed - possibly running new updates that depend on old missing ones and exploding during the update, or leaving a site with fatal errors or data loss once the update has run 'successfully' (and no way to recover without restoring a backup).

So the ideal process that someone goes to.

1. Try to update from 8.x-1.0 to 8.x-3.0 and get this message.
2. Downgrade the contrib module back to the version that was installed originally.
3. Go to the project page where ideally there's a note from the project maintainer (or an issue in the queue) saying upgrade to 8.x-1.4 before you update to 8.x-3.0
4. Upgrade to 8.x-1.4
5. Upgrade to 8.x-3.0

To address the which version do I need question so people don't have to do a git bisect we could add the information to the hook.

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -763,6 +763,29 @@ function hook_post_update_NAME(&$sandbox) {
+function hook_removed_post_updates() {
+  return [
+    'mymodule_post_update_foo',
+    'mymodule_post_update_bar',
+    'mymodule_post_update_baz',
+  ];
+}

So instead of this we could do:

function hook_removed_post_updates() {
  return [
    // The values are the last version the update was in.
    'mymodule_post_update_foo' => '8.x-1.1',
    'mymodule_post_update_bar' => '8.x-1.1',
    'mymodule_post_update_baz' => '8.x-2.1',,
  ];
}

Or even

function hook_removed_post_updates() {
  return [
    // The keys are the last version the update was in.
    '8.x-1.1' => [
      'mymodule_post_update_foo',
      'mymodule_post_update_bar',
    ],
    '8.x-2.1' => ['mymodule_post_update_baz'],
  ];
}

 // The values are the last version the update was in.
    'mymodule_post_update_foo' => '8.x-1.1',
    'mymodule_post_update_bar' => '8.x-1.1',
    'mymodule_post_update_baz' => '8.x-2.1',,

We don't know this information though.

For core, we're removing the updates in 9.0.0.

This means the last version they'll be in will be all the future releases of 8.8.x, then 8.9.x once it's released.

The closest we could do is branch, but we can't even 100% rule out an 8.10.x branch (a completely unexpected third party dependency update to resolve a critical security issue in a year or something).

We can tell them when it was removed though, and say they need to roll back to a version before that?

Yes that would be possible, it'd look like this:

function hook_removed_post_updates() {
  return [
    // The values are the version the update was removed in.
    'mymodule_post_update_foo' => '8.x-3.0',
    'mymodule_post_update_bar' => '8.x-3.0',
    'mymodule_post_update_baz' => '8.x-3.0',
  ];
}

This approach would also work for hook_update_last_removed() except that it'd be an API change there (or require handling both scalar and array return types).

Should address #41.3, #41.4, and #41.8

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -295,10 +295,13 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   -        $post_update_registry->registerInvokedUpdates($post_update_registry->getModuleUpdateFunctions($module));
   

   Is it worth throwing an exception here if there is a duplicate in
   $post_update_registry->getModuleUpdateFunctions($module) and $post_update_registry->getRemovedPostUpdateFunctionsForModule($module)?

   Ah see below...

2. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -97,15 +112,26 @@ protected function getAvailableUpdateFunctions() {
   +          $removed = $this->updateType == 'post_update' ? $this->getRemovedPostUpdateFunctionsForModule($matches['module']) : [];
   +          $function_name = $matches['module'] . '_' . $this->updateType . '_' . $matches['name'];
   +          if (array_search($function_name, $removed) === FALSE) {
   +            $updates[] = $function_name;
   +          }
   +          else {
   +            $invalid_updates[] = $function_name;
   +          }
            }
          }
        }
   +    if (!empty($invalid_updates)) {
   +      throw new RemovedPostUpdateNameException($invalid_updates);
   +    }
   

   Ah so we're doing that here. I wonder if the complexity of gathering all the invalid updates here is necessary. Perhaps we could throw on the first one. Since I think that that this will also cause errors on module install too. So even the most rudimentary test coverage - like is the module installable will pick this up. I think this is also a reason that catching the exception and turning it into a translated error is unnecessary.

I have to apologize: when I did the usability review for #3098475: Add more strict checking of hook_update_last_removed() and better explanation, I looked at the error message for core, but the one for contrib modules was an afterthought. In my defense, there was not a screenshot of the contrib version when I started my review.

+1 for finding a way to give the site owner more specific information about which intermediate version to use. Since we are adding a new hook here, we can do something like the suggestion in #43.

Once we figure out the best approach here, let's add a follow-up issue to make similar changes to hook_update_last_removed(). I am adding an issue tag for that and adding an item to the Remaining tasks.

Let's also consider documentation improvements that could go along with the new hook. For example, the doc block for the new hook (and therefore the generated page on a.d.o) could recommend that the implementer add a notice to the release notes (which, I am told, "no one" ever reads) or the project page. The core message from #3098475 has a link to Upgrading a Drupal 8 site to Drupal 9. Can we find a good place to add a page where we can give site owners information on how to handle this error message? In general, maintaining a Drupal site is a lot harder than installing it initially; if we don not already have a guide for maintaining/updating a Drupal site, then we should add one.

Instead of (or as well as?) the removed version number, should we reference a URL from hook_removed_post_updates() where modules can provide further information about the removal? This could be a documentation page, release note or change record, as appropriate?

This implements #46. Updating the screenshot too.

Also resolves #48.

+++ b/core/modules/system/system.install
@@ -1142,12 +1142,14 @@ function system_requirements($phase) {
               '%module' => $module_info->info['name'],
               '@missing_updates' => implode(', ', $missing_updates),
+              '@version' => implode(', ', array_unique(array_intersect_key($removed_post_updates, array_flip($missing_updates)))),
             ]),

It's extremely unlikely anyone would ever get a version of the message with multiple versions of the same module, but if they ever do, this is what it'll look like. I don't really relish the idea of trying to figure out the lowest valid version string here since we have to deal with both core and contrib version strings, and potentially current contrib and future contrib semver strings.

Just realised I cross-posted with #50, personally I think the version number is enough here. If you get to this error message, then you've just updated a module - which means you either used composer update, or you went to the project page anyway to download it.

If we want to do something like this, it would be better to store information about the project page for modules somewhere, then we could generically link to it, rather than a one-off in this hook.

Here's the follow-up: #3117789: Allow hook_update_last_removed() to provide version information.

Thanks for all the responses and ideas since #41! I like where this is headed. @catch posted #3117789 so removing the 'needs followup' tag.

Re: #50: Unless/until #605538: Refactor releases to be anticipatory, not just post hoc is ever done, by the time a maintainer is committing the changes to this hook, there is no release node ID they can link to. So that's definitely not possible.

Generally, I agree with #52 that a release version number is sufficient, instead of trying to include a link via this hook.

However, I don't think this would work, either:

If we want to do something like this, it would be better to store information about the project page for modules somewhere, then we could generically link to it, rather than a one-off in this hook.

Not all modules have project pages, and not all modules even live on d.o. Some things live on github, gitlab, are custom modules that live in some private repo, etc. So we can't assume that there's a project page, and there's no way to generically link to anything. I vote for spitting out version numbers and letting the site admin figure it out. They hopefully know where they got their modules from in the first place. ;) Or at least their composer.json knows, or their drush make file, or whatever they're using. Saying (approx): "you have to upgrade to the last version below X, then update to this." is still vastly more user-friendly than "you have to upgrade to a version that still includes post_update_foo()."

Re: #47 interdiff looks good, thanks. One more minor thing about #41.8 I should have mentioned in the first place:

+++ b/core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php
@@ -300,7 +300,15 @@ protected function assertInstallModuleUpdates($module) {
+        $this->assertEmpty(array_diff([
...

Instead of this, why not:

$expected_updates = [
  ...
];
$this->assertSame($expected_updates, $existing_updates);

? That way, if they don't match, the test failure will show what's different, instead of just complaining that something isn't empty.

I'll try to finish reviewing later today.

Thanks again,
-Derek

Addressing #55.

Minor update to the API docs following discussion with @dww on #3106666: Remove post updates added prior to 8.8.0 (which now has a patch using the API added here).

Fixing a typo found via #3106666: Remove post updates added prior to 8.8.0.

One typo didn't get fixed...

We reviewed this on this week's UX meeting (video link forthcoming) and based on the wording on the screenshot at #52, arrived at:

"The installed version of $module module is too old to update to version $desired_version. Update to $minimum_version first."

Ok, I see we (or at least I :)) got horribly turned around with #52. After talking more with @catch, it was determined:

1) We don't/can't know $minimum_version. We can only know what version the update was last removed in (hence the "prior" wording in the issue summary's screenshot)

2) #52 will only happen in an extreme edge case, for "a contrib module that is only using hook_post_update_NAME() for two major releases without adding/removing a single hook_update_N()" (per @catch) (in other words, it should basically never come up in the field in practice).

That said, #52 is wrong, even just grammatically. It really should be just a single version: $minimum_version. But that requires writing some complex logic to reconcile and reduce version numbers across core/contrib/semver contrib, and not the kind of thing we need to hold up a critical, beta-blocking task on.

So, +1 to the screenshot in the issue summary. And we can have a minor follow-up for the "edge case" wording to help that make sense.

Follow-up here: #3118119: Improve multiple removed version message for removed post updates.

Oh wait. BUT. Could we please include the bit from #3098475-94: Add more strict checking of hook_update_last_removed() and better explanation and include a link to the project page (for contrib) in the message?

We don't have any way in core to get the project page for a module - it's not in .info.yml. So that would require adding a 'homepage' key or similar for modules to be able to point to. That's quite a big change that I also think we should do in a follow-up.

It would be possible to squeeze a link into the hook output somehow, but if we're only going to link to the project page (if it's available) that seems like the wrong place to put that information.

Meh. Ok. :P hehe

Added another follow-up #3118128: Add 'homepage' to .info.yml.

A few minor nits and questions.

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,30 @@ function hook_post_update_NAME(&$sandbox) {
   + * Return an array of removed hook_post_update_NAME() function names.
   

   Returns

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,30 @@ function hook_post_update_NAME(&$sandbox) {
   + * @return array
   

   @return string[]

3. +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,12 @@
   +class RemovedPostUpdateNameException extends \Exception {
   

   I think this should extend \LogicException?

4. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -102,11 +117,18 @@ protected function getAvailableUpdateFunctions() {
   -          $updates[] = $matches['module'] . '_' . $this->updateType . '_' . $matches['name'];
   +          $removed = $this->updateType == 'post_update' ? array_keys($this->getRemovedPostUpdates($matches['module'])) : [];
   +          $function_name = $matches['module'] . '_' . $this->updateType . '_' . $matches['name'];
   +          if (array_search($function_name, $removed) === FALSE) {
   +            $updates[] = $function_name;
   +          }
   +          else {
   +            $message = 'The following update is  specified as removed in hook_removed_post_updates() but still exists in the code base: ' . $function_name;
   +            throw new RemovedPostUpdateNameException($message);
   +          }
   

   I think we can improve the logic here a bit now we don't to collect all the wrong updates.

   Something like

             $function_name = $matches['module'] . '_' . $this->updateType . '_' . $matches['name'];
             if ($this->updateType == 'post_update') {
               $removed = array_keys($this->getRemovedPostUpdates($matches['module']));
               if (array_search($function_name, $removed) !== FALSE) {
                 $message = 'The following update is specified as removed in hook_removed_post_updates() but still exists in the code base: ' . $function_name;
                 throw new RemovedPostUpdateNameException($message);
               }
             }
             $updates[] = $function_name;
   

5. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -102,11 +117,18 @@ protected function getAvailableUpdateFunctions() {
   +            $message = 'The following update is  specified as removed in hook_removed_post_updates() but still exists in the code base: ' . $function_name;
   

   Some extra space in the message. between is and specified

6. +++ b/core/modules/system/system.install
   @@ -1133,6 +1133,28 @@ function system_requirements($phase) {
   +      $post_update_registry = \Drupal::service('update.post_update_registry');
   ...
   +        $module_info = \Drupal::service('extension.list.module')->get($module);
   

   We're getting the post update registry outside the loop but we're getting the extension.list.module service in it. I'm really sure that the optimisations really matter fwiw but it is inconsistent.

7. +++ b/core/modules/system/system.install
   @@ -1133,6 +1133,28 @@ function system_requirements($phase) {
   +            'description' => t('The installed version of the %module module is too old to update. Update to a version prior to @version first (missing_updates: @missing_updates).', [
   +              '%module' => $module_info->info['name'],
   +              '@missing_updates' => implode(', ', $missing_updates),
   +              '@version' => implode(', ', array_unique(array_intersect_key($removed_post_updates, array_flip($missing_updates)))),
   +            ]),
   
   +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateRemovedPostUpdateTest.php
   @@ -0,0 +1,92 @@
   +    $assert_session->pageTextContains('The installed version of the Update test after module is too old to update');
   

   It'd be good to test the version string is outputted correctly.

8. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateRemovedPostUpdateTest.php
   @@ -0,0 +1,92 @@
   +use Drupal\Component\Render\FormattableMarkup;
   

   Not used.

9. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateRemovedPostUpdateTest.php
   @@ -0,0 +1,92 @@
   + * Tests hook_post_update().
   ...
   +   * Tests hook_post_update_NAME().
   

   Tests hook_removed_post_updates().

   I guess?

We don't have any way in core to get the project page for a module

Would it be possible to parse the composer.json file and look for the “homepage” key?

"homepage": "https://drupal.org/project/webform",

or

"homepage": "https://www.drupal.org/project/drupal",

It might also be that it is a custom or site-specific module, which would mean that there is no homepage to refer to. Definitely out of scope here.

Re: #64 and #69: Indeed, see #55.

Oh, meant to say, I do think the "a version prior to 8.9.0, 8.4.0" wording is quite WTF-y and I am not sure I would descope fixing that to a followup. Edit: It should just be the minimum of those from version_compare()?

The reason I'm resistant to version_compare() is due to upcoming contrib semver.

version_compare('8.x-1.0', '3.0.0'); returns 1 which treats 3.0.0 as the lower version.

That means we'd need to normalise contrib version numbers down to standard semver version numbers, which I don't think we have an API for. So either adding an API before we can commit this issue, or doing the normalisation in system_requirements(), both of which feel a bit unwieldy.

Can we at least come up with a wording that's less ridiculous for multiple values? Like:

Update first to a version prior to all of the following:

• 8.9.0
• 8.4.0

Then it's less silly and we can deal with a semver-aware version compare if we want in a followup.

#74 sounds good.

Nw for #68 and #74.

This should cover all the nits in #68.

I changed the message for multiple versions to the one in #74, that does look better (uploading a screenshot from stark, courtesy of the test output).

Also adding test coverage for the multiple version case, and switched to using contrib-style version numbers of the test hook implementation because it feels a bit more readable what's going on given core is removing all its updates in a single version.

Also fixing a copypasta comment in the test.

@tim.plunkett is going to provide a review here. Thanks!

I am updating the Remaining tasks:

• We have a draft change record, so I am changing "Needs CR" to "Publish CR". Maybe we can just delete that item.
• The follow-up I requested in #49 was added in #53, and the issue tag was removed in #55. I am deleting this item.

I think the issue summary needs an update, so I am adding the tag for that. The screenshot in #52 is different from the one in the summary. Are both possible in different situations? Please explain this in detail in the "User interface changes" section.

In #49, I made two suggestions that I still think are good ideas:

1. Add a link to a (new?) documentation page to the error message.
2. Expand the doc block to say that contrib modules should document the upgrade path on their project pages. Maybe add another documentation page and link to it.

Added the second screenshot to the issue summary.

Mostly nits.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -295,10 +295,13 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        // should include existing post updates, as well as any specified as
   
   +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,12 @@
   + * An exception thrown for removed post update functions.
   

   Nit: "post-update" should probably be hyphenated everywhere that it's used in text (rather than having a space).

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -744,7 +744,7 @@ function hook_update_N(&$sandbox) {
   - * @see hook_update_N()
   + * @see hook_removed_post_updates()
   

   Why remove the @see to hook_upadte_N()? That's also still something they should read if they're deciding what kind of update to write.

3. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +763,30 @@ function hook_post_update_NAME(&$sandbox) {
   +function hook_removed_post_updates() {
   +  return [
   +    'mymodule_post_update_foo' => '8.x-3.0',
   +    'mymodule_post_update_bar' => '8.x-3.0',
   +    'mymodule_post_update_baz' => '8.x-3.0',
   +  ];
   

   Should we also include a semver pattern in the example for forward-compatibility of these docs? It'd be a good idea anyway to show two different versions.

4. +++ b/core/modules/system/tests/modules/update_test_postupdate/update_test_postupdate.post_update.php
   @@ -67,3 +67,14 @@ function update_test_postupdate_post_update_test_batch(&$sandbox = NULL) {
   +    'update_test_postupdate_post_update_foo' => '8.x-1.0',
   +    'update_test_postupdate_post_update_bar' => '8.x-2.0',
   +    'update_test_postupdate_post_update_baz' => '8.x-3.0',
   

   It'd be good to have a contrib semver pattern here as well to ensure this functionality works with semver.

5. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdateRemovedPostUpdateTest.php
   @@ -0,0 +1,108 @@
   +    // Excludes 'update_test_postupdate_post_update_baz' and
   +    // 'update_test_postupdate_post_update_bar'.
   +    $post_updates = [
   +      'update_test_postupdate_post_update_first',
   +      'update_test_postupdate_post_update_second',
   +      'update_test_postupdate_post_update_test1',
   +      'update_test_postupdate_post_update_test0',
   +      'update_test_postupdate_post_update_foo',
   

   This comment looks wrong until I read the hunk above. We should maybe add:
   "...which are checked by UpdatePostUpdateTest and ModuleInstallUninstallTest."

6. +++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
   @@ -71,6 +78,43 @@ function module_a_post_update_a() {
   +/**
   + * Module C update A.
   + */
   +function module_c_post_update_a() {
   +}
   

   Naming both the modules and the updates with letters A, B, C is confusing. The modules could be letters and the updates could be numbers. Edit: I guess this would be messy to fix and out of scope since there's already module_b_post_update_a()

#82.1. Think I got them.

#82.2. Yes this shouldn't be removed.

#82. 3 and #4 I don't think we should start introducing semver contrib version references until we're actually ready to recommend doing that.

There are very, very few places that we actually reference contrib version numbers in core (at least outside update module) and this isn't the place to begin documenting that you can use them. We also need to document what people need to do with hook_update_N() for semver contrib (since there is a risk of people adding mymodule_update_4000() which will never run after mymodule_update_8300()) - plenty to do there before this starts to look outdated.

#82.5 - bit confused by this - they're checked by UpdatePostUpdateTest because that's the test we're in, we're just 'faking' the module install process to control things like schema version etc (similar to hook_update_N() tests which I based this off). However I added an extra sentence to clarify why we're not including them - any better?

#82. 3 and #4 I don't think we should start introducing semver contrib version references until we're actually ready to recommend doing that.

@tedbow has already begun adding contrib semver tests actually, which is why I'm recommending including both types whenever we make data providers going forward. We're talking about turning on semver for contrib (edit: quietly allowing it, that is, not requiring it) potentially this week.

I think we need a blocking issue for hook_update_N() docs before doing that.

Here's the issue I mentioned. As far as I know it's already supported (the related issue just adds tests); it just has insufficient test coverage and I guess minimal documentation.

Why hook_update_N() specifically, though? I guess to ensure safe/correct update hook numbering? There's this bit:

 * The numbers are normally composed of three parts:                            
 * - 1 or 2 digits for Drupal core compatibility (Drupal 8, 9, 10, etc.). This  
 *   convention must be followed.                                               
 * - 1 digit for your module's major release version; for example, for 8.x-1.*  
 *   use 1, for 8.x-2.* use 2, for Core 8.0.x use 0, and for Core 8.1.x use 1.  
 *   This convention is optional but suggested for clarity.

We shouldn't change the pattern recommendation because the update hooks still need to be in order. I guess we can just expand the text slightly to explain that the Drupal core compatibility is the earliest major version with which the module is compatible (so 8 if it's compatible with both 8.x and 9.x), and that the second digit is your major version number whether it's 8.x-1.* or 1.0.*. I can file a separate issue to do that since it should already have been done previously.

Filed #3118475: Document how contrib hook_update_N() should be numbered now that modules can be compatible with multiple major branches and versioned semantically.

There was an existing duplicate; fixing the reference.

So the more we discuss it, the more I think #3010334: Document how contrib hook_update_N() should be numbered now that modules can be compatible with multiple major branches and versioned semantically is completely unrelated to this issue's test coverage. That one is about a naming convention for update hooks; my suggestions in #82 are about actual version numbers for modules. Core already supports both numbering schemes for contrib version numbers, so we should add tests for both numbering schemes wherever we test contrib version numbers. If it turns out this patch fails with a semver module, that would be critical and beta-blocking.

I don't really object to adding it to the test coverage, but I do object to adding it to the hook_removed_post_updates() documentation while that will be the first place in docs any mention of contrib semver is added. We've theoretically always supported any random version number a contrib module might add (and no version number at all), but it's a different thing once we document something we expect to be used on Drupal.org and we should introduce that consistently.

Here's the change to the test coverage.

That's also related to #3118119: Improve multiple removed version message for removed post updates but added a comment there since we already agreed to look at it in a follow-up.

Unrelated test failure in a build test.

Thought I posted it already but there are other places semver module versions are already mentioned in our API docs, for example Drupal\update\ModuleVersion. It has one change I'd make:

   /**
-   * The '8.x-' prefix is used on contrib module version numbers.
+   * The '8.x-' prefix is used on legacy contrib module version numbers.
    *
    * @var string
    */
   const CORE_PREFIX = '8.x-';
   const CORE_PREFIX = '8.x-'; 
 
   /**
    * The major version.

...but otherwise dicsusses both prefixed and semver module numbers as valid.

Drupal\Component\Version\Constraint also uses a prefixed module version in the API docs, but menions semver strings as well in the inline documentation within a couple methods.

That said, I'm fine with adding that one line of documentation in a followup or adding it to the scope of #3010334: Document how contrib hook_update_N() should be numbered now that modules can be compatible with multiple major branches and versioned semantically if this lands first.

Filed #3118569: Add an example of a semver contrib module number to the update hook example for hook_removed_post_updates().

@catch pointed me to a suggestion in #80 to add a link to this hook's output to some documentation page. For this issue, I think the message is self-explanatory and does not require external documentation.

It's also somewhat out of scope here since the scope of this issue is to implement the equivalent of hook_update_last_removed() for post updates, and I don't think that handbook links any documentation pages either.

The only reason we proposed adding a handbook for the other update-related UX issue is that it's actually fairly complicated to figure out what you are supposed to do in the situation.

I re-reviewed all of the comments and patches and interdiffs since #36 (when I last reviewed), and agreed with the feedback and changes since then.

I'm happy to see this so close, and think it can go in after these minor nits.

The UI messaging is good and signed-off on, the API addition is sound, and the docs and test coverage are sufficient.

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +764,30 @@ function hook_post_update_NAME(&$sandbox) {
   + * Returns an array of removed hook_post_update_NAME() function names.
   

   #68.1 was wrong, hook declarations are written the other way, should be "Return ..."

2. +++ b/core/modules/system/system.install
   @@ -1135,6 +1135,40 @@ function system_requirements($phase) {
   +        if ($missing_updates = array_diff(array_keys($removed_post_updates), $existing_updates)) {
   +          $versions = array_unique(array_intersect_key($removed_post_updates, array_flip($missing_updates)));
   

   Woof, that's a lot of array_* functions called in succession. It works out in the end, of course.

3. +++ b/core/modules/system/tests/modules/update_test_postupdate/update_test_postupdate.post_update.php
   @@ -67,3 +67,14 @@ function update_test_postupdate_post_update_test_batch(&$sandbox = NULL) {
   +    'update_test_postupdate_post_update_foo' => '8.x-1.0',
   +    'update_test_postupdate_post_update_bar' => '8.x-2.0',
   +    'update_test_postupdate_post_update_baz' => '3.0.0',
   

   +1 to the new test scenario

4. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -102,11 +117,18 @@ protected function getAvailableUpdateFunctions() {
   +              $message = 'The following update is specified as removed in hook_removed_post_updates() but still exists in the code base: ' . $function_name;
   +              throw new RemovedPostUpdateNameException($message);
   

   This can be a one-liner, and either way should use sprintf instead of concatenation.

5. +++ b/core/modules/system/system.install
   @@ -1135,6 +1135,40 @@ function system_requirements($phase) {
   +          if (count($versions) === 1) {
   +            $description = t('The installed version of the %module module is too old to update. Update to a version prior to @version first (missing_updates: @missing_updates).', [
   +              '%module' => $module_info->info['name'],
   +              '@missing_updates' => implode(', ', $missing_updates),
   +              '@version' => implode(', ', $versions),
   +            ]);
   +          }
   +          else {
   +            $description = t('The installed version of the %module module is too old to update. Update first to a version prior to all of the following: @versions (missing_updates: @missing_updates)', [
   +              '%module' => $module_info->info['name'],
   +              '@missing_updates' => implode(', ', $missing_updates),
   +              '@versions' => implode(', ', $versions),
   +            ]);
   +          }
   

   The arguments here are identical, just the message is different. Isn't this a job for PluralTranslatableMarkup (aka format_plural)?

6. I think in the last two snippets, both should append () when displaying a function name. I think that change would make it much clearer (in both screenshots above) that these are function names, and not some other magical thing.

In #3052318: Update from 8.6 to 8.7 fails due to corrupt menu_link_content or taxonomy_term entity data we agreed to not append parens because when there are post-updates, the update page looks like this:

In that screenshot of HEAD before the fix, the displayed post-update name doesn't include the module name and, therefore, isn't an existing callable function, so no () should probably be added.

NW to address #98.1, 4, and 5. Thanks!

This can be a one-liner, and either way should use sprintf instead of concatenation.

But are sprintf slower than concatenation? See: Stop the sprintf() abuse old article.

@xmacinfo please open a separate issue about that. There are over 200 usages of sprintf on the same line an exception is thrown (via quick grep)

<rant class="slightly-off-topic">
I don't know why we should insist on continuing this clumsy pattern, just because core already does it a lot. Core also has examples of string concatenation in throw new .... We rarely use sprintf() over string cat in other parts of core. Yes, undoing the clumsy pattern everywhere else is way out of scope in here, but why force this patch to make more work for ourselves later? I fail to see why #98.4 is an improvement, other than consistency with other hard-to-read parts of core. ;) I learned programming in the dark ages of C and sprintf() is totally natural to me. It's just clumsy and harder to read/maintain. Programming languages have moved on in the last 30+ years, so should we. ;)
</rant>

I was going to assign myself to re-roll this to fix the other parts of #98, but @jungle already did that, so I'll try to review as soon as the new patch is up...

Thanks,
-Derek

Besides #98.1, 4, and 5. Following changes made.

1. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -102,11 +117,18 @@ protected function getAvailableUpdateFunctions() {
   +          if ($this->updateType == 'post_update') {
   

   changed == to ===

2. +++ b/core/modules/system/system.install
   @@ -1135,6 +1135,40 @@ function system_requirements($phase) {
   +            $description = t('The installed version of the %module module is too old to update. Update to a version prior to @version first (missing_updates: @missing_updates).', [
   

   changed missing_updates to missing update

3. +++ b/core/modules/system/system.install
   @@ -1135,6 +1135,40 @@ function system_requirements($phase) {
   +            $description = t('The installed version of the %module module is too old to update. Update first to a version prior to all of the following: @versions (missing_updates: @missing_updates)', [
   

   changed missing_updates to missing updates and added a full stop at the end.

Thanks, @jungle!

A few more problems/nits:

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +764,30 @@ function hook_post_update_NAME(&$sandbox) {
   +    'mymodule_post_update_foo' => '8.x-3.0',
   +    'mymodule_post_update_bar' => '8.x-3.0',
   +    'mymodule_post_update_baz' => '8.x-3.0',
   

   I know there's concern about introducing semver examples here, but can we at least have one of these updates from a different version so folks don't think they all have to match for some reason?

2. +++ b/core/lib/Drupal/Core/Update/RemovedPostUpdateNameException.php
   @@ -0,0 +1,12 @@
   + * Occurs when a module defines hook_post_update_NAME() that are also
   + * specified has having been removed in hook_removed_post_updates().
   

   Plural agreement bug: s/that are/that is/ or s/NAME() /NAME() functions/ or something...

3. +++ b/core/modules/system/system.install
   @@ -1135,6 +1136,35 @@ function system_requirements($phase) {
   +        if ($missing_updates = array_diff(array_keys($removed_post_updates), $existing_updates)) {
   

   Still bummed this assignment is inside the if()... /shrug

4. +++ b/core/modules/system/system.install
   @@ -1135,6 +1136,35 @@ function system_requirements($phase) {
   +          $description = new PluralTranslatableMarkup(count($versions),
   +            'The installed version of the %module module is too old to update. Update to a version prior to @versions first (missing update: @missing_updates).',
   +            'The installed version of the %module module is too old to update. Update first to a version prior to all of the following: @versions (missing updates: @missing_updates).',
   +            [
   +              '%module' => $module_info->info['name'],
   +              '@missing_updates' => implode(', ', $missing_updates),
   +              '@versions' => implode(', ', $versions),
   +            ]
   +          );
   

   The conditional thing we're counting is $versions. We can't assume that "missing update" is also singular just because $versions is. In fact, the more common case is going to be that a single version is flagged here, but it removed multiple post update functions.

   Also, not sure why "first" is only used in the plural case, but not in the singular.

   If we really want this to be perfectly accurate, we need an outer if() for count($missing_updates), then use PluralTranslateableMarkup to handle count($versions).

   Or, the simpler approach would be to always assume multiple missing updates, and let PluralTranslateableMarkup just worry about the count of versions...

5. +++ b/core/modules/system/system.install
   @@ -1135,6 +1136,35 @@ function system_requirements($phase) {
   +            'title' => t('Missing updates for: @module', ['@module' => $module_info->info['name']]),
   

   Perhaps out of scope here, but wearing my quasi-unofficial-update-manager-maintainer hat here, the confusion in the UI around the overloaded term "updates" rubs me wrong. "Updates" can mean code updates you're missing (Update manager) or DB/schema updates. Not sure if there's a simple way to phrase this more clearly so it's about DB/schema instead of code... /shrug

6. +++ b/core/modules/system/tests/modules/update_test_postupdate/update_test_postupdate.post_update.php
   @@ -67,3 +67,14 @@ function update_test_postupdate_post_update_test_batch(&$sandbox = NULL) {
   +    'update_test_postupdate_post_update_foo' => '8.x-1.0',
   +    'update_test_postupdate_post_update_bar' => '8.x-2.0',
   +    'update_test_postupdate_post_update_baz' => '3.0.0',
   

   Do we want to test the case of a single version removing multiple (post) updates?

Thanks @jungle for those fixes! All look good to me.

Re: #106.2:

Occurs when a module defines hook_post_update_NAME() that are also
+ * specified has having been removed in hook_removed_post_updates().

I think part of the problem here is that there's a typo of "has" in place of "as", so it says "has having".

Regarding the plural agreement, it's totally acceptable to me as a native speaker to say "There were lots of hook_post_update_NAME() listed in the docs but none gave examples of semver." Since hook_post_update_NAME() is not a word in English, it's fine and grammatically correct to treat it as though the plural is the same as the singular. Like "sheep". ;)

That said, it's also fine with me to change it to:

Occurs when a module defines hook_post_update_NAME() implementations that are listed as removed in hook_removed_post_updates().

Re: #106.

The conditional thing we're counting is $versions. We can't assume that "missing update" is also singular just because $versions is. In fact, the more common case is going to be that a single version is flagged here, but it removed multiple post update functions.

I think we can say "missing updates" even if it's just one, given the use of the colon. (Also a single update hook implementation makes many data updates, so it seems fine semantically.)

Also, not sure why "first" is only used in the plural case, but not in the singular.

"First" is used in both sentences. It's just in a different position in the plural case because it was more natural with the rewording I suggested.

If we really want this to be perfectly accurate, we need an outer if() for count($missing_updates), then use PluralTranslateableMarkup to handle count($versions).

That's not necessary.

Or, the simpler approach would be to always assume multiple missing updates, and let PluralTranslateableMarkup just worry about the count of versions...

Yes, this.

This is the most words ever written to say "Add an s to missing update".

So, TLDR, #106 should be addressed as follows:

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -763,6 +764,30 @@ function hook_post_update_NAME(&$sandbox) {
   +    'mymodule_post_update_foo' => '8.x-3.0',
   +    'mymodule_post_update_bar' => '8.x-3.0',
   +    'mymodule_post_update_baz' => '8.x-3.0',
   

   Change mymodule_post_update_baz to have a different version number. @catch would prefer that it not be semver for now, so use the 8.x- prefixed style.

2. Change the sentence to:
   

   Occurs when a module defines hook_post_update_NAME() implementations that are listed as removed in hook_removed_post_updates().

3. Leave as-is.
4. Say missing updates: @missing_updates instead of missing update: @missing_updates
5. Leave as is. It is 100% clear from context because you're getting this message when trying to run update.php.
6. Sure, anytime someone asks for more test coverage, it's probably worthwhile.

Apologies for the numerous edits; I had to get the right number of llamas.

Going to try to update for #107.

This should address #106.

Edit: Sorry @catch for not assigning this issue to myself 😔

Crosspost with lauriii...

#107.1

Changed foo instead of baz to have an earlier rather than later version number in there.

#107.2 done.

#107.4 done

#107.6: we now have test coverage for:

- skipping multiple updates that were removed in two versions
- skipping two updates removed in one version
- skipping one update removed in one version

This is also possible, and realistic, with the single test module hook implementation - just added one more skipped update to it, so by adding this we also get testing for when a module is specifying multiple updates removed from a version, but the site has only skipped some of those updates (which will be quite common - say you're updating from 8.x-3.5 to 8.x-4.0, skipping 8.x-3.6 to 8.3.11. and 8.x-4.0 removes all updates between 8.x-3.0 and 8.x-3.12).

Just need to update the other test for the extra removed post update.

Random ckeditor test failure.

It does look like a random failure, but the testbot also reports a problem with coding standards, so NW for that.

Fixing the whitespace cruft.

There were a couple crossposts since yesterday, so I've attached an interdiff from #91-116 that I generated to help me review.

#98.6 was cancelled out by #99, which is fair.

I didn't mean to start a debate about sprintf, sorry about that.

The final resolution of PluralTranslatableMarkup is great, thanks for the back-and-forth on that.

#106 and #107 are addressed, as well as #115.

I believe that means everything has been addressed and all of the appropriate sign-offs have been given.
Thanks to all those who worked on this!

Thanks @benjifisher for reminding me to attach the interdiff :)

Update issue credit to include all reviewers plus I added @Berdir for work on the duplicate #3097661: No hook_update_last_removed() equivalent for post updates

Very minor conflict on core/modules/system/tests/src/Functional/Module/InstallUninstallTest.php on 8.9.x - here's a patch for 8.9.x (and hopefully 8.8.x).

Committed 4ff3848 and pushed to 9.0.x. Thanks!

I think there is a case that we need to backport this to 8.8 (despite that it's very clearly a public API addition) so that contributed modules creating versions that are compatible with both D8 and D9 at the same can potentially use when defining their upgrade paths.

The primary risk of name collision seems not to have any in known contrib: http://grep.xnddx.ru/search?text=removed_post_updates&filename=

So the risks are only for custom code (and the fact that we keep adding important things in 8.8.x patch releases because of stuff we missed).

Still worth discussing a bit before we do commit it to 8.8.x and break someone's site if they stumbled in front of the correct template of function name somehow.

Here's a patch for 8.8.x - which changes core/modules/system/tests/src/Functional/Update/UpdatePostUpdateTest.php instead of core/modules/system/tests/src/Functional/UpdateSystem/UpdatePostUpdateTest.php - because we moved things about in #3106395: Move tests that test the update system to UpdateSystem namespace

So the reason to add this to 8.8 is the following:

1. Because we've added it to Drupal 9 and 8.9.x, contrib modules start to use it.

2. 8.8 sites (rightly) start updating all their contrib modules to versions that have implemented the hook in preparation for updating to 9.x

If we don't backport it, then those 8.8 sites won't get warnings that they've skipped updates that have been removed.

This is a pre-existing issue if a contrib module just arbitrarily removes post updates, but it's a bit different now we encourage it. So, we should probably backport yeah.

I think the patch in #116 was applied to 9.0x (see #120, #121).

The patch in #116 does not apply to 8.9.x; the patch in #120 applies but has test failures. That means this issue still NW for 8.9.x, so we should not yet change the target branch to 8.8.x. I am reverting that change (from #122).

So the 8.x patches are failing because git magically applied them to system.install even though we've not backported #3098475: Add more strict checking of hook_update_last_removed() and better explanation yet. We can decouple this from that but I think we should wait as that one is rtbc.

rerolling the 8.9.x version at least.

Not sure that's necessary now that #3098475: Add more strict checking of hook_update_last_removed() and better explanation has landed?

+++ b/core/modules/system/system.install
@@ -24,6 +24,7 @@
+use Drupal\Core\StringTranslation\PluralTranslatableMarkup;

This wasn't needed after #2917600: update_fix_compatibility() puts sites into unrecoverable state.
Was causing failure because the same use statement was present 2x.

looking at the 8.8.x patch now

@tedbow so we need to ensure that stuff added to system_requirements() only runs in the update phase. atm in the the 8.8.x patch it is running in the install phase too.

Ho hum I didn't mean to upload the patches I'd done. Sorry @tedbow I meant to let you get on with it.

Sorry @tedbow I meant to let you get on with it.

No worries just learning this issue. Here is try at 8.8.x patch

This is a review of the 8.8.x patch in #134.

1. +++ b/core/modules/system/system.install
   @@ -1109,6 +1109,9 @@ function system_requirements($phase) {
        // Also check post-updates. Only do this if we're not already showing an
        // error for hook_update_N().
   

   This comment is now irrelevant.

2. +++ b/core/modules/system/system.install
   @@ -1109,6 +1109,9 @@ function system_requirements($phase) {
        if (empty($module_list)) {
   

   This check is irrelevant too.

See 8.8.x patch in #132 that does:

+++ b/core/modules/system/system.install
@@ -1110,6 +1111,35 @@ function system_requirements($phase) {
     }
   }
 
+  // Check all the expected post-updates have been run.
+  if ($phase === 'update') {
+    $existing_updates = \Drupal::service('keyvalue')->get('post_update')->get('existing_updates', []);
+    $post_update_registry = \Drupal::service('update.post_update_registry');

#135.1

This comment shouldn't be irrelevant in 8.9.x. We should still be checking hook_update_last_removed() first, and falling back to checking hook_removed_post_updates() only if we're not already going to show a message for hook_update_last_removed() (to avoid showing two different error messages for one module).

#3098475: Add more strict checking of hook_update_last_removed() and better explanation has been backported to 8.9.x already, but probably won't be backported to 8.8.x (because there is already some handling in 8.8, it is just not very good), so it likely will be irrelevant for 8.8.

@catch #135 is a review of #134 - a patch against 8.8.x. I'll edit the comment to make it clearer.

Re-uploading the two correct patches for 8.9.x and 8.8.x since #132 actually has a patch that addresses #135.

Since the 8.9.x was just a re-roll, and also looks great, I've gone ahead and committed that to 8.9.x, that way we can focus on the actual 8.8.x backport by itself.

Reviewing this today

I don't see any reason to not commit the 8.8 patch as-is.
The only difference is that UpdatePostUpdateTest.php is in a different directory between versions, and system.install needs an extra use PluralTranslatableMarkup

Thanks for the extra review. Committed 1f57787 and pushed to 8.8.x. Thanks!