From 884aeef38b805ee6c4d2ab9370db770171451303 Mon Sep 17 00:00:00 2001
From: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date: Fri, 24 Jun 2016 16:32:22 +0100
Subject: [PATCH] Issue #2754477 by alexpott, mr.baileys, swentel: Unexpected
config entity delete due to dependency calculations
---
core/lib/Drupal/Core/Config/ConfigManager.php | 9 +-
.../Config/Entity/ConfigDependencyManager.php | 28 +++--
...m_display.taxonomy_term.forums.default.yml | 1 +
.../Core/Config/ConfigDependencyTest.php | 115 ++++++++++++------
.../Core/Config/ConfigImporterTest.php | 2 +-
5 files changed, 101 insertions(+), 54 deletions(-)
diff --git a/core/lib/Drupal/Core/Config/ConfigManager.php b/core/lib/Drupal/Core/Config/ConfigManager.php
index 1ab9fdb55f8d..11d7c84398cf 100644
--- a/core/lib/Drupal/Core/Config/ConfigManager.php
+++ b/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
diff --git a/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
index 377f7c4d00d2..6b3793e60bb5 100644
--- a/core/lib/Drupal/Core/Config/Entity/ConfigDependencyManager.php
+++ b/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);
diff --git a/core/modules/forum/config/optional/core.entity_form_display.taxonomy_term.forums.default.yml b/core/modules/forum/config/optional/core.entity_form_display.taxonomy_term.forums.default.yml
index a2f890d30872..b18c869becd0 100644
--- a/core/modules/forum/config/optional/core.entity_form_display.taxonomy_term.forums.default.yml
+++ b/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
diff --git a/core/tests/Drupal/KernelTests/Core/Config/ConfigDependencyTest.php b/core/tests/Drupal/KernelTests/Core/Config/ConfigDependencyTest.php
index 2fd6301f5c7f..d30b3c53b7d9 100644
--- a/core/tests/Drupal/KernelTests/Core/Config/ConfigDependencyTest.php
+++ b/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.');
}
diff --git a/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
index 945e994e3ba7..8fff1fa7d92e 100644
--- a/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php
+++ b/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);
--
GitLab