Config entity updater misbehaves when updating multiple entity types

(fix typo in title)

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
   @@ -92,13 +92,13 @@ public static function create(ContainerInterface $container) {
          }
   -      $sandbox[$sandbox_key]['entities'] = $storage->getQuery()->accessCheck(FALSE)->execute();
   -      $sandbox[$sandbox_key]['count'] = count($sandbox[$sandbox_key]['entities']);
   +      $sandbox['sandbox_keys'][$sandbox_key]['entities'] = $storage->getQuery()->accessCheck(FALSE)->execute();
   +      $sandbox['sandbox_keys'][$sandbox_key]['count'] = count($sandbox['sandbox_keys'][$sandbox_key]['entities']);
   

   Yes, the extra key works too, I think we can assume that what we write into $sandbox is internal and not an API :)

   That said, sandbox_keys doesn't make much sense to me. We store much more in there, the key is just the key to the actual data. what about "config_entity_updater"?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
   @@ -111,7 +111,7 @@ public function update(array &$sandbox, $entity_type_id, callable $callback = NU
    
        /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $entity */
   -    $entities = $storage->loadMultiple(array_splice($sandbox[$sandbox_key]['entities'], 0, $this->batchSize));
   +    $entities = $storage->loadMultiple(array_splice($sandbox['sandbox_keys'][$sandbox_key]['entities'], 0, $this->batchSize));
        foreach ($entities as $entity) {
   

   One thing that's a bit weird is that for entity types/sandbox_key's that are done (finished = 1), this will basically do array_splice([], 0, $this->batchSize), which is kinda pointless, so likely call loadMultiple([]) several times for already finished types, but would make the patch quite a bit bigger to wrap that in another condition. And it's not going to have a measurable performance difference. Would be different if we'd do a query each time..

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
   @@ -119,7 +119,14 @@ public function update(array &$sandbox, $entity_type_id, callable $callback = NU
   +    $sandbox['#finished'] = $sandbox['sandbox_keys'][$sandbox_key]['#finished'];
   +
   +    $sub_sandbox_info = reset($sandbox['sandbox_keys']);
   +    while(($sandbox['#finished'] == 1) && $sub_sandbox_info) {
   +      $sandbox['#finished'] = $sub_sandbox_info['#finished'];
   +      $sub_sandbox_info = next($sandbox['sandbox_keys']);
   

   So this now only cares about it being 1 or not, which technically is indeed the only thing that matters, but it will also result in a completely bogus progress and means it's kinda pointless to exactly calculate $sandbox['sandbox_keys'][$sandbox_key]['#finished'] in the first place.

   There are two options to improve it. The first is pretty easy and actually less code than what you have: https://3v4l.org/4l1hG

   That's not very exact in case one key is 1k items to process and the other 3 only 10, as you'll immediately reach 75% and then it will go slowly. But it's probably good enough.

   The other would be to calculate the total count and total count of remaining entities and then calculate the global finished based on that. Should be doable too but we're talking about config entities here, so we don't expect more than a handful of batch runs in total anyway. And using this to update multiple content entity types would need a completely different implementation anyway (we can't load 1M node ids into $sandbox)

Another thing to keep in mind. Not a big deal when called 2-3 times, but it means that each batch run actually processes up to $number_of_keys * $batch_size, so the generic loop in the other issue would could be hundreds of entities at least on the first run. e.g. 20 entity types * 50...

To respect the batch size, we'd need to subtract the already processed items on each run.

Given the following data set:
A: 20
B: 40
C: 130

with batch size = 50, the first run would process all 20 of A and 30 of B, then it would process the remaining 10 of B and 40 of C and so on..

Alternatively, instead of all those changes, we could just decide to not actually support multiple calls to it in the same function, knowing that is broken. We'd just throw an exception if $sandbox is already initialized with a different key. Would require to split two core updates retroactively, which we have to anyway to call it again (we'd move the first to a new function), and we couldn't do generic things. I'd expect this would only minimally affect contrib. I'd be OK with that :)

Nice find.

Alternatively, instead of all those changes, we could just decide to not actually support multiple calls to it in the same function, knowing that is broken. We'd just throw an exception if $sandbox is already initialized with a different key. Would require to split two core updates retroactively, which we have to anyway to call it again (we'd move the first to a new function), and we couldn't do generic things. I'd expect this would only minimally affect contrib. I'd be OK with that :)

I think that this makes a lot of sense. Calling this twice in the same function breaks the batch size setting and makes everything more complex.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
   @@ -92,13 +92,13 @@ public static function create(ContainerInterface $container) {
   +    if (!isset($sandbox['sandbox_keys'][$sandbox_key])) {
   

   I think 'sandbox_keys' is not obvious enough name for this key. Though name collision is not very likely, $sandbox in ConfigEntityUpdater is not isolated either. So why not call it explicitly 'config_entity_updater_keys' or just 'config_entity_updater'?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
   @@ -119,7 +119,14 @@ public function update(array &$sandbox, $entity_type_id, callable $callback = NU
   +    $sandbox['sandbox_keys'][$sandbox_key]['#finished'] = empty($sandbox['sandbox_keys'][$sandbox_key]['entities']) ? 1 : ($sandbox['sandbox_keys'][$sandbox_key]['count'] - count($sandbox['sandbox_keys'][$sandbox_key]['entities'])) / $sandbox['sandbox_keys'][$sandbox_key]['count'];
   

   For the sake of readability, why not assign $sandbox['sandbox_keys'][$sandbox_key] to a temporary variable?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
   @@ -119,7 +119,14 @@ public function update(array &$sandbox, $entity_type_id, callable $callback = NU
   +    $sub_sandbox_info = reset($sandbox['sandbox_keys']);
   +    while(($sandbox['#finished'] == 1) && $sub_sandbox_info) {
   +      $sandbox['#finished'] = $sub_sandbox_info['#finished'];
   +      $sub_sandbox_info = next($sandbox['sandbox_keys']);
   +    }
   

   As far as I see this piece of code, it assigns $sandbox['#finished'] the value of #finished of the first unfinished sandbox key. That means that instead of growing steadily with iterations from 0 to 1 #finished will jump unpredictably, and so will the progress bar. Why not use something like

   $sandbox['#finished'] = array_reduce($sandbox['sandbox_keys'], function($carry, $item) {
       $carry += $item['#finished'];
     });
     $sandbox['#finished'] = (int)($sandbox['#finished] /count($sandbox['sandbox_keys']));

Upd: While I was writing this, Berdir came up with essentially the same comments ).

Well, first I should notice that my approach above with array_reduce is not perfect, cause we need to take into account the relative size of batch (number of entities with certain key relative to the entire number of entities to be processed). That's not a big deal to add this to calculation, but we need to actually calculate the #finished only after last key is processed. This is first. Second is concern in #15 about not respecting the size of entity update batch. That makes me suggest that we simplify things inside ConfigEntityUpdater and can assign $sandbox['#finished'] = $sandbox['sandbox_keys'][$sandbox_key]['#finished], which will work in case of single call to update(). And if we make multiple calls, we should calculate $sandbox['#finished'], and (somehow) reset entity update batch size, outside of ConfigEntityUpdater::update(), just in the body of post update hook.

@hchonov the class was designed to be a helper so update functions didn't have to know about the entity update batch size - if you call it twice from within the same update then it breaks that. So I think as @Berdir points out we have two options:

1. To code around this complexity
2. To limit the ability to be called twice from the same function. Note that update hook gets called with a clean batch $sandbox

I prefer option 2 because it keeps the code simpler and it means that we respect the entity update batch size. I disagree that it is a major drawback. Having two update function updating separate entities is a good thing. It is a separation of concerns and make updates likely to work whatever the complexity and size of the site being updated.

> And what about 2. being a regression? Even in core we have 2 updates relying on the config entity updater for updating multiple entity types in the same update.

2 update functions that we *know* are broken. similar functions in contrib will be broken too. So in a way, we'll tell those modules that they need to fix their update functions.

generic update-all/many-entity-types use cases are very rare already in core and should be even more rare in contrib. If you have a complex use case you can still implement the batch yourself.

And you could still wrap it yourself. You'd first set your own sandbox, and then process the entity types one-by-one, and once it reports #finished = 1, you clean it up, set your actual finished and restart it with the next. Not pretty but quite doable.

Also, I've rewritten the specific use case in the other issue to not use the config entity updater and just loop over the configs directly.

@hchonov well the problem is that if we allow multiple calls from within the same function we have to allow for things that @Berdir describes in #15. And I think causing the regression is better than have the bug. This should be a critical - it results in update functions claiming they're completed when they are not.

I've had a play around with an idea of adding some additional functionality to the ConfigUpdater to allow it to add new batch sets so we can get around this problem by executing each update as a separate part of the batch. This works but it introduces two problems

1. The callables can't be a closure - because they are serialised and closures can't be serialized.
2. It changes the update order and these need batches are always done at the end because the updates are a single batch set so we can only add this one after.

Given these two problems I'm not sure that this solution is worth pursuing.

@hchonov

Should we then remove the config updater when developers would need to learn about the batch size anyway?

is not really true though. In the vast majority of cases an update is concerned with a single entity type and the config entity updater works perfectly. And in the case of text_post_update_add_required_summary_flag() and system_post_update_extra_fields() it's really not onerous to split these into separate updates.

Here's what an implementation of only allowing the ConfigEntityUpdater once per update would look like. Obviously we need to add docs.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
   @@ -99,6 +102,9 @@ public function update(array &$sandbox, $entity_type_id, callable $callback = NU
   +      // Save the caller so we can determine that this is only called once per
   +      // update.
   +      $sandbox['config_entity_updater'] = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS)[0];
   

   considering that we can't get rid of the $sandbox_key without API change anyway, could we just use $sandbox_key for that?

2. +++ b/core/modules/system/system.post_update.php
   @@ -150,9 +150,17 @@ function system_post_update_language_item_callback() {
     */
   -function system_post_update_extra_fields(&$sandbox = NULL) {
   +function system_post_update_extra_fields() {
   +  // Replaced by system_post_update_extra_fields_form_display() and
   +  // system_post_update_extra_fields_view_display().
   +}
   

   The second call actually did work, because that's the one that won the #finished conflict, so I think it might be enough to split of only the first call into a new function?

Thanks for the review @Berdir.

Fixed #26.2

But I think #26.1 is not right. We can't use $sandbox_key here because that contains the entity type. I'm not changing the key at all. I'm adding a new key to lock the update to being called from a single location with the same $sandbox.

@hchonov that's the opposite of what @Berdir said in #26.2 - who's right? I think @Berdir's reasoning is correct

The second call actually did work, because that's the one that won the #finished conflict, so I think it might be enough to split of only the first call into a new function?

Updated issue summary so that it is inline with the chosen solution. We still need a change record.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
   @@ -92,6 +92,9 @@ public static function create(ContainerInterface $container) {
        $sandbox_key = 'config_entity_updater:' . $entity_type_id;
   +    if (isset($sandbox['config_entity_updater']) && $sandbox['config_entity_updater'] !== debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS)[0]) {
   +      throw new \RuntimeException('\Drupal\Core\Config\Entity\ConfigEntityUpdater::update() can only be called once per update function');
   +    }
        if (!isset($sandbox[$sandbox_key])) {
          $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
          if (!($entity_type instanceof ConfigEntityTypeInterface)) {
   
   @@ -99,6 +102,9 @@ public function update(array &$sandbox, $entity_type_id, callable $callback = NU
   +      $sandbox['config_entity_updater'] = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS)[0];
   

   What I meant is that we would do $sandbox['config_entity_updater'] = $sandbox_key; and if you call it with another $sandbox key, we abort?

   You could mess with it by doing more than one update callback on the same entity type but that definitely never worked because it would clash on the sandbox values like count, so I think that would be simpler and doesn't require us to load the backtrace, which is afaik slow-ish.

2. +++ b/core/tests/Drupal/KernelTests/Core/Config/Entity/ConfigEntityUpdaterTest.php
   @@ -52,7 +52,7 @@ public function testUpdate() {
        // will have been updated.
   -    $updater->update($sandbox, 'config_test', $callback);
   +    $this->callUpdate($updater, $sandbox, 'config_test', $callback);
        $entities = $storage->loadMultiple();
        $this->assertEquals('config_test_8 (updated)', $entities['config_test_8']->label());
        $this->assertEquals('config_test_9', $entities['config_test_9']->label());
   @@ -63,7 +63,7 @@ public function testUpdate() {
   
   @@ -63,7 +63,7 @@ public function testUpdate() {
        $this->assertEquals(10 / 15, $sandbox['#finished']);
    
        // Update the rest.
   -    $updater->update($sandbox, 'config_test', $callback);
   +    $this->callUpdate($updater, $sandbox, 'config_test', $callback);
        $entities = $storage->loadMultiple();
   

   We also wouldn't have to change this, because calling it with the *same* $entity_type_id would still work just fine.

   Because the implementation doesn't care if you call it once or twice or 7 times from the same function. As your workaround with the parent proves. The only thing it cares about is that you don't mix different entity types.

@Berdir I don't think this is performance critical enough to worry about performance. I think the strictness of storing where we're called from has an advantage. I wouldn't be surprised to see something like:

  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'my_config_entity', function ($entity) use ($something) {
     return $this->something($entity);
  });

  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'my_config_entity', function ($entity) use ($something_else) {
     return $this->something_else($entity);
  });

And this would be broken.

Yes it should be written as

  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'my_config_entity', function ($entity) use ($something, $something_else) {
     return $this->something($entity) || $this->something_else($entity);
  });

But it would look it works.

I think if we want to support calls from different locations then we need to go back to the previous patches from hchonov.

Yes, performance isn't my main concern, just mentioned that too.

IMHO the debug_backtrace() implementation requires weird test hacks to fake being the same call and is quite hard to understand.

Yes, the code example in #34 would not fail, but I don't think it's very realistic that people would actually do that. Because...

> But it would look it works.

Not really. The problem with different entity types is that it works as long as you don't actually have to rely on multiple calls/batch. Your code example, while not failing, would pretty obviously not work, because it has the same key, would not re-initialize the sandbox and would either never run the callback or only on the second chunk of the batch. So if you really do two operations like that, a simple check against an updated entity will show that it didn't do both changes.

a simple check against an updated entity will show that it didn't do both changes.

Ah - good point. I'm convinced!

A bit of consistency.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php
@@ -91,12 +91,16 @@ public static function create(ContainerInterface $container) {
-    $sandbox_key = 'config_entity_updater:' . $entity_type_id;
+    $sandbox_key = 'config_entity_updater';
+    if (isset($sandbox[$sandbox_key]) && $sandbox[$sandbox_key]['entity_type'] !== $entity_type_id) {
+      throw new \RuntimeException('Updating multiple entity types in the same update function is not supported');
+    }

+++ b/core/tests/Drupal/KernelTests/Core/Config/Entity/ConfigEntityUpdaterTest.php
@@ -58,8 +58,9 @@ public function testUpdate() {
+    $this->assertEquals('config_test', $sandbox['config_entity_updater']['entity_type']);

@@ -125,4 +126,16 @@ public function testUpdateException() {
+  public function testUpdateOncePerUpdateException() {
+    $this->expectException(\RuntimeException::class);
+    $this->expectExceptionMessage('Updating multiple entity types in the same update function is not supported');
+    $updater = $this->container->get('class_resolver')->getInstanceFromDefinition(ConfigEntityUpdater::class);
+    $sandbox = [];
+    $updater->update($sandbox, 'config_test');
+    $updater->update($sandbox, 'config_query_test');
+  }

They key need needs to be config_entity_updater, otherwise it will never throw the exception as $sandbox_key is based on $entity_type_id. So both tests should fail now (The tests are correct, the implementation isn't).

The key is config_entity_updater. I try to run tests before submitting patches :)

Anyhow I'd be umming and ahhing about keeping the $sandbox_key variable as it is no longer variable. So I've moved it to a class constant because it makes things simpler.

The IS proposes a way to accommodate updating multiple types at once, but the patch forbids it.

Updated the issue summary and created the issue summary https://www.drupal.org/node/3100978

+++ b/core/modules/system/system.post_update.php
@@ -174,10 +174,37 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
+function system_post_update_extra_fields_form_display(&$sandbox = NULL) {
+  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
+  $entity_field_manager = \Drupal::service('entity_field.manager');
+
+  $callback = function (EntityDisplayInterface $display) use ($entity_field_manager) {
+    $display_context = $display instanceof EntityViewDisplayInterface ? 'display' : 'form';
+    $extra_fields = $entity_field_manager->getExtraFields($display->getTargetEntityTypeId(), $display->getTargetBundle());
+
+    // If any extra fields are used as a component, resave the display with the
+    // updated component information.
+    $needs_save = FALSE;

I suppose we could also make this an actual function so we can reuse it between the two updates, but not sure how important that really is at this point, would actually result in a bigger patch now. Might make sense if we have to do another update on multiple types in the future.

In my opinion, this is ready. I think @alexpott is still not 100% convinced about checking for the entity type vs the backtrace. But I think this is the cleaner option and the protection is good enough. See the example in #34 and my reply in #35. Doing it in the backtrace requires workarounds in tests that could in theory also be used in an upate function (if someone puts the update code with an argument for the entity type into a helper function..)

Committed/pushed to 9.0.x/8.9.x/8.8.x, thanks!

Backporting to 8.8.x since it's a critical upgrade path and the new post update is only doing something we were supposed to be doing something before.

Unless I'm missing something, the change record examples don't work: $this is not defined the closure is not running within a class instance ?

It's only abstract code anyway, but I changed it to do_something() instead of $this->something() now ;)

I'm running into issues related to this change when updating from 8.7 to 8.8

In https://www.drupal.org/project/drupal/issues/3092714#comment-13343349 There was an assumption that, post-update hooks get called with a clean batch $sandbox

For post-update hooks at least, this does not seem to be the case, which causes the above solution not to work.

I logged the output of "$sandbox['config_entity_updater']['entity_type']" at the start of all post update-hook that implement ConfigEntityUpdater that haven't run yet, and they all output block which happen to be the first update hook that is being run. I then removed that update hook and they all now output "entity_form_display" which is was the 2nd update hook to be run

This shows that sandbox is not clean at the start of a post-update hook and It now throws an exception for every post-update hook across any module that implements ConfigEntityUpdater except for the very first update hook that runs.

I'm not sure what needs to be done to tidy all this up. I guess 2 solutions would be
1. Ensure that post-update hooks do receive a clean sandbox
2. Implement a different approach for restricting a single ConfigEntityUpdater operation per update hook.

Also running into the issue when trying to update from 8.7.12 to 8.8.8

[ERROR] Updating multiple entity types in the same update function is not supported

If you get that error then you have an update function that has multiple calls, there's no other explanation. And core doesn't have that, so it has to be a custom or contrib module. What updates are being reported as pending? Check those modules and report it there.

Here is what I could get from the failed deployment logs :

Executing required previous updates
Executing update function "8801" of module "system"
Executing update function "8802" of module "system"
Executing update function "8803" of module "system"
Executing update function "8804" of module "system"
Executing update function "8805" of module "system"
Executing update function "8800" of module "locale"
Executing update function "add_status_extra_filter" of module "media"
Executing update function "create_language_content_settings" of module "path"
Executing update function "entity_reference_autocomplete_match_limit" of module "system"
Executing update function "extra_fields_form_display" of module "system"
Executing update function "fix_jquery_htmlprefilter" of module "system"
Executing update function "layout_plugin_schema_change" of module "system"
Executing update function "configure_status_field_widget" of module "taxonomy"
Executing update function "add_required_summary_flag" of module "text"

[ERROR] Updating multiple entity types in the same update function is not
supported

If you run drush updb again, which updates does it still report as pending?