Add hook_post_update_NAME() for data value updates to reliably run after data format updates

I think something like this got discussed around #2233403: Let update.php invoke hooks again which got us to our current state of hook_update_N().

For the first kind of updates, changes should be performed on the lowest-level data API possible (e.g., for that issue, using the raw Config API rather than loading/saving the block as a Block Entity), because we do not want hooks, such as hook_ENTITY_TYPE_load() and hook_ENTITY_TYPE_update() being invoked while the schema is still in flux, especially since those hooks are for reacting to meaningful changes to entities, not to schema changes.

The config API isn't the lowest level API available, and https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Config!Config.php... fires an event. I can't think of an event subscriber that would care about the structure of the block context mapping, but the existence of that event suggests that some module somewhere could be doing something that could end up breaking.

There's two options there:
- update the database directly, but then if the configuration storage is not in the database, you're in trouble.
- don't fire the event

I've opened #2538228: Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly for that specific issue. I'd missed it on the block upgrade one, and there's nothing that can be done without an API change/addition on that issue.

I've had this in the back of my mind for a while, but slightly reversed from the issue summary. There is a further issue which the block one doesn't really cover, which is what happens when you update the structure of the routing table or something else that is required for update.php itself to get served (and rebuild.php doesn't fix it).

- add hook_upgrade_N for the type 1 update. Run this from a front controller with only a very minimal container (or at an event dispatcher and module handler that are null implementations.

Then redirect via link or 302 to update.php.

- leave hook_update_N() exactly as it is for type 2 updates.

Although this will run into the same problems that #2001310: Disallow firing hooks during update did in the sense that page rendering itself relies on the event system, so would probably have to swap out the container only when running the updates themselves or something.

Why is this not a critical given this part of the issue summary?

In #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager), we addressed this by creating a block_update_8001() for the format change and block_update_8002() for the value change. Which works great, until some future time when we have a block_update_8003() that performs a schema/structure/format change. Because at that time, if a site updates straight from beta-12 to the latest codebase, then block_update_8002() will run and fire hooks on code that likely requires the changes in block_update_8003(), but that update function hasn't run yet.

Looks like there is no workaround without modifying core and the existing update functions will cause problems in later releases, no?

I'm not sure that description is still correct because we are now not using entity API anymore in 8002 but just config save and flag it as schema-trusted data.

So we don't care if 8003 is going to change the schema and we only invoke config save events, which need to be able to handle a change like this: #2538228: Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly.

That said, @catch was thinking about making this critical I think ;)

Why is the existing update dependencies system incapable of solving this problem? Just make 8003 a pre-requisite to 8002, and done? I'm confused how we got through 80+ hook_update_N()s in 7.x core alone, let alone all of 7.x contrib's updates, without this API.

@webchick afaik you can't make 8003 a dependency of 8002 with update dependencies - module updates have to run sequentially. Otherwise the schema version would get set to 8003, and 8002 shouldn't run. It'd be interesting to try, but could cause more problems i think.

I posted on one of the issues about emptying 8002 and moving it to 8004, then adding hook_update_dependencies() alter to make anything that depends on 8002 depend on 8003 instead. I think that is viable as a workaround but it's extremely fragile/complex even if it actually works.

I'm confused how we got through 80+ hook_update_N()s in 7.x core alone, let alone all of 7.x contrib's updates, without this API.

During the ~6 months prior to 7.0, we slowly built a parallel API for modules like the field system to bypass what would now be schema validation/config dependencies and config entity CRUD hooks. https://api.drupal.org/api/drupal/modules%21system%21system.api.php/grou...
Those examples are all for things that would now be config entities. They were all one-offs when core hook_update_N() blew up trying to use the normal API functions, found in issues like #895386: Clean-up the upgrade path: taxonomy.

At one point we were going to have multiple versions of those _update_7000_ functions I think (to reflect different schema versions during beta), but iirc they got consolidated prior to the 7.0 release candidate - i.e. we kept them at _update_7000_ and kepts rewriting them up until relesae.

We have a similar API in Drupal 8 to the _update_7000 ones, which is loading/saving raw config objects in updates instead of the config entities, and we used that in [#10141254]. However this means you can't change a config entity value, and have it work the same as it would if it was done via the UI or a config sync.

So we are effectively saying to people "don't use config entities in your updates" in both 7.x and 8.x at the moment - the API is more or less the same in terms of updates since we've not had to use it for nearly two years.

However, actual Drupal 7 sites never use _update_7000_create_field() and similar, they always use field_create_*(), for two reasons:

1. No-one knows about or understands those _update_7000_ functions except for the handful of people that spent months of their lives trying to get 7.x out of the door.

2. In practice, you have to use field_create_*() for normal run-of-the-mill update handlers, because if you don't then things break horribly. Modules like field collection really need the hooks that field_create_field() invokes to function properly; http://www.drupalcontrib.org/api/drupal/drupal!modules!field!field.api.p...

If Drupal 7 ever changes the schema of the {field_config} or {field_config_instance} table, every module with a field_create_* in a hook_update_N() will have to add an update dependency retrospectively so that it runs after the schema change. This more or less means that 7.x can never make that kind of change again.

And then Drupal 7 has separate APIs for each kind of config entity, whereas 8.x we have a single, extensible API where you can do things like change the config schema for fields from a contrib module, which makes it much more likely that changes will happen, even if core doesn't.

The goal of this issue would be to not have to worry about any of that - contrib modules would create/update fields and instances in hook_update_entity_N() and it should always work regardless of what other core or contrib updates are doing.

Given that:

1. We are saying it's not OK to use entity APIs in update functions in #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager) and #2538228: Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly.

2. We know that the Drupal 7 APIs those were ported from are used massively in 7.x update functions and relied on by major contrib modules (http://www.drupalcontrib.org/api/drupal/drupal!modules!field!field.api.p...)

I'm bumping this to critical. We can decide to not do this and deal with fallout of trying to make do with hook_update_N() by itself again, but we should actually make that decision and not just let it drift.

Discussed this with Alex Pott at some length.

I think he convinced me that this is a major bug in the same sense that #2547507: Consider adding hook_update_early_N() support is. i.e. it makes particular updates extremely complicated if not impossible.

For example #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager) probably 30-40% of the work and discussion could have been avoided if we'd had this API in place.

However like the early updates, this really becomes critical when we have a core update that needs it, or it becomes a contrib blocker when a contrib module needs it.

At that point we'll either have to delay that core issue or contrib module until a minor release that includes this, or add the feature to a patch-level release.

Both of those are very risky, but it's possible to defer this until after release, assuming we don't need such a core update before release candidate.

This could be an issue we explicitly deem an "RC target" (or whatever the tag was) to help assuage your concerns. Not bad enough to hold RC or even release, but it could be whitelisted to be committed during RC if it's ready.

Yes it's also something that by definition doesn't affect runtime code. So while it's 'disruptive' in the sense of a new best practice, the risk of it introducing a functional regression during RC is very minimal.

My main concern in terms of release management with this is that a horrible issue comes in during RC which requires either this or the early update API. Then we not only have to fix that new issue, but either or both of those issues too as a pre-requisite.

So it's a straight gamble whether deferring this actually gets the release out faster or not - i.e. if we'd had it before the block context upgrade path one, that would have been a lot quicker/simpler.

The equivalent of that issue during release candidate could end up being a 4 week issue instead of a 1 week - either because we bump this to critical as a blocker or have to do another two-updates-with-a-gap-in-the-middle-with-circular-discussions-RTBC-10-times-workaround-then-still-wrong-even-after-all-that (see #2540546: Add a test implementation of the contrib update path for #2528178 for the wrongness) dance to get it done without it.

If people are completely aware of that risk and understand it, then as above I think it's OK as a major - since it doesn't block an actual upgrade path yet.

I think we might have a use-case... #2464427-170: Replace CacheablePluginInterface with CacheableDependencyInterface

I moved the upgrade path of #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface to #2553381: Add upgrade path for 2464427.

Sorry for the noise.

Started to have a look at it. Its tricky, because once we have a splitted update process we also have to think about two schema versions.
An alternative could be that the dependencies of hook_update_N() could be used to be placed after the drush_flush_all_caches() call.

Talked to alexpott in irc about the double schema a few days ago.

An option would be to not follow hook_update_N() pattern at all, since we don't really need to track schema version here, just whether an update has run or not.

So we could have an interface for updates, and annotate them with an 'update ID' or something, then store those that have run in a k/v collection so they don't run twice.

This only works if we absolutely disallow dependencies between late updates.

I'm not sure it's easier, it might be better - especially if we also add the early update API then have three different schema versions to track - would be more different from the other two update APIs then.

The model I had in mind was https://www.drupal.org/sandbox/catch/2337361 - this was an answer to custom site builds having lots of conflicting hook_update_N(). We took them out of the update system into hook_install() for one-off modules, then ran them with drush updb - and you could run either before or after hook_update_N() itself.

So do #8 & #11 mean #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface can continue as-is, i.e. can it continue to use the APIs? If so, we can unpostpone it.

Otherwise, like #12 asks/indicates, #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface is a use case for this, does that mean this needs to become critical again?

We're going to make this critical because of configuration synchronisation :( otherwise we'll have updates that will never look imported causing users a severe case of head-scratching and "is my site broken issues".

For example:

1. A user has a dev and prod site we make a change like #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface
2. The user applies the change to both dev and prod and runs update.php everywhere.
3. After this change the active config is not in agreement with the active view... View::load($id)->save() - would change the config to add the new cacheability meta data. However we've not done that in the upgrade path.
4. The user then exports all the configuration on dev including a new view created before the upgrade.
5. The user imports this configuration to prod.
6. The config system will save the view and in the process apply to the new cacheability metadata and then the user will see a difference between staging and active configuration that no amount of pressing "import all" will resolve.

I'm not sure whether for now we should switch to something annotation based, this feels just like quite a different API.
It would be enough and a bit bitter as more similar to the existing hook_update_N experience to just store the executed hook_update_data() (or how they'll be named) calls and be done with it.

Started to code something up for that. (the work is based upon earlier work on hook_update_early_N)

There's two options there:
- update the database directly, but then if the configuration storage is not in the database, you're in trouble.
- don't fire the event

Even if we configured a NULL event dispatcher, the first case could still cause troubles. And this is true also for content entity storage: as soon as an update function tried to write directly to a SQL database, it could get an error. I'm wondering whether we should provide also an API to replace update function implementations. For instance:

/**
 * @file node.install
 */

/**
 * Fix yet another Entity Schema issue.
 */
function node_update_8007() {
  // Do SQL-specific stuff.
}

would be replaced by

/**
 * @file fancy_xml_entity_storage.install
 */

/**
 * Implements hook_update_implementations_alter().
 */
function fancy_xml_entity_storage_update_implementations_alter(array &$implementations) {
  $implementations['node_update_8007'] = 'fancy_xml_entity_storage_node_update_8007';
}

/**
 * Fix yet another Entity Schema issue.
 */
function fancy_xml_entity_storage_node_update_8007() {
  // Do XML-specific stuff.
}

@plach
I fear this is an additional problem, now we made everything swappable.

For #23 I think there's a case to be made for an in-place entity field migration system.

So you could do something like:

Copy field A to temporary field A1.

Drop the data from field A.

Update the schema for field A to multiple or whatever.

Load and save the data from A1 to A (using some part of the entity API, not raw SQL, in an update hook)

Drop temporary field A1.

If we eventually did that and got it right, then it's going to work better than manual SQL query method - both in terms of storage agnosticism and because we'd only be writing the batch (and etc.) handling once instead of dozens/hundreds of times in contrib/custom modules. I don't think any of that is release blocking though or that we need even a plan for it here, but a new major to discuss more could be good.

Some progress

• Adds a test
• Fixed code due to some problems related with that
• Execute the actual functions
• We certainly need stuff like the available updates reporting

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -575,6 +576,17 @@ protected function triggerBatch(Request $request) {
+    // Now we rebuild all caches and after that execute the hook_update_after()
+    // functions.
+    $operations[] = ['drupal_flush_all_caches', []];

+++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
@@ -0,0 +1,116 @@
+}

I think we should only add this if there are after updates since we flush in DbUpdateController::batchFinished()

Some initial feedback.

1. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,131 @@
   + * Provides all and missing hook_update_data_N implementations.
   

   This class could use better documentation. Here in the class description, hook_update_data_N is referred to, but later it searches for update_after.

2. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,131 @@
   +  protected $updateName = 'update_after';
   

   Does this need to be a variable, or will a constant work?

3. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,131 @@
   +   * Gets the non already executed update functions.
   

   How about 'Find all update functions that haven't been executed'?

4. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,131 @@
   +    $existing_update_functions = $this->keyValue->get('core.update__' . $this->updateName, []);
   

   I don't see anyplace where this is actually set or updated.

Once this is more finalized, an entry in update.api.php will be needed too.

yeah this is indeed still in its early stages, will work on it.

This is looking good. Its tempting to keep going here, but I realise we need something that works and can look at niceties later. So in that regard, some of my comments are blue-sky thinking.

1. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,131 @@
   + * Provides all and missing hook_update_data_N implementations.
   

   needs cleanup

2. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,131 @@
   +  protected $updateName = 'update_after';
   

   So in theory this could also process hook_update_N in future?
   I'd be willing to put in some work here to make it also use some folder based discovery of OO updates, instead of functions. 95% of the merge conflicts I encounter on a daily basis are between update hooks in .install files. Would be great if we could get a one per file discovery to avoid that in the future, although wouldn't hold up release on that.

3. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,131 @@
   +    // We don't support install profiles at that point?
   

   We should support profiles. 95% of the sites I build have 100+ update hooks in the install profile.

95% of the merge conflicts I encounter on a daily basis are between update hooks in .install files. Would be great if we could get a one per file discovery to avoid that in the future, although wouldn't hold up release on that.

+1 to this, would be awesome, open an issue?

+++ b/core/modules/system/tests/modules/update_test_after/update_test_after.update_after.inc
@@ -0,0 +1,37 @@
+ * Implements hook_update_after().

update_after solves our problem here but isn't very future proof, if we ever have a similar issue, we can't exactly have update_after_after. Personally I think a name would be better such as hook_update_data or hook_update_content etc.

Thank you for the quick feedback!

Does this need to be a variable, or will a constant work?

Good question ...

This class is supposed to be potentially used for #2547507: Consider adding hook_update_early_N() support as well

I don't see anyplace where this is actually set or updated.

Ha, yeah that was indeed missing.

I'd be willing to put in some work here to make it also use some folder based discovery of OO updates, instead of functions. 95% of the merge conflicts I encounter on a daily basis are between update hooks in .install files. Would be great if we could get a one per file discovery to avoid that in the future, although wouldn't hold up release on that.

Well, I think we should keep at as straightforward as possible for now. Encapsulating things in an API makes it possible to change things later.

We should support profiles. 95% of the sites I build have 100+ update hooks in the install profile.

Sure, let's add that.

update_after solves our problem here but isn't very future proof, if we ever have a similar issue, we can't exactly have update_after_after. Personally I think a name would be better such as hook_update_data or hook_update_content etc.

Yeah I agree hook_update_after() potentially let people think that its about running after a specific one. I would go with hook_update_data() or even hook_update_entity()

Some work.

Worked on the rename.

Looks great, especially tests! I found only nits:

1. +++ b/core/includes/update.inc
   @@ -219,6 +219,58 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   + * Executes a single hook_post_update_$name().
   

   We usually express these variable hook names with uppercase text hook_post_update_NAME.

2. +++ b/core/includes/update.inc
   @@ -219,6 +219,58 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   +function update_post_update_do_one($function, &$context) {
   

   What about update_post_update_invoke? We are talking about hooks after all.

3. +++ b/core/includes/update.inc
   @@ -219,6 +219,58 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   +      $key_value_store = \Drupal::keyValue('update_test_post_update');
   

   test?

4. +++ b/core/includes/update.inc
   @@ -219,6 +219,58 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   +  $context['message'] = 'Updating content after: ' . Html::escape($function);
   

   No translation here?

5. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -172,7 +180,7 @@ public function scan($type, $include_tests = NULL) {
   +      $searchdirs[static::ORIGIN_SITE] = isset($this->sitePath) ? $this->sitePath : DrupalKernel::findSitePath(Request::createFromGlobals());
   

   Can we use ?: to simplify this?

6. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -180,7 +188,7 @@ public function scan($type, $include_tests = NULL) {
   +      $include_tests = Settings::get('extension_discovery_scan_tests') || drupal_valid_test_ua();
   

   Is this a performance improvement?

7. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,41 @@ function hook_update_N(&$sandbox) {
   + * Note: In contrast to hook_update_N there is no enforced order, so you can
   ...
   + * @see hook_update_N
   

   Missing () after hook_update_N.

8. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,41 @@ function hook_update_N(&$sandbox) {
   + *   PDOException.ame for the name of those updates.
   

   oops

9. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,41 @@ function hook_update_N(&$sandbox) {
   +function hook_post_update_X(&$sandbox) {
   

   Can we use hook_post_update_NAME here?

10. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    + * Provides all and missing hook_$updateName_X implementations.
    + *
    + * It therefore scans for functions in $module.$updateName.inc named like
    + * $module_$updateName_X with X being a machine name.
    

    As you may have guessed, I'd consistently use hook_post_update_NAME() :)

11. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    +    $regexp = '/^(?<module>.+)_' . $this->updateName . '_(?<version>.+)$/';
    ...
    +        $updates[] = $matches['module'] . '_' . $this->updateName . '_' . $matches['version'];
    

    Can we call this name instead of version?

12. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    +   * Find all update functions that haven't been executed
    

    Missing trailing dot.

13. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    +    // First figure out which hook_update got executed already.
    

    hook_post_update_NAME() :D

14. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    +  protected function loadUpdateFiles(array $module_extensions) {
    

    Missing PHP doc

15. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    +   * Loads the {$this->updateName}.inc file for a given extension.
    

    The file name has changed, can we make the PHP doc implementation-agnostic?

16. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    +   * Returns a list of all the pending database updates.
    ...
    +  public function getMissingUpdateList() {
    

    These are slightly confusing, can we mention post updates, instead?

17. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    +   *   going to proceed due to missing requirements. The system module will
    +   *   always be listed first.
    

    Do we need to specify this? If we are not going to provide dependencies, it looks misleading to me.

18. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,169 @@
    +   *   The subarray for each module can contain the following keys:
    +   *   - start: The starting update that is to be processed. If this does not
    +   *       exist then do not process any updates for this module as there are
    +   *       other requirements that need to be resolved.
    +   *   - pending: An array of all the pending updates for the module including
    +   *       the update number and the description from source code comment for
    +   *       each update function. This array is keyed by the update number.
    

    This doc seems to be inherited from hook_update_N(), can we not mention numbers and any logic involving dependencies?

19. +++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
    @@ -0,0 +1,190 @@
    +    parent::setUp(); // TODO: Change the autogenerated stub
    

    hm

20. +++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
    @@ -0,0 +1,190 @@
    +    $key_value->get('core.update__post_update', [])->wilLReturn([]);
    ...
    +    $key_value->get('core.update__post_update', [])->wilLReturn(['module_a_post_update_a']);
    ...
    +    $key_value->get('core.update__post_update', [])->wilLReturn([]);
    ...
    +    $key_value->get('core.update__post_update', [])->wilLReturn(['module_a_post_update_a']);
    

    will

hook_post_update_X() sounds similar to hook_update_N(), can we use hook_post_update_NAME()?

What about update_post_update_invoke? We are talking about hooks after all.

Note, regular updates use update_do_done()

No translation here?

You are right, well, let's fix update_do_one() as well.

Can we use ?: to simplify this?

Good point

Is this a performance improvement?

Its enabling of unit tests without having to deal with drupal_valid_test_ua(), much like other changes here.

Do we need to specify this? If we are not going to provide dependencies, it looks misleading to me.

Yeah, I think its also just wrong.

This doc seems to be inherited from hook_update_N(), can we not mention numbers and any logic involving dependencies?

Yeah its just a copy & paste plus some improvements

will

OH PHP

This doc seems to be inherited from hook_update_N(), can we not mention numbers and any logic involving dependencies?

adapted the comment.

What about update_post_update_invoke? We are talking about hooks after all.

Went with update_invoke_post_update, as it doesn't result into the same naming scheme as update_POST_UPDATE_NAME

Looking at the test failures now.

one small fix.

Reviewed:

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,41 @@ function hook_update_N(&$sandbox) {
   + * Executes an update which is intended to update data, like entities.
   

   It is not saying anywhere that these need to be going into a special named PHP file as far as I see?

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,41 @@ function hook_update_N(&$sandbox) {
   + * Note: In contrast to hook_update_N there is no enforced order, so you can
   

   hook_update_N()

3. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,41 @@ function hook_update_N(&$sandbox) {
   + *   Optionally, update hooks may return a translated string that will be
   

   Do we have a term for them other than "update hooks"? That sounds hard to differentiate if we don't have our own terms.

4. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,175 @@
   + * Provides all and missing hook_$updateName_NAME implementations.
   

   all and missing? not sure I get what you meant here...

It is not saying anywhere that these need to be going into a special named PHP file as far as I see?

Strong point!

Do we have a term for them other than "update hooks"? That sounds hard to differentiate if we don't have our own terms.

Just used "post_update_NAME hooks", I think this makes it more clear.

all and missing? not sure I get what you meant here...

Well, that particular class is written in a way that its useable for #2547507: Consider adding hook_update_early_N() support

Let's also fix the remaining failures ...

This is looking very solid now. The tests are great too.

On the new documentation though

+++ b/core/lib/Drupal/Core/Extension/module.api.php
@@ -660,6 +660,43 @@ function hook_update_N(&$sandbox) {
+ * Note: In contrast to hook_update_N() there is no enforced order, so you can
+ * also use machine names for the name of those updates.

If I'm reading this right, NAME is completely arbitrary, and is for code organization/clarity. I think this should be explicitly stated, possibly with an example function name or two. Also, "there is no enforced order" doesn't explain what order they will be run in (which I understand to be the order they are declared in).

Just a comment, feel free to ignore. I wonder if we want to handle this the other way with a "pre" hook and call it hook_update_data_N(). It's clear then that you want data updates to go there, vs. I am really not sure what to put in hook_post_update_N().

We discussed hook_update_data_N() on the call and generally thought 'data' was not very descriptive - since all updates update some kind of data. With the semantic distinction in core we're making between data structures and data values it gives you an idea, but anything in a database is 'data'.

Whereas hook_post_update_X() describes the fact it runs after hook_update_N(), and if we add the early updates, they can be hook_pre_update_X(). It's not amazingly descriptive but it's a start.

Also discussed possibly having a major issue to add an OOP version of this (called something else) in a minor release, at which point we could deprecate the hook we're adding here.

Fair enough! Thanks.

Updated some documentation which tries to fix #49

Also updated the issue summary

This looked ready to me, so I did a few manual tests and I found some problems:

• The status report page is not displaying any error when only post updates are pending.
• The updates summary page looks broken and does not list regular updates (note the mismatch between the number of updates and the descriptions):
  
• The performed post updates are being stored multiple times in the state and are always listed on the selection page. However after starting the update process they are not run, and only post updates not already executed are processed, which is the expected behavior.

I have also one question: if a module is installed and its code defines some post updates, should they be marked as executed as we do for regular updates? I'd say we do, otherwise we might end-up performing unintended changes to the site data. The current behavior seems to execute post updates even after install, so if we agree, we need to fix also this.

Aside from that, I thought @dawehner would appreciate a second round of nitpicks:

1. +++ b/core/includes/update.inc
   @@ -215,7 +215,60 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   +      // @TODO We may want to do different error handling for different
   +      // exception types, but for now we'll just log the exception and
   +      // return the message for printing.
   

   The @TODO needs to be qualified or simply removed :)

2. +++ b/core/includes/update.inc
   @@ -215,7 +215,60 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   +      watchdog_exception('update', $e);
   

   Maybe update_post_update or just post_update?

3. +++ b/core/includes/update.inc
   @@ -215,7 +215,60 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   +        'query' => t('%type: @message in %function (line %line of %file).', $variables)
   

   [ubernitpick] missing trailing comma

4. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -91,14 +91,29 @@ class ExtensionDiscovery {
   +   *   The available profile directories
   

   Missing trailing dot.

5. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,47 @@ function hook_update_N(&$sandbox) {
   + * These implementations have to be placed in a $module.post_update.php file.
   

   I think I would expect MODULE.post_update.php here.

6. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,47 @@ function hook_update_N(&$sandbox) {
   + *   Drupal\Core\Utility\UpdateException with a meaningful message for the user.
   

   Missing leading backslash for UpdateException.

7. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,47 @@ function hook_update_N(&$sandbox) {
   + *   Optionally, post_update_Name hooks may return a translated string that will
   

   hook_post_update_NAME() also here :)

8. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,175 @@
   + * Provides all and missing hook_$updateName_NAME implementations.
   + *
   + * It therefore scans for functions in $module.$updateName.inc named like
   + * $module_$updateName_NAME with NAME being a machine name.
   

   Can we use hook_post_update_NAME() also in the three instances here? Also, the file extension is wrong, should be MODULE.post_update.php.

9. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,175 @@
   +  public function __construct($root, $site_path, array $enabled_modules, KeyValueStoreInterface $key_value, $include_tests = NULL) {
   

   Missing PHP doc

10. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,175 @@
    +   * @return callable[]
    
    +++ b/core/modules/system/src/Controller/DbUpdateController.php
    @@ -589,6 +615,15 @@ protected function triggerBatch(Request $request) {
    +   * @return \Drupal\Core\Update\UpdateRegistry
    

    The two return descriptions are missing.

11. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,175 @@
    +    // First figure out which hook_{$this->updateName}_NAME got executed already.
    

    80 chars

12. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
    @@ -0,0 +1,175 @@
    +   * Loads the {$this->updateName}.php file for a given extension.
    

    I didn't explain myself correctly earlier, can we just say something generic like "Loads the post update file for a given extension."?

13. +++ b/core/modules/system/src/Controller/DbUpdateController.php
    @@ -257,35 +258,48 @@ protected function selection(Request $request) {
    +          $after_update_registry = $this->getPostUpdateRegistry();
    
    @@ -575,6 +589,18 @@ protected function triggerBatch(Request $request) {
    +    $after_update_registry = $this->getPostUpdateRegistry();
    

    $post_update_registry

14. +++ b/core/modules/system/tests/fixtures/update/drupal-8.update-test-postupdate-enabled.php
    @@ -0,0 +1,39 @@
    + * Partial database to mimic the installation of the update_test_post_update module.
    

    80 chars?

#54 made me realize another potential issue: what happens if a module has post update functions to run, but one of its hook_update_N functions fail? Will all the post update hooks be run? They probably shouldn't, but that raises the question: should any module post update hooks run if there are pending updates from any other module?

The UI issues raised in #54 can be tested for--I'll write some of those, and work on a test case to illustrate #55.

This adds tests for not running post update hooks if there are still pending updates (and a fix for that). I also added UI tests, and fixed the issue where normal updates were no longer displayed. I removed the bit of the code that was trying to add the function name as an HTML comment (if this is needed, we need to figure out how to not get that escaped).

I did not address the duplicate storage of post update hooks, or any of the nits from #54.

I also updated the IS to note we'll need a corresponding PR/issue in the drush queue.

First, I 100% agree that we should not execute any hook_post_update_NAME() implementations, because we don't know the state of the system at that point.

I think we can skip it better, #abort is set, so we can leverage that.

We are adding a new API so this needs a change notice.
Some more naming bikeshedding review.

1. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -660,6 +660,47 @@ function hook_update_N(&$sandbox) {
   +  return t('Node 123 saved');
   

   This should be t('Node %nid saved.', ['%nid' => 123])

2. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,175 @@
   +class UpdateRegistry {
   
   +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -589,6 +615,15 @@ protected function triggerBatch(Request $request) {
   +  protected function getPostUpdateRegistry() {
   +    return new UpdateRegistry($this->root, \Drupal::service('site.path'), array_keys($this->moduleHandler()->getModuleList()), \Drupal::keyValue('post_update'));
   

   Isn't it PostUpdateRegsitry?

3. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,175 @@
   +   * Find all update functions that haven't been executed.
   ...
   +  public function getMissingUpdateFunctions() {
   

   This funtion namr does't explain the desc of the function.How about getNonExecutedFunctions?

4. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -0,0 +1,175 @@
   +   * Returns a list of all the pending updates.
   ...
   +  public function getMissingUpdateList() {
   

   How about getPendingUpdates() or getPendingUpdateList()?

5. +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -257,35 +258,48 @@ protected function selection(Request $request) {
   +          $after_update_registry = $this->getPostUpdateRegistry();
   
   @@ -575,6 +589,18 @@ protected function triggerBatch(Request $request) {
   +    $after_update_registry = $this->getPostUpdateRegistry();
   

   The var should be called $post_update_registry.

Working on the CR now.

This should be t('Node %nid saved.', ['%nid' => 123])

Fixed that part.

Isn't it PostUpdateRegsitry?

Nope, the class is named UpdateRegistry and should be, as its written in a generic way.

This funtion namr does't explain the desc of the function.How about getNonExecutedFunctions?

Strong point! What about using getPendingUpdateFunctions() and getPendingUpdateInformation() for some more consistency?

The var should be called $post_update_registry.

Fixed it.

Strong point! What about using getPendingUpdateFunctions() and getPendingUpdateInformation() for some more consistency?

+1 to that.

Should we convert the second part of the block context update to use this?

Given that hook_update_N() has dependencies and hook_post_update_NAME() doesn't and the idea is to be able to do stuff in between block_update_8001() and block_update_8002()
I think converting doesn't make sense.

You can depend on 8001 and be guaranteed it runs before post_update so seems better than having to fit between 8001 and 8002. Plus we can use entity api to save the block config entities.

Good point, we know that the post_updates are executed always later. Let's update the docs also a bit, to make that clear.

I am under the impression that this won't solve the issue of updates with an alternate non-SQL storage (e.g. MongoDB / CouchDB).

@fgm this is unrelated to alternative storage as such, but it does provide a place where you can legitimately use the entity api to update content values, which is storage agnostic at least.

I am under the impression that this doesn't try to solve the issues of updates with an alternative non-SQL storage (e.g. MongoDB / CouchDB),
see #2538108-24: Add hook_post_update_NAME() for data value updates to reliably run after data format updates

Btw. things worked pretty fine until we really tested it :P
It turns out we need to ensure that we don't run the updates after a module installation ...

Fixed the remaining failures

There we go.

I tested this again: the status report item is still missing, which is bad because atm users have no clue that post updates are pending, unless also regular updates are.

I found a couple of minor UI glitches:

• It would be nice to group updates and post updates belonging to the same module together:
  
• Can we find a better way to display this summary for post updates?
  

Another code review:

1. +++ b/core/includes/update.inc
   @@ -215,7 +215,68 @@ function update_do_one($module, $number, $dependency_map, &$context) {
   +      $key_value_store = \Drupal::keyValue('update__post_update');
   

   The key value is wrong here: this implies that updates are never registered as executed and thus are re-executed every time update.php is run. We need better test coverage. Aside from that, I think it would make sense to add a static method (or another mechanism) to encapsulate the instantiation of the KV collection. This would have prevented this problem in the first place.

2. +++ b/core/modules/block/block.install
   @@ -95,52 +96,5 @@ function block_update_8001() {
   -function block_update_8002() {
   

   It would be better to leave an empty placeholder function to ensure the next update function will be 8003.

3. +++ b/core/modules/block/block.post_update.php
   @@ -0,0 +1,61 @@
   +  $block_update_8001 = \Drupal::keyValue('update_backup')->get('block_update_8001', []);
   

   Can we add an early return if the block schema is 8002 already?

4. +++ b/core/modules/system/src/Tests/Module/InstallTest.php
   @@ -53,6 +53,19 @@ public function testRequiredModuleSchemaVersions() {
   +  /**
   +   * Ensures that post update functions are removed on uninstall.
   +   */
   +  public function testUninstallPostUpdateFunctions() {
   +    $post_update_key_value = \Drupal::keyValue('post_update');
   +    $existing_updates = $post_update_key_value->get('existing_updates', []);
   +    $this->assertTrue(in_array('module_test_post_update_test', $existing_updates));
   

   Am I missing something or we are missing all the uninstallation assertions?

Also, it seems all my precious nitpicks from #54 were not addressed :(

It would be nice to group updates and post updates belonging to the same module together:

Grouped them together now, see screenshot.

Also, it seems all my precious nitpicks from #54 were not addressed :(

Mh, I thought @jhedstrom would have fixed it, sorry

The @TODO needs to be qualified or simply removed :)

Its the same as update_do_one()

Maybe update_post_update or just post_update?

I went with 'update', because collecting post_update messages and update messages together makes it easier to look at all of them. Remember, for the normal user, its not obvious that they are separated.

Can we use hook_post_update_NAME() also in the three instances here? Also, the file extension is wrong, should be MODULE.post_update.php.

Yeah I'm still not a fan in tighten this class docs to a specific update type, given that this class will be reused in the future.

It would be better to leave an empty placeholder function to ensure the next update function will be 8003.

strong point!!

Am I missing something or we are missing all the uninstallation assertions?

Yeah you are right.

The key value is wrong here: this implies that updates are never registered as executed and thus are re-executed every time update.php is run. We need better test coverage. Aside from that, I think it would make sense to add a static method (or another mechanism) to encapsulate the instantiation of the KV collection. This would have prevented this problem in the first place.

This statement allowed me to think a bit about it and resulted into registering the registry into a service.

We need an actual way that we just skip the post_update if the actual update 8002() was executed. Let's leverage a state entry for that.

Reroll

This is starting to look very good :)

However...
the status report is still not displaying updates and uninstallation is not working properly, I still see the entries in the KV after uninstalling the module. I have also a few more code complaints:

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -260,6 +261,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        $post_update_key_value = \Drupal::keyValue('post_update');
   
   @@ -445,6 +452,11 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +      $post_update_key_value = \Drupal::keyValue('post_update');
   

   Can we use the factory also here?

2. +++ b/core/includes/update.inc
   @@ -242,14 +242,11 @@ function update_invoke_post_update($function, &$context) {
   +    // @TODO We may want to do different error handling for different
   +    // exception types, but for now we'll just log the exception and
   +    // return the message for printing.
        catch (Exception $e) {
   

   Let's qualify them anyway, I created the following issue: #2564311: Consider doing different exception handling besides logging while applying updates. Also, not wrapping at 80 chars.

3. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -679,12 +679,12 @@ function hook_update_N(&$sandbox) {
   + *   Optionally, post_update_NAME() hooks may return a translated string that will
   

   Would it make sense to write "hook_post_update_NAME()" here?

4. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -14,8 +14,8 @@
     * Provides all and missing hook_$updateName_NAME implementations.
     *
   - * It therefore scans for functions in $module.$updateName.inc named like
   - * $module_$updateName_NAME with NAME being a machine name.
   + * It therefore scans for functions in MODULE.$updateName.php named like
   + * MODULE_$updateName_NAME with NAME being a machine name.
   

   I see the point of not explicitly mentioning post updates here. I guess in the future we could use this class (or a subclass) for regular updates and pre updates. However those variables are not very readable in docs. Would it work for you if we just avoided them (also in the first line) and just generically mentioned "update functions"?

5. +++ b/core/lib/Drupal/Core/Update/UpdateRegistryFactory.php
   @@ -0,0 +1,30 @@
   +  public function configure() {
   

   This is a weird name for a factory method, is it a standard already introduced elsewhere? Can we use ::create() or something similar otherwise?

6. +++ b/core/modules/block/block.install
   @@ -99,6 +99,7 @@ function block_update_8001() {
   +  \Drupal::state()->set('block_update_8002', TRUE);
   
   +++ b/core/modules/block/block.post_update.php
   @@ -20,10 +20,18 @@ function block_post_update_disable_blocks_with_missing_contexts() {
   +  if ($module_schema >= 8002 && !\Drupal::state()->get('block_update_8002', FALSE)) {
   

   I think block_update_8002_placeholder would make the code easier to follow.

7. +++ b/core/modules/block/block.post_update.php
   @@ -20,10 +20,18 @@ function block_post_update_disable_blocks_with_missing_contexts() {
   +  // The state entry 'block_update_8002' is used in order to indicate that
   +  // block_update_8002() would have been executed, so this function needs to be
   

   Can we change this to: "The state entry 'block_update_8002_placeholder' is used in order to indicate that the placeholder block_update_8002() function has been executed"? I think it would also help to figure out what's going on.

I think we are very close now :)

Cleaned up the change record draft significantly. I read the patch and updated parts of the change notice where I did not understand what they meant :) I think the change record looks good to go now. https://www.drupal.org/node/2563673/revisions/view/8851141/8855673

I forgot to add the follow-up issue in #84.2, did it now.

Let's qualify them anyway, I created the following issue:

LOL

Would it make sense to write "hook_post_update_NAME()" here?

Sure, let's do it.

This is a weird name for a factory method, is it a standard already introduced elsewhere? Can we use ::create() or something similar otherwise?

Sure

I see the point of not explicitly mentioning post updates here. I guess in the future we could use this class (or a subclass) for regular updates and pre updates. However those variables are not very readable in docs. Would it work for you if we just avoided them (also in the first line) and just generically mentioned "update functions"?

I hope this version is a bit better.

Note: I haven't found the motivation to exeucte InstallUninstalTest yet

No promise, as devel module is failing locally atm. due to schema issues.

Manual testing results are great now, I found only one minor thing: I renamed (actually I prepended an underscore to) one post_update function to avoid having it registered when installing my test module, but after doing that this is what the UI displayed:

I guess that when looking for post update functions we need to check that the prefix is actually a module name.

Let's qualify them anyway, I created the following issue:

LOL

:)

Now for reals: #2564311: Consider doing different exception handling besides logging while applying updates. Also, comments are not wrapping at 80 chars.

Hopfully, my last code-related remarks:

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -445,6 +451,15 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +      $post_update_key_value = \Drupal::keyValue('post_update');
   +      $post_update_registry = new UpdateRegistry($this->root, \Drupal::service('site.path'), array_keys($this->moduleHandler->getModuleList()), $post_update_key_value);
   +      $module_post_update_functions = $post_update_registry->getModuleUpdateFunctions($module);
   +      $post_update_key_value->set('existing_updates', array_diff($post_update_key_value->get('existing_updates', []), $module_post_update_functions));
   +
   

   I guess these lines can be removed now.

2. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -684,7 +684,7 @@ function hook_update_N(&$sandbox) {
   + *   Optionally, hook_post_update_NAME() hooks may return a translated string that will
   

   80 chars

3. +++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
   @@ -12,14 +12,22 @@
   - * Provides all and missing hook_$updateName_NAME implementations.
   + * Provides all and missing update implementations.
     *
   - * It therefore scans for functions in MODULE.$updateName.php named like
   - * MODULE_$updateName_NAME with NAME being a machine name.
   + * Note: This registry is specific to a type of updates, like 'post_update' as
   + * example.
   + *
   + * It therefore scans for functions named like the type of updates, so it looks
   + * like MODULE_UPDATETYPE_NAME() with NAME being a machine name.
   

   +1, thanks!

4. +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   @@ -188,6 +192,46 @@ protected function assertSuccessfulUninstall($module, $package = 'Core') {
   +   * Ensures the module post update functions after install.
   

   This sounds weird, but maybe it's just me.

5. +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   @@ -188,6 +192,46 @@ protected function assertSuccessfulUninstall($module, $package = 'Core') {
   +  protected function assertModuleUpdates($module) {
   

   Can we rename this to assertInstallModuleUpdates for consistency? :)

6. +++ b/core/modules/system/src/Tests/System/StatusTest.php
   @@ -57,6 +62,23 @@ public function testStatusPage() {
   +    // update_test_postupdate_update_8001() needs to be exeucted.
   

   typo

7. +++ b/core/tests/Drupal/Tests/Core/Update/UpdateRegistryTest.php
   @@ -17,6 +17,8 @@
   + * Note we load code, so we shoudl better run in isolation.
   

   typo

8. +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   @@ -228,10 +228,15 @@ protected function assertUninstallModuleUpdates($module) {
   +        $this->assertFalse(array_intersect(['block_post_update_disable_blocks_with_missing_contexts'], $all_update_functions), 'Ensures that no pending post update functions are available.');
   ...
   +        $this->assertFalse(array_intersect(['block_post_update_disable_blocks_with_missing_contexts'], $existing_updates), 'Ensures that no pending post update functions are available.');
   

   Ensures?

9. +++ b/core/modules/system/system.install
   @@ -611,6 +611,15 @@ function system_requirements($phase) {
   +    if (!empty($missing_post_update_functions)) {
   +      $requirements['update']['severity'] = REQUIREMENT_ERROR;
   +      $requirements['update']['value'] = t('Out of date');
   +      $requirements['update']['description'] = t('Some modules have database schema updates to install. You should run the <a href="@update">database update script</a> immediately.', array('@update' => \Drupal::url('system.db_update')));
   +    }
   

   Can we set a flag if regular updates were found in the loop above, so we don't have to repeat this code twice?

Thank you plach for your quick review cycles!

/me hopes that this was a random failure

+++ b/core/modules/system/system.install
@@ -598,18 +598,31 @@ function system_requirements($phase) {
+    if (empty($requirements['update']['severity'])) {

This will be always true :)

Let's point the TODO to the issue created by @plach

Let's reduce the filesize again.

The patch looks great, I manually tested it multiple times and test coverage is amazing!

@dawehner++

Discussed with @alexpott, @catch, @effulgentsia, and @webchick -- #2212151: Document/test updating active configuration based on API changes (i.e. plugins) can be closed as a duplicate of this issue. However, we also need to add a few lines of documentation with this patch explaining how this API should be used to solve that use case. Example also of where this is needed: #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface

Thanks everyone!

Is something like this enough?

Looks good to me

Wouldn't that be regular hook_update_N() using config API though? It's changing the format of a value.

@catch:

I'm not a Views nor config expert, but I interpreted that code as changing the View's content, not its format. Isn't being able to save entities in a regular environment one of the main points of this API?

I opened https://github.com/drush-ops/drush/issues/1602 for drush support of this new API.

@plach so I think the issue is that the placeholder before the update will be invalid (because we dropped the bc layer) and after the update will be valid (because it's using the new format). In that way it's a 'format update' as opposed to a strict value update.

This could go wrong if another module also has a post_update, if we have validation that the argument format in the Views config matches the new style. Because then when the other update saves the views, they'd fail validation - because this update has not run yet.

In the case of #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface we keep the backwards compatibility layer for the old views, at least in the sense that the view will pass config schema checking, then in the post_update it would literally just load and resave the affected views (which will update the cacheability automatically). In that case the view is valid both before and after the update, it's not quite correct yet, but there's nothing that will fail validation or conflict with another update. If another post_update hook loaded and saved the same view, it might update the cacheability metadata early, but we wouldn't care.

So in the end it comes down to:
1. Do we validate the argument/replacement format at all?
2. Are we ever likely to add validation in the future?

If the answer to either of those is yes or maybe, we should use regular hook_update_N() for that issue, and use #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface or a simplified version as an example here.

#106 makes sense to me but, as I said, I'm not an expert :)

@dawehner:

Please, could you please provide an updated patch or move this back to RTBC depending on the answers to #106.1 and #106.2?

@plach so I think the issue is that the placeholder before the update will be invalid (because we dropped the bc layer) and after the update will be valid (because it's using the new format). In that way it's a 'format update' as opposed to a strict value update.

Well, it is still a valid value (its a string oddly using %1, that doesn't mean anything anymore), it is just not the one you actually want, but you are right, if other hook_post_update_NAME() implementations would deal with that particular value,
you could end up in an issue, but you know, this is the case for all hook_post_update_NAME() issues. Just take for example some update code in block module now. What happens if your hook_post_update_NAME() code just iterates over all enabled blocks, and then do something in page manager with it (just a bad example here). What I want to point out in this particular
comment is that you can easily have kinda implicit dependencies between hook_post_update_NAME() if you just try it hard enough.

So in the end it comes down to:
1. Do we validate the argument/replacement format at all?
2. Are we ever likely to add validation in the future?

As said earlier, its hard to validate / invalidate stuff. We could say that %1 or !1 is invalid, but I could actually at least for the former imagine that it appears for example in rewritten URLs quite easily. In general this comes down to the question whether we will use the validator for config (entities) in some future. There is an issue out there, but its postponed to 8.1

Given that the example is fine for now. Please disagree with me ...

Tossing over to catch for a looksee.

So say Module A added an post-update to 8.x-1.1 of their module following this example.

Module B also adds a post-update which loads and saves views.

Site 1 updates both modules, all goes fine.

Then in 8.1.0 we add validation for the views config entities.

Site 2 updates to 8.1.0 and both modules at the same time, that update starts failing.

So we can't add post updates which are fine now but might not be later.

It's hook_update_N()'s job to get the site into a consistent state where loading and saving entities can happen without error, and the token conversion makes sense there.

However:

+++ b/core/modules/block/block.post_update.php
@@ -0,0 +1,79 @@
+  $block_update_8001 = \Drupal::keyValue('update_backup')->get('block_update_8001', []);
+
+  $block_ids = array_keys($block_update_8001);
+  $block_storage = \Drupal::entityManager()->getStorage('block');
+  $blocks = $block_storage->loadMultiple($block_ids);
+  /** @var $blocks \Drupal\block\BlockInterface[] */
+  foreach ($blocks as $block) {
+    // This block will have an invalid context mapping service and must be
+    // disabled in order to prevent information disclosure.
+
+    // Disable currently enabled blocks.
+    if ($block_update_8001[$block->id()]['status']) {
+      $block->setStatus(FALSE);
+      $block->save();
+    }

For me this is a good example - get a list of blocks from a previous update, load them, change one thing that's valid before and after, save them. So can we just use that?

Thinking specifically about #2457257: Use Twig parser to detect errors in Twig code entered in Views UI which references the same Views issue.

Yeah let's go with the other example and document a bit around it.

Just a doc change, no need to wait for the bot

Addressed some quick feedback from catch.

I meant to put plach in the commit message but messed it up, however added to d.o credit at least.

Committed/pushed to 8.0.x, thanks!

Published the CR: https://www.drupal.org/node/2563673 (it was draft)

#2571533: Allow setting custom storage on FieldStorageConfig is adding a new hook_post_update_NAME(), which breaks UpdatePostUpdateTest because that test hardcodes the list of expected existing post_updates :-)

Feels weird to have to update the test each time we add a new post_update in core ? (even though that's probably what we're going to do for now to unblock that patch over there...)

I would love to see a followup which splits the storage into one key per update ran. Do not use massive arrays when there is no need. Just a $keyValue->exists() is fine to check whether an update already executed.