Issue #2850973 by alexpott, kferencz91, jonathan1055, dawehner:...

Issue #2850973 by alexpott, kferencz91, jonathan1055, dawehner: ConfigEntityInterface::onDependencyRemoval() called with incorrect dependency list

core/core.services.yml

@@ -854,9 +854,9 @@ services:
     arguments: ['@router.route_provider', '@path.current', '@url_generator']
     tags:
       # @todo Try to combine those tags together, see https://www.drupal.org/node/2915772.
-      - { name: service_collector, tag: non_lazy_route_enhancer, call: addRouteEnhancer }
+      - { name: service_collector, tag: non_lazy_route_enhancer, call: addDeprecatedRouteEnhancer }
       - { name: service_collector, tag: route_enhancer, call: addRouteEnhancer  }
-      - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
+      - { name: service_collector, tag: non_lazy_route_filter, call: addDeprecatedRouteFilter }
       - { name: service_collector, tag: route_filter, call: addRouteFilter }
     calls:
       - [setContext, ['@router.request_context']]

core/lib/Drupal/Core/Config/ConfigManager.php

@@ -292,78 +292,85 @@ public function findConfigEntityDependentsAsEntities($type, array $names, Config
    * {@inheritdoc}
    */
   public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names, $dry_run = TRUE) {
-    // Determine the current list of dependent configuration entities and set up
-    // initial values.
     $dependency_manager = $this->getConfigDependencyManager();
-    $dependents = $this->findConfigEntityDependentsAsEntities($type, $names, $dependency_manager);
-    $original_dependencies = $dependents;
-    $delete_uuids = [];
 
+    // Store the list of dependents in three separate variables. This allows us
+    // to determine how the dependency graph changes as entities are fixed by
+    // calling the onDependencyRemoval() method.
+
+    // The list of original dependents on $names. This list never changes.
+    $original_dependents = $this->findConfigEntityDependentsAsEntities($type, $names, $dependency_manager);
+
+    // The current list of dependents on $names. This list is recalculated when
+    // calling an entity's onDependencyRemoval() method results in the entity
+    // changing. This list is passed to each entity's onDependencyRemoval()
+    // method as the list of affected entities.
+    $current_dependents = $original_dependents;
+
+    // The list of dependents to process. This list changes as entities are
+    // processed and are either fixed or deleted.
+    $dependents_to_process = $original_dependents;
+
+    // Initialize other variables.
+    $affected_uuids = [];
     $return = [
       'update' => [],
       'delete' => [],
       'unchanged' => [],
     ];
 
-    // Create a map of UUIDs to $original_dependencies key so that we can remove
-    // fixed dependencies.
-    $uuid_map = [];
-    foreach ($original_dependencies as $key => $entity) {
-      $uuid_map[$entity->uuid()] = $key;
-    }
-
-    // Try to fix any dependencies and find out what will happen to the
-    // dependency graph. Entities are processed in the order of most dependent
-    // first. For example, this ensures that Menu UI third party dependencies on
-    // node types are fixed before processing the node type's other
-    // dependencies.
-    while ($dependent = array_pop($dependents)) {
+    // Try to fix the dependents and find out what will happen to the dependency
+    // graph. Entities are processed in the order of most dependent first. For
+    // example, this ensures that Menu UI third party dependencies on node types
+    // are fixed before processing the node type's other dependents.
+    while ($dependent = array_pop($dependents_to_process)) {
       /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $dependent */
       if ($dry_run) {
         // Clone the entity so any changes do not change any static caches.
         $dependent = clone $dependent;
       }
       $fixed = FALSE;
-      if ($this->callOnDependencyRemoval($dependent, $original_dependencies, $type, $names)) {
+      if ($this->callOnDependencyRemoval($dependent, $current_dependents, $type, $names)) {
         // Recalculate dependencies and update the dependency graph data.
         $dependent->calculateDependencies();
         $dependency_manager->updateData($dependent->getConfigDependencyName(), $dependent->getDependencies());
-        // Based on the updated data rebuild the list of dependents. This will
-        // remove entities that are no longer dependent after the recalculation.
-        $dependents = $this->findConfigEntityDependentsAsEntities($type, $names, $dependency_manager);
-        // Remove any entities that we've already marked for deletion.
-        $dependents = array_filter($dependents, function ($dependent) use ($delete_uuids) {
-          return !in_array($dependent->uuid(), $delete_uuids);
+        // Based on the updated data rebuild the list of current dependents.
+        // This will remove entities that are no longer dependent after the
+        // recalculation.
+        $current_dependents = $this->findConfigEntityDependentsAsEntities($type, $names, $dependency_manager);
+        // Rebuild the list of entities that we need to process using the new
+        // list of current dependents and removing any entities that we've
+        // already processed.
+        $dependents_to_process = array_filter($current_dependents, function ($current_dependent) use ($affected_uuids) {
+          return !in_array($current_dependent->uuid(), $affected_uuids);
         });
-        // Ensure that the dependency has actually been fixed. It is possible
-        // that the dependent has multiple dependencies that cause it to be in
-        // the dependency chain.
+        // Ensure that the dependent has actually been fixed. It is possible
+        // that other dependencies cause it to still be in the list.
         $fixed = TRUE;
-        foreach ($dependents as $key => $entity) {
+        foreach ($dependents_to_process as $key => $entity) {
           if ($entity->uuid() == $dependent->uuid()) {
             $fixed = FALSE;
-            unset($dependents[$key]);
+            unset($dependents_to_process[$key]);
             break;
           }
         }
         if ($fixed) {
-          // Remove the fixed dependency from the list of original dependencies.
-          unset($original_dependencies[$uuid_map[$dependent->uuid()]]);
+          $affected_uuids[] = $dependent->uuid();
           $return['update'][] = $dependent;
         }
       }
       // If the entity cannot be fixed then it has to be deleted.
       if (!$fixed) {
-        $delete_uuids[] = $dependent->uuid();
+        $affected_uuids[] = $dependent->uuid();
         // Deletes should occur in the order of the least dependent first. For
         // example, this ensures that fields are removed before field storages.
         array_unshift($return['delete'], $dependent);
       }
     }
-    // Use the lists of UUIDs to filter the original list to work out which
-    // configuration entities are unchanged.
-    $return['unchanged'] = array_filter($original_dependencies, function ($dependent) use ($delete_uuids) {
-      return !(in_array($dependent->uuid(), $delete_uuids));
+    // Use the list of affected UUIDs to filter the original list to work out
+    // which configuration entities are unchanged.
+    $return['unchanged'] = array_filter($original_dependents, function ($dependent) use ($affected_uuids) {
+      return !(in_array($dependent->uuid(), $affected_uuids));
     });
 
     return $return;

core/lib/Drupal/Core/Routing/Router.php

@@ -97,6 +97,17 @@ public function addRouteFilter(FilterInterface $route_filter) {
     $this->filters[] = $route_filter;
   }
 
+  /**
+   * Adds a deprecated route filter.
+   *
+   * @param \Drupal\Core\Routing\FilterInterface $route_filter
+   *   The route filter.
+   */
+  public function addDeprecatedRouteFilter(FilterInterface $route_filter) {
+    @trigger_error('non_lazy_route_filter is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Instead, should use route_filter, see https://www.drupal.org/node/2894934', E_USER_DEPRECATED);
+    $this->filters[] = $route_filter;
+  }
+
   /**
    * Adds a route enhancer.
    *
@@ -107,6 +118,17 @@ public function addRouteEnhancer(EnhancerInterface $route_enhancer) {
     $this->enhancers[] = $route_enhancer;
   }
 
+  /**
+   * Adds a deprecated route enhancer.
+   *
+   * @param \Drupal\Core\Routing\EnhancerInterface $route_enhancer
+   *   The route enhancer.
+   */
+  public function addDeprecatedRouteEnhancer(EnhancerInterface $route_enhancer) {
+    @trigger_error('non_lazy_route_enhancer is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Instead, should use route_enhancer, see https://www.drupal.org/node/2894934', E_USER_DEPRECATED);
+    $this->enhancers[] = $route_enhancer;
+  }
+
   /**
    * {@inheritdoc}
    */

core/modules/config/tests/config_test/src/Entity/ConfigTest.php

@@ -117,9 +117,12 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    * {@inheritdoc}
    */
   public function onDependencyRemoval(array $dependencies) {
-    // Record which entities have this method called on.
+    // Record which entities have this method called on and what dependencies
+    // are passed.
     $called = \Drupal::state()->get('config_test.on_dependency_removal_called', []);
-    $called[] = $this->id();
+    $called[$this->id()] = $dependencies;
+    $called[$this->id()]['config'] = array_keys($called[$this->id()]['config']);
+    $called[$this->id()]['content'] = array_keys($called[$this->id()]['content']);
     \Drupal::state()->set('config_test.on_dependency_removal_called', $called);
 
     $changed = parent::onDependencyRemoval($dependencies);

core/tests/Drupal/KernelTests/Core/Config/ConfigDependencyTest.php

@@ -235,9 +235,9 @@ public function providerConfigEntityUninstallComplex() {
     // Ensure that alphabetical order has no influence on dependency fixing and
     // removal.
     return [
-      [['a', 'b', 'c', 'd']],
-      [['d', 'c', 'b', 'a']],
-      [['c', 'd', 'a', 'b']],
+      [['a', 'b', 'c', 'd', 'e']],
+      [['e', 'd', 'c', 'b', 'a']],
+      [['e', 'c', 'd', 'a', 'b']],
     ];
   }
 
@@ -316,6 +316,25 @@ public function testConfigEntityUninstallComplex(array $entity_id_suffixes) {
     );
     $entity_4->save();
 
+    // Entity 5 will be fixed because it is dependent on entity 3, which is
+    // unchanged, and entity 1 which will be fixed because
+    // \Drupal\config_test\Entity::onDependencyRemoval() will remove the
+    // dependency.
+    $entity_5 = $storage->create(
+      [
+        'id' => 'entity_' . $entity_id_suffixes[4],
+        'dependencies' => [
+          'enforced' => [
+            'config' => [
+              $entity_1->getConfigDependencyName(),
+              $entity_3->getConfigDependencyName(),
+            ],
+          ],
+        ],
+      ]
+    );
+    $entity_5->save();
+
     // Set a more complicated test where dependencies will be fixed.
     \Drupal::state()->set('config_test.fix_dependencies', [$entity_1->getConfigDependencyName()]);
     \Drupal::state()->set('config_test.on_dependency_removal_called', []);
@@ -323,14 +342,22 @@ public function testConfigEntityUninstallComplex(array $entity_id_suffixes) {
     // Do a dry run using
     // \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval().
     $config_entities = $config_manager->getConfigEntitiesToChangeOnDependencyRemoval('module', ['node']);
+
+    // Assert that \Drupal\config_test\Entity\ConfigTest::onDependencyRemoval()
+    // is called as expected and with the correct dependencies.
+    $called = \Drupal::state()->get('config_test.on_dependency_removal_called', []);
+    $this->assertArrayNotHasKey($entity_3->id(), $called, 'ConfigEntityInterface::onDependencyRemoval() is not called for entity 3.');
+    $this->assertSame([$entity_1->id(), $entity_4->id(), $entity_2->id(), $entity_5->id()], array_keys($called), 'The most dependent entites have ConfigEntityInterface::onDependencyRemoval() called first.');
+    $this->assertSame(['config' => [], 'content' => [], 'module' => ['node'], 'theme' => []], $called[$entity_1->id()]);
+    $this->assertSame(['config' => [$entity_1->getConfigDependencyName()], 'content' => [], 'module' => [], 'theme' => []], $called[$entity_2->id()]);
+    $this->assertSame(['config' => [$entity_1->getConfigDependencyName()], 'content' => [], 'module' => ['node'], 'theme' => []], $called[$entity_4->id()]);
+    $this->assertSame(['config' => [$entity_1->getConfigDependencyName()], 'content' => [], 'module' => [], 'theme' => []], $called[$entity_5->id()]);
+
     $this->assertEqual($entity_1->uuid(), $config_entities['delete'][1]->uuid(), 'Entity 1 will be deleted.');
-    $this->assertEqual($entity_2->uuid(), reset($config_entities['update'])->uuid(), 'Entity 2 will be updated.');
+    $this->assertEqual($entity_2->uuid(), $config_entities['update'][0]->uuid(), 'Entity 2 will be updated.');
     $this->assertEqual($entity_3->uuid(), reset($config_entities['unchanged'])->uuid(), 'Entity 3 is not changed.');
     $this->assertEqual($entity_4->uuid(), $config_entities['delete'][0]->uuid(), 'Entity 4 will be deleted.');
-
-    $called = \Drupal::state()->get('config_test.on_dependency_removal_called', []);
-    $this->assertFalse(in_array($entity_3->id(), $called), 'ConfigEntityInterface::onDependencyRemoval() is not called for entity 3.');
-    $this->assertSame([$entity_1->id(), $entity_4->id(), $entity_2->id()], $called, 'The most dependent entites have ConfigEntityInterface::onDependencyRemoval() called first.');
+    $this->assertEqual($entity_5->uuid(), $config_entities['update'][1]->uuid(), 'Entity 5 is updated.');
 
     // Perform a module rebuild so we can know where the node module is located
     // and uninstall it.
@@ -443,8 +470,11 @@ public function testConfigEntityUninstallThirdParty() {
     $this->assertSame($expected, $config_entity_ids);
 
     $called = \Drupal::state()->get('config_test.on_dependency_removal_called', []);
-    $this->assertFalse(in_array($entity_3->id(), $called), 'ConfigEntityInterface::onDependencyRemoval() is not called for entity 3.');
-    $this->assertSame([$entity_1->id(), $entity_4->id(), $entity_2->id()], $called, 'The most dependent entities have ConfigEntityInterface::onDependencyRemoval() called first.');
+    $this->assertArrayNotHasKey($entity_3->id(), $called, 'ConfigEntityInterface::onDependencyRemoval() is not called for entity 3.');
+    $this->assertSame([$entity_1->id(), $entity_4->id(), $entity_2->id()], array_keys($called), 'The most dependent entities have ConfigEntityInterface::onDependencyRemoval() called first.');
+    $this->assertSame(['config' => [], 'content' => [], 'module' => ['node'], 'theme' => []], $called[$entity_1->id()]);
+    $this->assertSame(['config' => [], 'content' => [], 'module' => ['node'], 'theme' => []], $called[$entity_2->id()]);
+    $this->assertSame(['config' => [], 'content' => [], 'module' => ['node'], 'theme' => []], $called[$entity_4->id()]);
 
     // Perform a module rebuild so we can know where the node module is located
     // and uninstall it.