Issue #2754477 by alexpott, mr.baileys, swentel: Unexpected config entity...

Issue #2754477 by alexpott, mr.baileys, swentel: Unexpected config entity delete due to dependency calculations

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

@@ -307,8 +307,9 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
 
     // 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 fields are removed before
-    // field storages.
+    // 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)) {
       /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $dependent */
       if ($dry_run) {
@@ -346,7 +347,9 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
       // If the entity cannot be fixed then it has to be deleted.
       if (!$fixed) {
         $delete_uuids[] = $dependent->uuid();
-        $return['delete'][] = $dependent;
+        // 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

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

@@ -173,7 +173,9 @@ public function getDependentEntities($type, $name) {
     // dependent is at the top. For example, this ensures that fields are
     // always after field storages. This is because field storages need to be
     // created before a field.
-    return array_reverse(array_intersect_key($this->graph, $dependencies));
+    $graph = $this->getGraph();
+    uasort($graph, array($this, 'sortGraph'));
+    return array_replace(array_intersect_key($graph, $dependencies), $dependencies);
   }
 
   /**
@@ -188,7 +190,8 @@ public function sortAll() {
     // Sort by reverse weight and alphabetically. The most dependent entities
     // are last and entities with the same weight are alphabetically ordered.
     uasort($graph, array($this, 'sortGraph'));
-    return array_keys($graph);
+    // Use array_intersect_key() to exclude modules and themes from the list.
+    return array_reverse(array_keys(array_intersect_key($graph, $this->data)));
   }
 
   /**
@@ -228,9 +231,11 @@ protected function createGraphConfigEntityDependencies($entities_to_check) {
     $graph = $this->getGraph();
 
     foreach ($entities_to_check as $entity) {
-      if (isset($graph[$entity]) && !empty($graph[$entity]['reverse_paths'])) {
-        foreach ($graph[$entity]['reverse_paths'] as $dependency => $value) {
-          $dependent_entities[$dependency] = $this->data[$dependency];
+      if (isset($graph[$entity]) && !empty($graph[$entity]['paths'])) {
+        foreach ($graph[$entity]['paths'] as $dependency => $value) {
+          if (isset($this->data[$dependency])) {
+            $dependent_entities[$dependency] = $this->data[$dependency];
+          }
         }
       }
     }
@@ -248,12 +253,13 @@ protected function getGraph() {
       $graph = array();
       foreach ($this->data as $entity) {
         $graph_key = $entity->getConfigDependencyName();
-        $graph[$graph_key]['edges'] = array();
-        $dependencies = $entity->getDependencies('config');
-        if (!empty($dependencies)) {
-          foreach ($dependencies as $dependency) {
-            $graph[$graph_key]['edges'][$dependency] = TRUE;
-          }
+        if (!isset($graph[$graph_key])) {
+          $graph[$graph_key]['edges'] = [];
+        }
+        // Include all dependencies in the graph so that topographical sorting
+        // works.
+        foreach (array_merge($entity->getDependencies('config'), $entity->getDependencies('module'), $entity->getDependencies('theme')) as $dependency) {
+          $graph[$dependency]['edges'][$graph_key] = TRUE;
         }
       }
       $graph_object = new Graph($graph);

core/modules/forum/config/optional/core.entity_form_display.taxonomy_term.forums.default.yml

@@ -2,6 +2,7 @@ langcode: en
 status: true
 dependencies:
   config:
+    - field.field.taxonomy_term.forums.forum_container
     - taxonomy.vocabulary.forums
   module:
     - text

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

@@ -188,7 +188,8 @@ public function testConfigEntityUninstall() {
     /** @var \Drupal\Core\Config\ConfigManagerInterface $config_manager */
     $config_manager = \Drupal::service('config.manager');
     /** @var \Drupal\Core\Config\Entity\ConfigEntityStorage $storage */
-    $storage = $this->container->get('entity.manager')->getStorage('config_test');
+    $storage = $this->container->get('entity.manager')
+      ->getStorage('config_test');
     // Test dependencies between modules.
     $entity1 = $storage->create(
       array(
@@ -221,14 +222,42 @@ public function testConfigEntityUninstall() {
     $config_manager->uninstall('module', 'node');
     $this->assertFalse($storage->load('entity1'), 'Entity 1 deleted');
     $this->assertFalse($storage->load('entity2'), 'Entity 2 deleted');
+  }
 
-    // Set a more complicated test where dependencies will be fixed.
-    \Drupal::state()->set('config_test.fix_dependencies', array($entity1->getConfigDependencyName()));
-    \Drupal::state()->set('config_test.on_dependency_removal_called', []);
-    // Entity1 will be deleted because it depends on node.
-    $entity1 = $storage->create(
+  /**
+   * Data provider for self::testConfigEntityUninstallComplex().
+   */
+  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']],
+    ];
+  }
+
+  /**
+   * Tests complex configuration entity dependency handling during uninstall.
+   *
+   * Configuration entities can be deleted or updated during module uninstall
+   * because they have dependencies on the module.
+   *
+   * @param array $entity_id_suffixes
+   *   The suffixes to add to the 4 entities created by the test.
+   *
+   * @dataProvider providerConfigEntityUninstallComplex
+   */
+  public function testConfigEntityUninstallComplex(array $entity_id_suffixes) {
+    /** @var \Drupal\Core\Config\ConfigManagerInterface $config_manager */
+    $config_manager = \Drupal::service('config.manager');
+    /** @var \Drupal\Core\Config\Entity\ConfigEntityStorage $storage */
+    $storage = $this->container->get('entity.manager')
+      ->getStorage('config_test');
+    // Entity 1 will be deleted because it depends on node.
+    $entity_1 = $storage->create(
       array(
-        'id' => 'entity1',
+        'id' => 'entity_' . $entity_id_suffixes[0],
         'dependencies' => array(
           'enforced' => array(
             'module' => array('node', 'config_test')
@@ -236,77 +265,85 @@ public function testConfigEntityUninstall() {
         ),
       )
     );
-    $entity1->save();
+    $entity_1->save();
 
-    // Entity2 has a dependency on Entity1 but it can be fixed because
+    // Entity 2 has a dependency on entity 1 but it can be fixed because
     // \Drupal\config_test\Entity::onDependencyRemoval() will remove the
     // dependency before config entities are deleted.
-    $entity2 = $storage->create(
+    $entity_2 = $storage->create(
       array(
-        'id' => 'entity2',
+        'id' => 'entity_' . $entity_id_suffixes[1],
         'dependencies' => array(
           'enforced' => array(
-            'config' => array($entity1->getConfigDependencyName()),
+            'config' => array($entity_1->getConfigDependencyName()),
           ),
         ),
       )
     );
-    $entity2->save();
+    $entity_2->save();
 
-    // Entity3 will be unchanged because it is dependent on Entity2 which can
+    // Entity 3 will be unchanged because it is dependent on entity 2 which can
     // be fixed. The ConfigEntityInterface::onDependencyRemoval() method will
     // not be called for this entity.
-    $entity3 = $storage->create(
+    $entity_3 = $storage->create(
       array(
-        'id' => 'entity3',
+        'id' => 'entity_' . $entity_id_suffixes[2],
         'dependencies' => array(
           'enforced' => array(
-            'config' => array($entity2->getConfigDependencyName()),
+            'config' => array($entity_2->getConfigDependencyName()),
           ),
         ),
       )
     );
-    $entity3->save();
+    $entity_3->save();
 
-    // Entity4's config dependency will be fixed but it will still be deleted
+    // Entity 4's config dependency will be fixed but it will still be deleted
     // because it also depends on the node module.
-    $entity4 = $storage->create(
+    $entity_4 = $storage->create(
       array(
-        'id' => 'entity4',
+        'id' => 'entity_' . $entity_id_suffixes[3],
         'dependencies' => array(
           'enforced' => array(
-            'config' => array($entity1->getConfigDependencyName()),
+            'config' => array($entity_1->getConfigDependencyName()),
             'module' => array('node', 'config_test')
           ),
         ),
       )
     );
-    $entity4->save();
+    $entity_4->save();
+
+    // Set a more complicated test where dependencies will be fixed.
+    \Drupal::state()->set('config_test.fix_dependencies', array($entity_1->getConfigDependencyName()));
+    \Drupal::state()->set('config_test.on_dependency_removal_called', []);
 
     // Do a dry run using
     // \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval().
     $config_entities = $config_manager->getConfigEntitiesToChangeOnDependencyRemoval('module', ['node']);
-    $this->assertEqual($entity1->uuid(), $config_entities['delete'][0]->uuid(), 'Entity 1 will be deleted.');
-    $this->assertEqual($entity2->uuid(), reset($config_entities['update'])->uuid(), 'Entity 2 will be updated.');
-    $this->assertEqual($entity3->uuid(), reset($config_entities['unchanged'])->uuid(), 'Entity 3 is not changed.');
-    $this->assertEqual($entity4->uuid(), $config_entities['delete'][1]->uuid(), 'Entity 4 will be deleted.');
+    $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_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($entity3->id(), $called), 'ConfigEntityInterface::onDependencyRemoval() is not called for entity 3.');
-    $this->assertIdentical(['entity1', 'entity2', 'entity4'], $called, 'The most dependent entites have ConfigEntityInterface::onDependencyRemoval() called first.');
+    $this->assertFalse(in_array($entity_3->id(), $called), 'ConfigEntityInterface::onDependencyRemoval() is not called for entity 3.');
+    $this->assertIdentical([$entity_1->id(), $entity_4->id(), $entity_2->id()], $called, 'The most dependent entites have ConfigEntityInterface::onDependencyRemoval() called first.');
 
+    // Perform a module rebuild so we can know where the node module is located
+    // and uninstall it.
+    // @todo Remove as part of https://www.drupal.org/node/2186491
+    system_rebuild_module_data();
     // Perform the uninstall.
     $config_manager->uninstall('module', 'node');
 
     // Test that expected actions have been performed.
-    $this->assertFalse($storage->load('entity1'), 'Entity 1 deleted');
-    $entity2 = $storage->load('entity2');
-    $this->assertTrue($entity2, 'Entity 2 not deleted');
-    $this->assertEqual($entity2->calculateDependencies()->getDependencies()['config'], array(), 'Entity 2 dependencies updated to remove dependency on Entity1.');
-    $entity3 = $storage->load('entity3');
-    $this->assertTrue($entity3, 'Entity 3 not deleted');
-    $this->assertEqual($entity3->calculateDependencies()->getDependencies()['config'], [$entity2->getConfigDependencyName()], 'Entity 3 still depends on Entity 2.');
-    $this->assertFalse($storage->load('entity4'), 'Entity 4 deleted');
+    $this->assertFalse($storage->load($entity_1->id()), 'Entity 1 deleted');
+    $entity_2 = $storage->load($entity_2->id());
+    $this->assertTrue($entity_2, 'Entity 2 not deleted');
+    $this->assertEqual($entity_2->calculateDependencies()->getDependencies()['config'], array(), 'Entity 2 dependencies updated to remove dependency on entity 1.');
+    $entity_3 = $storage->load($entity_3->id());
+    $this->assertTrue($entity_3, 'Entity 3 not deleted');
+    $this->assertEqual($entity_3->calculateDependencies()->getDependencies()['config'], [$entity_2->getConfigDependencyName()], 'Entity 3 still depends on entity 2.');
+    $this->assertFalse($storage->load($entity_4->id()), 'Entity 4 deleted');
   }
 
   /**
@@ -456,8 +493,8 @@ public function testContentEntityDelete() {
     $entity3->save();
 
     $config_entities = $config_manager->getConfigEntitiesToChangeOnDependencyRemoval('content', [$content_entity->getConfigDependencyName()]);
-    $this->assertEqual($entity1->uuid(), $config_entities['delete'][0]->uuid(), 'Entity 1 will be deleted.');
-    $this->assertEqual($entity2->uuid(), $config_entities['delete'][1]->uuid(), 'Entity 2 will be deleted.');
+    $this->assertEqual($entity1->uuid(), $config_entities['delete'][1]->uuid(), 'Entity 1 will be deleted.');
+    $this->assertEqual($entity2->uuid(), $config_entities['delete'][0]->uuid(), 'Entity 2 will be deleted.');
     $this->assertTrue(empty($config_entities['update']), 'No dependencies of the content entity will be updated.');
     $this->assertTrue(empty($config_entities['unchanged']), 'No dependencies of the content entity will be unchanged.');
   }

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

@@ -348,8 +348,8 @@ function testSecondaryUpdateDeletedDeleterFirst() {
     $updates = $this->configImporter->reset()->getStorageComparer()->getChangelist('update');
     $expected = array(
       $name_deleter,
-      $name_deletee,
       $name_other,
+      $name_deletee,
     );
     $this->assertIdentical($expected, $updates);