Handling update path divergence between 11.x and 10.x

And we're back, started updating the issue summary.

I think if we add a hook_update_N to both 10.4.x and 11.x then they need have different update numbers. Due to this they update function must either be re-entrant or use a flag to denote that it has been run so that when you update from 10.x to 11.x.

Discussed a bit with @catch. I think we should implemented an automatic flag checking system. So for example, we add system_update_100101 to 11.x and we backport it to 10.4.x as system_update_100001.

In the backported update we do something like...

function system_update_100001 {
  update_skip_future_update('system', 100101);
  // The update happens.
}

The code in update_skip_future_update() looks something like this:

function update_skip_future_update(string, $module, int $number) {
  \Drupal::service('update.update_hook_registry')->skipFutureUpdate($module, $number);
}

Then in update_do_one() we add code to do something like:

  if (\Drupal::service('update.update_hook_registry')->isFutureUpdateSkipped($module, $number)) {
      $ret['results']['query'] = t('Some text about the update already being run');
      $ret['results']['success'] = TRUE;
      $context['sandbox']['#finished'] = 1;
  }
  elseif (function_exists($function)) {
     // The usual code...

\Drupal\Core\Update\UpdateHookRegistry::skipFutureUpdate() and \Drupal\Core\Update\UpdateHookRegistry::isFutureUpdateSkipped() manage a new key value collection that is injected via the UpdateHookRegistryFactory

We used to use flags in the variable system in the 6-7 update path (when a 7.x update was backported to 6.x) but it was applied very inconsistently and went wrong all the time (like having to move issues back and forwards between 7 and 6 for the original update, the backport, then the flag check in 7.x etc.

Adding a light API for it might help us to get it right.

  if (\Drupal::service('update.update_hook_registry')->isFutureUpdateSkipped($module, $number))

I think when this is the case we can also delete the flag, that will mean we only retain flags on sites that are on 10.x until these updates are run.

One thing to note here is that this will allow us to backport updates out of sync - i.e. two updates added to 11.1 could be backported to 10.4 in the reverse order. That will often make no difference but need to be aware since it adds new ways for things to go wrong.

Also discussed the following scenario with @alexpott, which isn't in the issue summary and is a new problem with two active major branches.

Update 11000 is added to 11.1.x, it's backported to 10.4.x as 10401. Someone updates to 10.4.0, then updates from there to 11.0.0, they've now gone 'backwards' to a release where that update never existed. We need to prevent this, but can do so by checking if there are skipped logged updates that were run in 10.4.x, that don't exist at all in the release we're updating to, and also are higher than hook_update_last_removed() - in that case we'd show an updates requirement error and refuse to update, telling people to update to a later minor release instead.

If we do all of this, I think it will handle every scenario, that means:

1. We can't add any new hook_update_N() to either 11.x or 10.4.x until we've implemented this.

2. Implementing this probably needs to block 11.0.0 so that the update requirements check is in place.

Could we do something with attributes where we tag the backport with the future version (and/or vice versa) and keep all the code to manage it inside the updater? Or am I completely crazy?

Tagging the backport with the future version we might be able to use instead of update_skip_future_update maybe?

We need to avoid anything that involves referencing the 10.x backport from the 11.x update because that creates a three-way dependency where you first have to commit the 11.x update, then the 10.x backport (where you don't 100% know what the backport update number will be until it's committed), then another 11.x commit to reference the 10.x backport, but I think the plan so far successfully avoid this.

There was quite a bit of discussion in slack here: https://drupal.slack.com/archives/C03L6441E1W/p1716230905698789

@catch proposed the topic for the D11 Readiness meeting and @Gabor posted it.

@catch, @mikelutz, @mstrelan and I discussed it.

@mikelutz's idea came down to using attributes to tag the version numbers it should apply to, possibly doing away with version based updates and starting sequentially at a really high number and stop encoding the major version in the update hook, you then store when a hook ran and for what version.

I think the relevant conclusion is here:

So we’d store the last hook ran like : with version being probably . and when we detect that we are on a minor or major version higher than what we recorded as our last ran, we run through them all again, only running ones tagged with our version, but not also tagged with the last version we ran.

@mstrelan concurred that the solution likely should include some form of attributes to remove a need for magic function naming which may also allow us to remove hook_update_dependencies.

@catch posted some thoughts:
Core requirement is that it handles the case of an update being in 11.1 and 10.4, but not in 11.0.

1. The 10.4 update will log that the update has run, including both the 11.x and 10.x update names in the logged information (in key/value).
2. If someone tries to update to 11.0 from 10.4, we'll have the logged update from 10.4, and we'll be able to see that there's no equivalent 11.x update to run - at this point we can have an update requirements error that prevents going further and tells you to update to the next minor release of 11.x.

#34 1 and 2. Yes, I agree with both of these points.

The solution in #33 make sense and seems to be on the right track. We do need to be careful with this line update_skip_future_update('system', 100101); because a typo could have serious consequences. Using the given scenario 10.4.x won't know the real update number so we can't even add a simple test that the update exists. And perhaps some name changing away from 'skip future' to 'has run' or something like that since we are really checking if the update has already run.

I think attributes is a good idea but that feels like a replacement of the update API that we should explore in its own issue - would be a good thing to work on parallel to OOP hooks. All the information we need is the same as #33 so if we implement that first, we'll have a good basis to build the new API from. We always needed something like this in the update system but managed to mostly escape needing it since Drupal 8 development which is a good ten+ years now.

I think attributes is a good idea but that feels like a replacement of the update API that we should explore in its own issue - would be a good thing to work on parallel to OOP hooks. All the information we need is the same as #33 so if we implement that first, we'll have a good basis to build the new API from. We always needed something like this in the update system but managed to mostly escape needing it since Drupal 8 development which is a good ten+ years now.

Couple of minor comments on the MR, but that is very encouraging. I think we can work in this issue - if we have this API, we don't need a policy as such.

I discussed this a bit with @daniel.bosen from Thunder. He wondered if we should consider doing this the other way around. I.e. allow an update to indicate it is skipped if an update has run. We'd need to log every update that has been run which is possible and might be desirable in other situations. But, after thinking about this some more I think we can't do this because we would not be able to detect when you go from 10.4.x to 11.0.0 and emit an error because we expect the skipped update to exist. This ability to prevent a kind of downgrade is a key feature of the system that'll help mitigate issues.

He wondered if we should consider doing this the other way around. I.e. allow an update to indicate it is skipped if an update has run. We'd need to log every update that has been run which is possible and might be desirable in other situations. But, after thinking about this some more I think we can't do this because we would not be able to detect when you go from 10.4.x to 11.0.0 and emit an error because we expect the skipped update to exist. This ability to prevent a kind of downgrade is a key feature of the system that'll help mitigate issues.

Agreed with this, there's also another reason which was a real struggle during the 6.x-7.x cycle.

Lets say system_update_110001() is added to 11.1.0, it is not immediately backported to 10.4.x but instead left at 'to be ported'.

system_update_110002() is added to 11.1.0 and is also set to 'to be ported'. Let's also assume zero interdependency between the two.

Until the 10.4.x backport is actually committed, we have no idea what the 10.4 update number will be and there's no way to 'reserve' one. This means we'd need a third MR, to 11.x, after the 10.4 commit, to add the reference to the 10.4 update once it exists. This is what we used to do for some update in 7.x, it did not work out well. The current approach avoids it entirely by only requiring knowledge of the two update numbers in the backport.

Argh d.o ate my comment.

So overall this seems like a sound approach, although I haven't spent time trying to think through edgecases. The main thing that is missing is API documentation for this mechanism. It should be detailed and include an example, like the IS does.

It took me a minute to figure out where the update hook docs live; it's lib/Drupal/Core/Extension/module.api.php.

We might also need to make handbook doc updates somewhere.

NW for the API docs.

Yep that's a good point, we need a new section in the hook_update_N() phpdoc with why/when/how to call the new method.

Also looking at that, we might want to a new issue to update (or make generic) some of the version number examples so we're not talking about Drupal 8 as much.

So, I'm all for doing something more robust and automated with attributes alongside a deeper rework with OOP updates, but looking at this, I wanted to throw one other idea out there as a temporary stopgap that might be easy enough to implement and avoid boilerplate service calls in each update hook and avoid needing to renumber and reroll for every backport.

So instead of calling this service manually in every hook, we just do it automatically, i.e. store a list of every update hook that we've ran (populate it with all existing schema numbers on the first runthrough) And at the end of an update, we store the current Drupal major version. When we go to run updates we check if the current major version is the same as the one we previously ran, and if it is, we run updates like normal, appending the ones that were run to the list.

If we discover that the major version changed, we do things differently. We pull the list of hooks that we ran, subtract any below LAST_UPDATE_REMOVED and compare it to a list of all the hooks currently discovered.

If we find one we ran that no longer exists, we are are in the situation of #34 and can bow out. Otherwise, we can run everything that exists that isn't in our list of functions that already ran. At the end, we update the ran list, and save the new major version number.

With a system like this, we add all new hooks in the 11000 range regardless of backport. We can backport some of them, keeping the same numbers, and they will run in order just fine on the old major. When you update to the new major, hooks that aren't backported will be ran, hooks that were backported will be skipped. (This again means hooks may run out of order, but that's going to be the case with the current solution anyway)

The benefits that I see is that the hook has the same number on all branches, so there less refactoring just to backport, and there's no boilerplate that the developer needs to add to every hook.

Negatives might be storage of all the updates we've ran already, but that list would be basically the same as what's been proposed here, An entry in KV for every backported hook.
We also would probably want to find some way to differentiate core modules from contrib and only apply this system to core modules, since contribs comparable problem would be related to their version numbers, not cores.
I am not sure we could implement this without also adjusting the drush updater, I looked breifly, it does hook into core for a lot of the work, but it depends on the implementation
Have to handle edge cases when there are no update hooks, or if an update errors out in the middle of an upgrade. We'd probably have to store the core version for each module, and ensure we only update it if all hooks succeed in a run, but I think it would be doable.

Anyway just wanted to throw the thought out there just in case it triggers any thoughts for anybody. Looks like Alex is making good progress on the other solution, which is probably simpler to implement.

I think #47 would work for this case and most of what the current MR handles.

If we find one we ran that no longer exists, we are are in the situation of #34 and can bow out.

However there's three cases where it's tricky:

#1:

Otherwise, we can run everything that exists that isn't in our list of functions that already ran. At the end, we update the ran list, and save the new major version number.

When a module is newly installed on 10.4, then updated to 11.x from there, schema_version would record the highest update number available at the time of install, but we wouldn't have a record of any updates being run. I guess we could do what we do for post updates and log all of the updates in the code base as if we'd run them, maybe that would work, but a bit of complexity involved there.

#2:

With a system like this, we add all new hooks in the 11000 range regardless of backport. We can backport some of them, keeping the same numbers, and they will run in order just fine on the old major.

This assumes that we always immediately backport an update to 10.4 when we commit it to 11.x, but if there's ever an issue we decide to backport later, especially if later is in 10.5 instead of 10.4, update numbers would get out of sync. One advantage of the current MR is that it also handles this case.

#3 We need to support sites updating directly from 10.3 to 11.1, these won't have a log of any updates that have been run because we don't log anything yet - this might be fine because we also won't have any backported updates in 10.3 though?

One more things:

avoid boilerplate service calls in each update hook

The boilerplate service calls are only in the backported updates, not every one - i.e. contrib will probably never have to deal with this.

needing to renumber and reroll for every backport.

We would need to do this though with the proposed solution yeah.

If we discover that the major version changed, we do things differently. We pull the list of hooks that we ran, subtract any below LAST_UPDATE_REMOVED and compare it to a list of all the hooks currently discovered.

This system has to work for extensions and core. We would need to have a concept of major version change that's reliable - it gets even more complex for contrib that often end up supporting multiple minors and major (see the current situation on inline entity form).

For me decoupling the update skipping from major version change makes it:

• simpler to predict how it is going to work, less magic in this area is important
• give better errors as we add the expected version string, because we have to add the boilerplate.

It is through convention that we remove updates and change update numbers around major versions but we could get into this situation if we ever had two LTS branches on the same major version.

When a module is newly installed on 10.4, then updated to 11.x from there, schema_version would record the highest update number available at the time of install, but we wouldn't have a record of any updates being run. I guess we could do what we do for post updates and log all of the updates in the code base as if we'd run them, maybe that would work, but a bit of complexity involved there.

Yeah, I suppose we would have to.

This assumes that we always immediately backport an update to 10.4 when we commit it to 11.x, but if there's ever an issue we decide to backport later, especially if later is in 10.5 instead of 10.4, update numbers would get out of sync. One advantage of the current MR is that it also handles this case.

Curious how often this would happen. If it's more than once or twice in a cycle we would probably need to come up with a better solution, but if it's once or twice, I would say:

given update hooks 1 and 2 in 11.x where 1 is committed to 11.x first, but 2 is backported first:
You would have to make a new commit to 11.x with an empty hook_update_3
backport the update code to hook_update 3, and in that update hook add hook_update_1 to the ran list (in some way that doesn't race/conflict with the list already checked out in the updater that would later be resaved.., so it probably does need some sort of consideration on implementation) so that on upgrade both are skipped in 11

#3 We need to support sites updating directly from 10.3 to 11.1, these won't have a log of any updates that have been run because we don't log anything yet - this might be fine because we also won't have any backported updates in 10.3 though?

I think we would need to replace the updater in 10,3 as well and start logging there, but that's no different than the current solution. If we don't have this in 10.3.0 release, and someone upgraded from the .0 release then 11 would just run everything after last schema (which would be right) and save the major version as 11. The code would have to be in 10.3 before we backported something after we didn't backport something. Which I believe would be the same time we would need to have the current code in 10.3 (i.e. the first time we wanted to flag that some backported hook_update_103xx was equivalent to some future update 110xx

Re #47 and #50... I guess one thing we could consider is always doing this.. and ignoring the part about major versions. I'm not actually sure that that is necessary.

The one very edge case I can imagine is what would happen if we have to add an update to 10.4.x only - say one of the remove contrib modules in 11.x needs an update due to security what would then happen?

Re #47 and #50... I guess one thing we could consider is always doing this.. and ignoring the part about major versions. I'm not actually sure that that is necessary.

I agree. I had considered it. It would be a behavior change, I couldn't come up with a reason why someone would introduce a lower number update hook intending it not to be ran unless you weren't past that schema number, but somebody out there is heating their office with the spacebar (https://xkcd.com/1172/)

The one very edge case I can imagine is what would happen if we have to add an update to 10.4.x only - say one of the remove contrib modules in 11.x needs an update due to security what would then happen?

Just use the next 110xx hook number and skip that number in 11.x. I would probably recommend committing an empty hook with a comment to 11.x. OR - if we go with the above plan of just doing this all the time, you could just use the next 104xx number.

Also the strictness that #47 would impose is probably only manageable for core - this could make it harder to upgrade if a module added and then removed an upgrade on purpose - even though it should not. But all sorts of funny things happen in contrib and custom. Whereas the current solution is light enough to be adopted by any module (even though it certainly is not simple)

Not worrying about the major version would definitely simplify things. And allow this to work for contrib out of the box for their own multiple supported versions. and solve potential issues with partial updates.

You would have to make a new commit to 11.x with an empty hook_update_3
backport the update code to hook_update 3, and in that update hook add hook_update_1 to the ran list

This is the kind of thing I would prefer never to have to think about if we can avoid it.

I think we would need to replace the updater in 10,3 as well and start logging there, but that's no different than the current solution. If we don't have this in 10.3.0 release, and someone upgraded from the .0 release then 11 would just run everything after last schema (which would be right) and save the major version as 11. The code would have to be in 10.3 before we backported something after we didn't backport something. Which I believe would be the same time we would need to have the current code in 10.3 (i.e. the first time we wanted to flag that some backported hook_update_103xx was equivalent to some future update 110xx

It would need to be in 10.3 before we backport something to 10.3, but we might not need to do that at all - we only need the new method to call in the first update function that needs to call it (which will hopefully be in 10.4).

The one very edge case I can imagine is what would happen if we have to add an update to 10.4.x only - say one of the remove contrib modules in 11.x needs an update due to security what would then happen?

That might need co-ordination with the 11.x contrib version of the module anyway, it would mean the 10.4 core update having to reference the 11.x contrib equivalent.

This is the kind of thing I would prefer never to have to think about if we can avoid it.

Another problem that goes away if we check for unran updates on every run instead of just on a major version update

Another problem that goes away if we check for unran updates on every run instead of just on a major version update.

That is a really good point. We probably should not hardcode core's assumptions. Contrib might want usecases for them to commit the same update hook to two active minor branches or suchlike... which is actually a situation core could also run into if we had to do an update on the security-covered branch.

I think what we need is a bunch of test coverage with a big old data provider testing various scenarios.

What about of moving to the post_update mechanism here, i.e. instead of numbers start using names & just keep the record of the executed ones.
Core can just move to this approach while still allowing numbered updates (mostly for contrib).

Probably, existing post_update code can be reused here. The issues I can think of are: order of the updates and the last removed update thing.

We would also need to document how rerunning an update hook changes if there is any change.

It's rare to need to rerun them unless you're testing an update hook, but when you're writing them sometimes rerunning, rather than resetting the db you'd just lower the schema and run the update command again. If there are other elements stored we would want to ensure that it is clear how to rerun an update hook.

What about of moving to the post_update mechanism here, i.e. instead of numbers start using names & just keep the record of the executed ones.

The issue here is that the numbering of the update hooks is generally important for order if you are jumping several versions at once. I'm not completely happy with running the backported ones out of order, but it's unavoidable, we will just have to make sure that we take it into account when deciding what to backport and when. But in the grand scheme of things we need to try to run them in the right order and particularly make it possible for a developer to specify the order when they need to.

Yeah even if the 10.x and 11.x orders are different, they need to be predictable. The current MR doesn't change the hook_update_N() at all, it just adds to it. We can look at completely redoing the hook_update_N() API but shouldn't be changing it in this issue which is an 11.x blocker.

I looked at adding docs to module.api.php but I think the given the amount this is going to be used and the overview nature of the update docs in that file I think we should add a section to https://www.drupal.org/docs/drupal-apis/update-api → instead. Because there we can really work on the describing the situations where it should be used.

The cspell yarhar nonsense is addressed in 🐛 A revert has cause cspell to fail due to the word yarhar Active

At one point I expressed a concern about skipped updates for modules that are moving out of Drupal core - eg. Forum... but as these modules have the same name in core and contrib I think this is fine and we don't need to extend the API to support skipping an update from a different module (that ability would kind of break my head).

I've update the MR to address most of the feedback on the MR - where I didn't know what was wanted I've asked for suggestions.

I've also improved the message to detect core provided modules and to display a more accurate message in those cases.

I've been trying to think about added extra test cases and what they might be - anyone got some good ideas?

I thought I would take a stab at the other option, just to see what it looked like. Not tested yet, and likely horribly broken. I'm happy to keep working on it if there is interest, and happy to close it if we are going with #33, but I wanted to at least attempt to put some code where my mouth is, since I was advocating for it.

Besides the potential for testing to reveal an inherant flaw in the plan, there is still a bunch of cleanup to do. update_get_update_function_list should get deprecated and moved into UpdateHookRegistry, update_resolve_dependencies should also probably be deprecated and moved to refactor away from passing $starting_update around, things like that.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

For me this is a big disadvantage - you can't tell people what to upgrade to.

Update 11000 is added to 11.1.x, it's backported to 10.4.x as 10401. Someone updates to 10.4.0, then updates from there to 11.0.0, they've now gone 'backwards' to a release where that update never existed. We need to prevent this, but can do so by checking if there are skipped logged updates that were run in 10.4.x, that don't exist at all in the release we're updating to, and also are higher than hook_update_last_removed() - in that case we'd show an updates requirement error and refuse to update, telling people to update to a later minor release instead.

I started coding an attribute and storage to be able to give an actual version number and I realized we may be completely over thinking this in both approaches here by trying to calculate if we can do an upgrade or not based on missing update hooks. We don't need to. We already know that if you are on 10.3 you can upgrade to 11.0, if you are on 10.4 you have to go straight to 11.1, and if you are on 10.5 you have to go straight to 11.2.

We are adding code and work to hunt for maybe a situation where possibly if you are on an early enough patch version of 10.4 or don't have any modules installed that may have a database update hook skipped in 11.0 that we can technically let you upgrade to only 11.0, but why bother? 11.0 will be in security phase by then, 11.1 will be the recommended version and more importantly regardless of database schema, by the first patch release, 11.4 will have bug fixes in it that aren't in 11.0. Regardless of whether the upgrade to 11.0 is technically possible at that point, we would be allowing them to upgrade to a more buggy version which would be a bad experience we want people to avoid anyway.

I think we just need to document and require that 10.4 must upgrade to 11.1, and 10.5 must upgrade to 11.2, enforce that in system_requirements, and not even mess with trying to calculate allowable upgrade versions via finding missing update hooks. We could address that in a separate issue, and it wouldn't even be a blocker until 10.4 beta.

Have we ever had a requirement that you need to skip a minor version when updating a major version?

No, but we've never had a major version receiving bugfixes while the next major's first minor was down to security support or out of support entirely, so we've never needed to.

I left some comments on the first approach wondering how to deal with the drush runner, which has its own version of updateDoOne that would need to be updated as well, and wondering if we could go back to attributes since we are changing updateDoOne. We could keep most of the code the same, but use #[UpdateSkip('11101', '11.1')] on the backported hook and look for it in UpdateDoOne and call the service to save the skip data right after or before we run the hook. It might be cleaner that way.

Regarding #72, that feels like a huge potential issue to require skipping a certain minor version depending where you update from.

Regarding #73 wouldn't the update_skip_future_update function handle drush too?

Regarding #72, that feels like a huge potential issue to require skipping a certain minor version depending where you update from.

In theory, no. In reality, it could be, but with the scheduled support cadence, there is really no way around it. Going from 10.4 or 10.5 to 11.0 would effectively be a downgrade. I'm leaning towards recommending my agency keep clients on 10.3 until we are ready to execute upgrades to 11. My mind could change depending on what happens in the next 6 months, but I'm thinking the sweet spot for upgrades to 11 will be to go from 10.3 to 11.0 in the first half of 2025 followed by a quick update to 11.1, and then 11.2 in the second half of 2025.

Regarding #73 wouldn't the update_skip_future_update function handle drush too?

Well, looks like we bypassed the update_skip_future_update function in favor of just calling \Drupal::service('update.update_hook_registry')->skipFutureUpdate($module, $number); directly in the module, but either way, no. That method just logs that the future update should be skipped, but it's the update runner that currently is responsible for checking that state and deciding not to run the future update when it eventually sees it. But drush has a separate batch runner to actually execute the hooks, which at the moment doesn't know it needs to check to see if an update should be skipped before running it, so if you were running drush updb as everything sits now, you would end up re-running the future update hook. Admittedly update hooks often are or can be made idempotent, (if thing A exists, convert it to thiing B. On rerun, thing A doesn't exist so the hook does nothing anyway). That would further reduce the times we would need to use this (i.e. only if the hook needs backporting to 10.x and only if thinks would break if it were to be re-run)

Going from 10.4 or 10.5 to 11.0 would effectively be a downgrade

Yes thank you for clarifying, I'm curious what the core team things about that policy, I think that's part of the question of this issue.

but it's the update runner that currently is responsible for checking that state and deciding not to run the future update when it eventually sees it.

Yes, that brings drush back into play, do you think it makes an argument to using: `update_skip_future_update`?

That would further reduce the times we would need to use this (i.e. only if the hook needs backporting to 10.x and only if thinks would break if it were to be re-run)

The only issue I see with this is you need to know if you're going to backport, I think this solution is meant to solve for the case where you didn't know you were going to backport when you wrote the original update hook.

Yes, that brings drush back into play, do you think it makes an argument to using: `update_skip_future_update`?

I'm not sure what you mean here, I may be missing something. All that function can do is log that we should not run that future update when we eventually see it. If the thing that calls that future update at some point in the future doesn't know to check the logs and see it shouldn't be run, then it's going to be run. There isn't going to be anything we could do in the present to prevent an update from being run in the future without the cooperation of the thing that is calling the future update function, unless, as I noted in the MR, we don't check that from the runner and instead wrap the contents of the future update with some sort of `if (should_be_skipped(__FUNCTION__)` every time.

The only issue I see with this is you need to know if you're going to backport, I think this solution is meant to solve for the case where you didn't know you were going to backport when you wrote the original update hook.

That's not really an issue, as all the code for this goes only in the backport, at least as things stand until we figure out what to do about drush.

Sorry, my understanding was that update_skip_future_update is called within the update hook itself, so after it checks the logs if it shouldn't run then it just returns and reports successful.

Then it doesn't matter who is calling the update hook since the hook itself checks if it should be skipped, so the past update logs the future update number to skip, the future update checks the log to see if something marked it as needing to be skipped.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I've opened 📌 Log hook_update_N() that have been run instead of only recording the schema version Active to continue exploring logging all updates that have run per the @mikelutz MR so that we can continue with the minimal fix here.

catch → changed the visibility of the branch 3108658-run-all-unran-updates to hidden.

I do wonder if we should just implement this in a simpler way as per #78.

In fact do we even need such a complex mechanism at all, can we do this purely from hook_update_N? Following the example from the IS:

1. When we know we have an update for both versions, we set a state flag system_skip_update_11100 in system_update_10400().
2. In system_update_11100() we then check that state flag:
   • If it's set, we delete the flag but exit successfully as the update can be skipped
   • If it's not set, we run the update as normal

In fact do we even need such a complex mechanism at all, can we do this purely from hook_update_N? Following the example from the IS:

When we know we have an update for both versions, we set a state flag system_skip_update_11100 in system_update_10400().
In system_update_11100() we then check that state flag:
If it's set, we delete the flag to keep things clean, and exit successfully as the update can be skipped
If it's not set, we run the update as normal
There is the case where we *might* backport something to both versions, and we don't know up front. We can still add the check to the later version in that case; just, it will never be set if we never complete the backport, so the update will still proceed as normal.

This is similar to what we used to do with variables in the 6-7 update, which broke all the time, but it's different in a very specific way which might make it OK after all.

In 6-7 the 6.x update would set a variable like 'system_update_6002' and then the 7.x update would look for 'system_update_6002' - this created the near endless chicken and egg situation where even if we knew we wanted to backport, we couldn't 100% predict the 6.x update number in advance (because another backport might land the same day and take that update number).

However, if the k/v entry is 'system_update_11001', then we can commit that to system_update_11001() and it will never, ever matter if it wasn't backported or what the backport update number was.

The only issue then is adding the couple of lines of boilerplate to every hook_update_N(), and the possibility of forgetting, but that seems OK.

The last bit which it's too late in the day to wrap my head around though - can we still do the 10.4 -> 11.0 detection that way? I guess we probably can by checking if a key is set, and if the update doesn't exist, setting a requirements warning.

So unless I've missed something, then #83 is completely viable and it's just whether we can live with the boilerplate. However the other big change from 6-7 is we have post updates now, which don't need all this, so we have less hook_update_N() to write overall anyway.

I don't have a massively strong opinion either way,so whichever makes this easiest overall is good with me.

I think doing

The last bit which it's too late in the day to wrap my head around though - can we still do the 10.4 -> 11.0 detection that way? I guess we probably can by checking if a key is set, and if the update doesn't exist, setting a requirements warning.

might be quite tricky, Unless we create helper functions to do this. In fact we could move remove the automatic skipping from the MR and use the API in the updates to achieve what is being suggested - but I think doing it automatically makes more sense.

One possible approach:

- keep the magic that's currently in the MR, because it makes things convenient and more fail-safe

- add an extra helper method for 11.x update hooks to check if they should skip, that duplicates the logic in the helper.

Then, until drush adds support for this, or even after that, we can keep the boilerplate in Drupal 11 update hooks (which there won't be that many of), and the worst that happens if the entire method doesn't run at all anyway.

Added a value object to make it slightly simpler... so now a future update could do:

$skipped_update = \Drupal::service('update.update_hook_registry')->getSkippedUpdate();
if ($skipped_update instanceof \Drupal\Core\Update\SkippedUpdate) {
  return $skipped_update->toMessage();
}

You don't even have to pass in the module and update number to the function because we can use the same trick to determine them from backtrace.

Also the value objects are nice for Drush because then they can use the same message as core easily.

NW for @quietone's requests. I left a few comments with some thoughts for folks to think about, but none are hard requirements for this MR, they could be done here if they make sense, or they could all be debated and done in a follow-up (or not)

I've made some of the requested changes and added comments on some of the others. I think we need to work on the docs added by @quietone.

Worked on the documentation module.api.php. I tried to be consistent by using 'commit' instead of applies or added. Then, changed to expand the example to be easier to follow for those new to updates. To show the problem first, then show the solution.

There is also a wiki page for this that should be updated, https://www.drupal.org/docs/drupal-apis/update-api/introduction-to-updat... →

Updated IS to reflect most recent changes.

Okay, I reviewed this pretty thoroughly as it was being built and gave it a final once over. As far as I can tell, we've covered all edge cases, tests look good, documentation looks good. Let's ship it.

Committed/pushed to 11.x and cherry-picked to 11.0.x and 10.4.x, thanks!

Theoretically we might need this if we backport an update to a 10.3 patch release (at least unless we're able to use a 103xx update number in all branches), however because this is a relatively big change I'm not backporting it just yet.

Don't think we need a release note here unless we backport to 10.3.x because there's no action for anyone to take, even contrib modules are relatively unlikely to need this at least compared to core.

catch → closed merge request !8146

Went to publish the CR and noticed it already has the expected 10.3 version numbers listed, let's make our current and future lives easier and backport it all the way back. Also means the API will be available for contrib to reliably use earlier too.

I like the nice improvements to the API documentation that happen at the end here. And I think the change from 'skipped' to 'equivalent' is the docs and code will make this easier for others to understand. Very nice.

Added a release notes snippet for 10.3.2

Updated the release note to explain who is affected.

https://xkcd.com/859/

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