Issue #3092714 by alexpott, hchonov, Berdir, gease: Config entity updater...

Issue #3092714 by alexpott, hchonov, Berdir, gease: Config entity updater misbehaves when updating multiple entity types

core/lib/Drupal/Core/Config/Entity/ConfigEntityUpdater.php

@@ -25,6 +25,11 @@
  */
 class ConfigEntityUpdater implements ContainerInjectionInterface {
 
+  /**
+   * The key used to store information in the update sandbox.
+   */
+  const SANDBOX_KEY = 'config_entity_updater';
+
   /**
    * The entity type manager.
    *
@@ -88,17 +93,24 @@ public static function create(ContainerInterface $container) {
    * @throws \InvalidArgumentException
    *   Thrown when the provided entity type ID is not a configuration entity
    *   type.
+   * @throws \RuntimeException
+   *   Thrown when used twice in the same update function for different entity
+   *   types. This method should only be called once per update function.
    */
   public function update(array &$sandbox, $entity_type_id, callable $callback = NULL) {
     $storage = $this->entityTypeManager->getStorage($entity_type_id);
-    $sandbox_key = 'config_entity_updater:' . $entity_type_id;
-    if (!isset($sandbox[$sandbox_key])) {
+
+    if (isset($sandbox[self::SANDBOX_KEY]) && $sandbox[self::SANDBOX_KEY]['entity_type'] !== $entity_type_id) {
+      throw new \RuntimeException('Updating multiple entity types in the same update function is not supported');
+    }
+    if (!isset($sandbox[self::SANDBOX_KEY])) {
       $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
       if (!($entity_type instanceof ConfigEntityTypeInterface)) {
         throw new \InvalidArgumentException("The provided entity type ID '$entity_type_id' is not a configuration entity type");
       }
-      $sandbox[$sandbox_key]['entities'] = $storage->getQuery()->accessCheck(FALSE)->execute();
-      $sandbox[$sandbox_key]['count'] = count($sandbox[$sandbox_key]['entities']);
+      $sandbox[self::SANDBOX_KEY]['entity_type'] = $entity_type_id;
+      $sandbox[self::SANDBOX_KEY]['entities'] = $storage->getQuery()->accessCheck(FALSE)->execute();
+      $sandbox[self::SANDBOX_KEY]['count'] = count($sandbox[self::SANDBOX_KEY]['entities']);
     }
 
     // The default behaviour is to fix dependencies.
@@ -111,7 +123,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[self::SANDBOX_KEY]['entities'], 0, $this->batchSize));
     foreach ($entities as $entity) {
       if (call_user_func($callback, $entity)) {
         $entity->trustData();
@@ -119,7 +131,7 @@ public function update(array &$sandbox, $entity_type_id, callable $callback = NU
       }
     }
 
-    $sandbox['#finished'] = empty($sandbox[$sandbox_key]['entities']) ? 1 : ($sandbox[$sandbox_key]['count'] - count($sandbox[$sandbox_key]['entities'])) / $sandbox[$sandbox_key]['count'];
+    $sandbox['#finished'] = empty($sandbox[self::SANDBOX_KEY]['entities']) ? 1 : ($sandbox[self::SANDBOX_KEY]['count'] - count($sandbox[self::SANDBOX_KEY]['entities'])) / $sandbox[self::SANDBOX_KEY]['count'];
   }
 
 }

core/modules/system/system.post_update.php

@@ -150,7 +150,7 @@ function system_post_update_language_item_callback() {
 }
 
 /**
- * Update all entity displays that contain extra fields.
+ * Update all entity view displays that contain extra fields.
  */
 function system_post_update_extra_fields(&$sandbox = NULL) {
   $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
@@ -174,10 +174,37 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
     return $needs_save;
   };
 
-  $config_entity_updater->update($sandbox, 'entity_form_display', $callback);
   $config_entity_updater->update($sandbox, 'entity_view_display', $callback);
 }
 
+/**
+ * Update all entity form displays that contain extra fields.
+ */
+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;
+    if (!empty($extra_fields[$display_context])) {
+      foreach ($extra_fields[$display_context] as $name => $extra_field) {
+        if ($component = $display->getComponent($name)) {
+          $display->setComponent($name, $component);
+          $needs_save = TRUE;
+        }
+      }
+    }
+    return $needs_save;
+  };
+
+  $config_entity_updater->update($sandbox, 'entity_form_display', $callback);
+}
+
 /**
  * Force cache clear to ensure aggregated JavaScript files are regenerated.
  *

core/modules/text/text.post_update.php

@@ -11,10 +11,27 @@
 use Drupal\text\Plugin\Field\FieldWidget\TextareaWithSummaryWidget;
 
 /**
- * Update text_with_summary fields and widgets to add summary required flags.
+ * Update text_with_summary fields to add summary required flags.
  */
 function text_post_update_add_required_summary_flag(&$sandbox = NULL) {
   $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
+
+  $field_callback = function (FieldConfigInterface $field) {
+    if ($field->getType() !== 'text_with_summary') {
+      return FALSE;
+    }
+    $field->setSetting('required_summary', FALSE);
+    return TRUE;
+  };
+
+  $config_entity_updater->update($sandbox, 'field_config', $field_callback);
+}
+
+/**
+ * Update text_with_summary widgets to add summary required flags.
+ */
+function text_post_update_add_required_summary_flag_form_display(&$sandbox = NULL) {
+  $config_entity_updater = \Drupal::classResolver(ConfigEntityUpdater::class);
   /** @var \Drupal\Core\Field\WidgetPluginManager $field_widget_manager */
   $field_widget_manager = \Drupal::service('plugin.manager.field.widget');
 
@@ -36,14 +53,5 @@ function text_post_update_add_required_summary_flag(&$sandbox = NULL) {
     return $needs_save;
   };
 
-  $field_callback = function (FieldConfigInterface $field) {
-    if ($field->getType() !== 'text_with_summary') {
-      return FALSE;
-    }
-    $field->setSetting('required_summary', FALSE);
-    return TRUE;
-  };
-
   $config_entity_updater->update($sandbox, 'entity_form_display', $widget_callback);
-  $config_entity_updater->update($sandbox, 'field_config', $field_callback);
 }

core/tests/Drupal/KernelTests/Core/Config/Entity/ConfigEntityUpdaterTest.php

@@ -58,8 +58,9 @@ public function testUpdate() {
     $this->assertEquals('config_test_9', $entities['config_test_9']->label());
     $this->assertEquals('config_test_10', $entities['config_test_10']->label());
     $this->assertEquals('config_test_14', $entities['config_test_14']->label());
-    $this->assertEquals(15, $sandbox['config_entity_updater:config_test']['count']);
-    $this->assertCount(5, $sandbox['config_entity_updater:config_test']['entities']);
+    $this->assertEquals(15, $sandbox['config_entity_updater']['count']);
+    $this->assertEquals('config_test', $sandbox['config_entity_updater']['entity_type']);
+    $this->assertCount(5, $sandbox['config_entity_updater']['entities']);
     $this->assertEquals(10 / 15, $sandbox['#finished']);
 
     // Update the rest.
@@ -70,7 +71,7 @@ public function testUpdate() {
     $this->assertEquals('config_test_10 (updated)', $entities['config_test_10']->label());
     $this->assertEquals('config_test_14 (updated)', $entities['config_test_14']->label());
     $this->assertEquals(1, $sandbox['#finished']);
-    $this->assertCount(0, $sandbox['config_entity_updater:config_test']['entities']);
+    $this->assertCount(0, $sandbox['config_entity_updater']['entities']);
   }
 
   /**
@@ -100,8 +101,8 @@ public function testUpdateDefaultCallback() {
     $this->assertEquals(['added_dependency'], $entities['config_test_8']->getDependencies()['module']);
     $this->assertEquals([], $entities['config_test_9']->getDependencies());
     $this->assertEquals([], $entities['config_test_14']->getDependencies());
-    $this->assertEquals(15, $sandbox['config_entity_updater:config_test']['count']);
-    $this->assertCount(6, $sandbox['config_entity_updater:config_test']['entities']);
+    $this->assertEquals(15, $sandbox['config_entity_updater']['count']);
+    $this->assertCount(6, $sandbox['config_entity_updater']['entities']);
     $this->assertEquals(9 / 15, $sandbox['#finished']);
 
     // Update the rest.
@@ -110,7 +111,7 @@ public function testUpdateDefaultCallback() {
     $this->assertEquals(['added_dependency'], $entities['config_test_9']->getDependencies()['module']);
     $this->assertEquals(['added_dependency'], $entities['config_test_14']->getDependencies()['module']);
     $this->assertEquals(1, $sandbox['#finished']);
-    $this->assertCount(0, $sandbox['config_entity_updater:config_test']['entities']);
+    $this->assertCount(0, $sandbox['config_entity_updater']['entities']);
   }
 
   /**
@@ -125,4 +126,16 @@ public function testUpdateException() {
     $updater->update($sandbox, 'entity_test_mul_changed');
   }
 
+  /**
+   * @covers ::update
+   */
+  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');
+  }
+
 }