No hook_update_last_removed() equivalent for post updates

We can simply append the current Drupal major version to the update name when it is being retrieved in the update process. If we do this starting D9 it does not even require an update for the previous update names, as when views_post_update_resave_views() is being saved in D9 we can save it as

views_post_update_resave_views_9 or 9_views_post_update_resave_views.

I've been pondering this one. I think that maybe for modules that have post updates in 8.x we could add a hook_update_N that cleans all the 8.x post updates from state. So for example, we add a patch that:

• Removes system.post-update.php
• Add system_update_9001 that cleans the state

And then in 9.x we're free to add whatever post updates we want in sysztem.post-update.php

I think each module chooses when to clear their post updates. This needs to be done on a module per module basis. Perhaps I should not have used system as an example.

Good idea with helper!

As post update functions are ordered this helper could make sure all updates it suppose to clean from registry actually registered since extension installed

Some similar ideas going on in #3066801: Add hook_removed_post_updates() but it does feel like simply deleting old updates, rather than remembering deleted ones forever, is the way to go.

I think the prefix doesn't work, per #3087644-105: Remove Drupal 8 updates up to and including 88**, we now already have a post update in system.post_update.php that we'll keep and we might add more in 8.8/8.9, then if system_update_9001() would remove everything prefixed with system, it would also forget about those that are still there and would run them again?

Also not sure if I like the approach from #3066801: Add hook_removed_post_updates(), we'd have to keep that removed list around for basically forever and it would keep growing.

Having an API where you can explicitly list which ones you remove seems like the safest option? For a few modules, that's going to be annoying, we have system with 18, views with 17 and layout_builder with 15, but many have just one or two and a few have beetween 3 and 8.

Here's a proof of concept patch that adds the API, a unit test and implements it for the action module. I'd expect it to fail until #3087644: Remove Drupal 8 updates up to and including 88** is in, also would have lots of conflicts with that, so waiting until we update additional modules.

Wondering if we want to add a Drupal::postUpdateRegistry() method as this will solely be used in update functions. Similar to \Drupal::entityDefinitionUpdateManager().

> One idea that's neither a list nor a prefix - could we use the update discovery code itself, then we could remove everything from the database that's not in the codebase.

That wouldn't prevent us from accidentally adding a post update with the same name in 9.x as we did in D8 and removed here. The hardcoded list would remove it and would then run the new post update, your suggestion would fine it in the code base and would therefore assume that it wasn't removed and would not run it.

I assume our tests would prevent this, as they would fail as they wouldn't run the update, but a contrib or custom module will likely not have that test coverage.

I wrote the original #3066801: Add hook_removed_post_updates() patch and I'm definitely not married to that solution and welcome something better. I'm glad people are working on this. That said, reading over this proposed solution, I do have a question.

+++ b/core/modules/action/action.install
@@ -0,0 +1,13 @@
+function action_update_9000() {
+  \Drupal::service('update.post_update_registry')->filterOutRemovedUpdates('action', ['move_plugins']);
+}

Unless I'm misunderstanding what's happening here, running this on hook_update_N() seems like it could create its own issues. What happens when a new developer installs this module from scratch? The key_value table would have no record of action_post_update_move_plugins() ever existing. So they could create it with the same name again and potentially merge it (especially since dev environments often are rebuilt from scratch). Then you could end up with a fractured subset of users with that function filtered out, and those that don't. That could result in unexpected behavior.

I don't really understand how that would be mixed.

Either someone installs it before it is removed, then it is added to the database, later on this update runs and removes it. So it is no longer in the database.

Or someone installs the module after it was removed, it doesn't exist in code, so, it is never added to the database in the first place.

The result is the same. It's no longer tracked as an already executed update and it's safe to add another post update with the same name.

Well chalk that up to my comment:

Unless I'm misunderstanding what's happening here

When you explain it that way, it makes more sense. Carry on.

I think the approach in #10 works. If an update with the same name is added after the hook_update_N that removes it (or even in the same release) then it should be run.

I think we should add test coverage of this though.

@catch I think we can add this. I think we need to provide a list of post updates to remove and if any of them aren't there then we need to produce a message like the hook_update_last_removed. I think we we could just check for the existence of the last one added.

Yes, I'm not quite sure how to handle that both. Even if we'd only add the latest, we would still need to remember that we actually 5 years ago added and then removed 3 years ago a post update with name X and must never use it again.

We could theoretically fail when removing a post update that wasn't actually executed but by then, it's basically already too late.

Maybe we could somehow have a mix, and have a hook that returns updates, and then in requirements checks for them having been run, and if yes, we remove them as part of the updates.. but that seems pretty complicated, and if you remove post updates from that hook again before that did run on every site we're back to square 1.

FWIW, I still don't really understand why we didn't just use numbers for post updates too :)

I think there are also some other risks around users skipping large volumes of post updates and then trying to do them all at once.

From what I understand, post-update hooks run in alphanumeric sequence. That means if post-update hooks were supposed to run in sequence B A D C, it actually runs in the sequence A B C D.

In most cases it's probably harmless, but if B has to be run before A, then you can potentially get some unexpected behavior there.

I'm not convinced that we can't achieve the best of all worlds mentioned in #20. I think we can do something like the following:

The removal of post updates needs to be done in a hook_update_N() - so we know the the schema number when the updates are removed.

In the same commit we then implement a hook_removed_post_updates() that returns an array of schema versions and the updates removed.

So for example in action_update_8001() we remove the post update action_post_update_move_plugins().

function action_update_8001() {
  \Drupal::service('update.post_update_registry')->filterOutRemovedUpdates('action', ['move_plugins']);
}

And we implement action_removed_post_updates() as so:

function action_removed_post_updates() {
  return [
    8001 => [‘action_post_update_move_plugins’]
}

Them when we go to update.php if action action schema is 8001 or greater we don’t care about the return from action_removed_post_updates() because the schema is 8001 but if the schema is 8000 and action_post_update_move_plugins is not in the post update registry then we error saying that are missing an update.

Perhaps we can make the return value of function action_removed_post_updates() also include a descriptive error message so something like:

function action_removed_post_updates() {
  return [
    8001 => [
      'message' => 'Updates are missing from the Action module. Update to latest Drupal 8 first.',
      'updates' => [‘action_post_update_move_plugins’]
    ],
}

If the approach in #26 is taken, would it be helpful at all to add an optional link property there as well where you can link back to either the Issue or Change Record where that particular post update hook was removed? Something like this:

function action_removed_post_updates() {
  return [
    8001 => [
      'message' => 'Updates are missing from the Action module. Update to latest Drupal 8 first.',
      'updates' => [‘action_post_update_move_plugins’]
      'link' => 'https://www.drupal.org/project/drupal/issues/3097661',
    ],
}

Then we could display some sort of message like "See @link for more information", so user's can get more context about what changes they're missing out on? I know I'd find that kind of information helpful. Perhaps that could just be included as part of the message value, but the idea of separating it out is appealing to me as it serves as a reminder to actually include a link.

I definitely think it would need to be optional for people working on custom modules, but maybe it could be mandated for core and highly recommended for contrib?

Tagging as a requirement for Drupal 9.0-beta1.

Here's a start - I've not introduced the message stuff yet as I'd want to start adding tests. I think one thing we're going to hit here is that the current update system is not that well tested wrt to the unhappy path. Already I discovered that if there are no other other updates the error behaviour is different than if there are updates.

Going to work on adding real update path tests now.

Some quick docs updates to make the patch/idea clearer.

Yeah that logic is not quite right. We need to use the info to check that the updates it details exist.

The other interesting thing is that #34 shows that core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz is not a bare standard database. It doesn't have action installed and testing has shown that core.extension:profile is NULL - which is not right either.

Ah I see what is happening here. The action module is not part of standard (phew) and the install profile in the database dumps is not correct because when they were created the settings file did not have 'standard' in it so we left it as NULL. So I'll open an issue to fix the profile in the database dumps but looking at the installed modules in core/modules/system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz it does look like a site build from the standard profile (phew). And it is nice that the filled dump has more modules installed.

Here's a test. Got a few of the paths covered. I need to create a blocking bug about what happens when update.inc sets a warning for hook_update_last_removed() and the new hook added here because they are something things that appear to be broken.

@catch yep that will fix some of the old behaviour I've seen. I wonder if this patch should move directly to the solution we're going for there and instead of trying to soldier on and try to run compatible updates if they are available we shoudl halt the update process. As I wrote on that issue

I think the idea of the current code is to allow the user to perform some of the available updates and continue soldiering on - but I think that that solution is from the world were you can disable a module and have it magically come back to life when it's being updated.

@catch,

I like the idea of more standardized naming conventions here, but can you clarify what you mean by "branch number"? Is that the core version branch? So if you're developing against core 8.8.x you use 88 and if you're developing against drupal 9.1.x you use 91? If that's the case, does that mean anytime a patch gets rerolled against a new version of core, the patches would need to be updated? Also, what happens if the same hook needs to happen twice in the 8.8.x lifecycle?

For some of the modules I maintain I've tended to do use a naming convention like this web_page_archive_post_update_3072289_set_default_retention_values(), where 3072289 maps to the Drupal.org issue number that introduced that particular update hook. Or for our internal custom modules, we use JIRA ticket numbers instead. I find it helpful as I can clearly map any change back to a particular issue. It still doesn't necessarily solve the issue of two post updates potentially running out of sequence but does significantly reduce the possibility of a collision.

"When we added the API, we intentionally did not support strict ordering or hook_update_dependencies() support since post-updates aren't expected to have the complex dependencies that hook_update_N() sometimes do."

I guess cases, where I could see problems happening with sorting, is something like this:

DB has a column with some numeric value n.

An issue decides to double that value and implements example_post_update_double_value().
Another later issue decides to add one to that value and implements example_post_update_add_one_to_value().

The developer writing example_post_update_add_one_to_value() may not have full context of what happened in example_post_update_double_value(). The module maintainer, may or may not remember, or if there are multiple maintainers, and maybe they don't have full context either.

For users who ran example_post_update_double_value() separately from example_post_update_add_one_to_value(), you would get the value 2n+1

But for users running those hooks at the same time, you would end up with the value 2n+2, since example_post_update_add_one_to_value() would execute before example_post_update_double_value() alphabetically.

Now this is obviously a very hypothetical edge case, and I'm unsure if it would actually happen in practice, but it could, so I definitely think some sort of sequence-based naming convention like you're suggesting makes a lot of sense.