Issue #2843074 by tim.plunkett, alexpott, claudiu.cristea, denutkarsh,...

Issue #2843074 by tim.plunkett, alexpott, claudiu.cristea, denutkarsh, effulgentsia: Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation

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

@@ -297,7 +297,7 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
     $dependency_manager = $this->getConfigDependencyManager();
     $dependents = $this->findConfigEntityDependentsAsEntities($type, $names, $dependency_manager);
     $original_dependencies = $dependents;
-    $delete_uuids = $update_uuids = [];
+    $delete_uuids = [];
 
     $return = [
       'update' => [],
@@ -305,6 +305,13 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
       '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
@@ -340,8 +347,9 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
           }
         }
         if ($fixed) {
+          // Remove the fixed dependency from the list of original dependencies.
+          unset($original_dependencies[$uuid_map[$dependent->uuid()]]);
           $return['update'][] = $dependent;
-          $update_uuids[] = $dependent->uuid();
         }
       }
       // If the entity cannot be fixed then it has to be deleted.
@@ -354,8 +362,8 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
     }
     // 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, $update_uuids) {
-      return !(in_array($dependent->uuid(), $delete_uuids) || in_array($dependent->uuid(), $update_uuids));
+    $return['unchanged'] = array_filter($original_dependencies, function ($dependent) use ($delete_uuids) {
+      return !(in_array($dependent->uuid(), $delete_uuids));
     });
 
     return $return;

core/modules/config/tests/config_test/config/schema/config_test.schema.yml

@@ -33,6 +33,12 @@ config_test.dynamic.*.*:
   type: config_test_dynamic
   label: 'Config test dynamic settings'
 
+config_test.dynamic.*.third_party.node:
+  type: mapping
+  mapping:
+    foo:
+      type: string
+
 config_test.query.*:
   type: config_entity
   mapping:

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

@@ -136,6 +136,10 @@ public function onDependencyRemoval(array $dependencies) {
         }
       }
     }
+    // If any of the dependencies removed still exists, return FALSE.
+    if (array_intersect_key(array_flip($this->dependencies['enforced']['config']), $dependencies['config'])) {
+      return FALSE;
+    }
     return $changed;
   }
 

core/modules/system/tests/modules/entity_test_third_party/config/schema/entity_test_third_party.schema.yml

+core.entity_view_display.*.*.*.third_party.entity_test_third_party:
+  type: mapping
+  label: 'Schema for entity_test module additions to entity_view_display entity'
+  mapping:
+    key:
+      type: string
+      label: 'Label for key'

core/modules/system/tests/modules/entity_test_third_party/entity_test_third_party.info.yml

+name: 'Entity test third-party settings module'
+type: module
+description: 'Provides third-party settings for test entity types.'
+package: Testing
+version: VERSION
+core: 8.x
+dependencies:
+  - entity_test

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

@@ -8,6 +8,8 @@
 /**
  * Tests for configuration dependencies.
  *
+ * @coversDefaultClass \Drupal\Core\Config\ConfigManager
+ *
  * @group config
  */
 class ConfigDependencyTest extends EntityKernelTestBase {
@@ -346,6 +348,123 @@ public function testConfigEntityUninstallComplex(array $entity_id_suffixes) {
     $this->assertFalse($storage->load($entity_4->id()), 'Entity 4 deleted');
   }
 
+  /**
+   * @covers ::uninstall
+   * @covers ::getConfigEntitiesToChangeOnDependencyRemoval
+   */
+  public function testConfigEntityUninstallThirdParty() {
+    /** @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_type.manager')
+      ->getStorage('config_test');
+    // Entity 1 will be fixed because it only has a dependency via third-party
+    // settings, which are fixable.
+    $entity_1 = $storage->create([
+      'id' => 'entity_1',
+      'dependencies' => [
+        'enforced' => [
+          'module' => ['config_test'],
+        ],
+      ],
+      'third_party_settings' => [
+        'node' => [
+          'foo' => 'bar',
+        ],
+      ],
+    ]);
+    $entity_1->save();
+
+    // Entity 2 has a dependency on entity 1.
+    $entity_2 = $storage->create([
+      'id' => 'entity_2',
+      'dependencies' => [
+        'enforced' => [
+          'config' => [$entity_1->getConfigDependencyName()],
+        ],
+      ],
+      'third_party_settings' => [
+        'node' => [
+          'foo' => 'bar',
+        ],
+      ],
+    ]);
+    $entity_2->save();
+
+    // 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.
+    $entity_3 = $storage->create([
+      'id' => 'entity_3',
+      'dependencies' => [
+        'enforced' => [
+          'config' => [$entity_2->getConfigDependencyName()],
+        ],
+      ],
+    ]);
+    $entity_3->save();
+
+    // Entity 4's config dependency will be fixed but it will still be deleted
+    // because it also depends on the node module.
+    $entity_4 = $storage->create([
+      'id' => 'entity_4',
+      'dependencies' => [
+        'enforced' => [
+          'config' => [$entity_1->getConfigDependencyName()],
+          'module' => ['node', 'config_test'],
+        ],
+      ],
+    ]);
+    $entity_4->save();
+
+    \Drupal::state()->set('config_test.fix_dependencies', []);
+    \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']);
+    $config_entity_ids = [
+      'update' => [],
+      'delete' => [],
+      'unchanged' => [],
+    ];
+    foreach ($config_entities as $type => $config_entities_by_type) {
+      foreach ($config_entities_by_type as $config_entity) {
+        $config_entity_ids[$type][] = $config_entity->id();
+      }
+    }
+    $expected = [
+      'update' => [$entity_1->id(), $entity_2->id()],
+      'delete' => [$entity_4->id()],
+      'unchanged' => [$entity_3->id()],
+    ];
+    $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.');
+
+    // 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.
+    $entity_1 = $storage->load($entity_1->id());
+    $this->assertTrue($entity_1, 'Entity 1 not deleted');
+    $this->assertSame($entity_1->getThirdPartySettings('node'), [], 'Entity 1 third party settings updated.');
+    $entity_2 = $storage->load($entity_2->id());
+    $this->assertTrue($entity_2, 'Entity 2 not deleted');
+    $this->assertSame($entity_2->getThirdPartySettings('node'), [], 'Entity 2 third party settings updated.');
+    $this->assertSame($entity_2->calculateDependencies()->getDependencies()['config'], [$entity_1->getConfigDependencyName()], 'Entity 2 still depends on entity 1.');
+    $entity_3 = $storage->load($entity_3->id());
+    $this->assertTrue($entity_3, 'Entity 3 not deleted');
+    $this->assertSame($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');
+  }
+
   /**
    * Tests deleting a configuration entity and dependency management.
    */

core/tests/Drupal/KernelTests/Core/Entity/EntityDisplayBaseTest.php

@@ -2,7 +2,10 @@
 
 namespace Drupal\KernelTests\Core\Entity;
 
+use Drupal\comment\Entity\CommentType;
 use Drupal\Core\Entity\Entity\EntityViewDisplay;
+use Drupal\field\Entity\FieldConfig;
+use Drupal\field\Entity\FieldStorageConfig;
 use Drupal\KernelTests\KernelTestBase;
 
 /**
@@ -15,7 +18,7 @@ class EntityDisplayBaseTest extends KernelTestBase {
   /**
    * {@inheritdoc}
    */
-  public static $modules = ['entity_test'];
+  public static $modules = ['entity_test', 'entity_test_third_party', 'field', 'system', 'comment'];
 
   /**
    * @covers ::preSave
@@ -54,4 +57,97 @@ public function testPreSave() {
     $this->assertSame('content', $component['region']);
   }
 
+  /**
+   * @covers ::onDependencyRemoval
+   */
+  public function testOnDependencyRemoval() {
+    // Create a comment field for entity_test.
+    $comment_bundle = CommentType::create([
+      'id' => 'entity_test',
+      'label' => 'entity_test',
+      'description' => '',
+      'target_entity_type_id' => 'entity_test',
+    ]);
+    $comment_bundle->save();
+    $comment_display = EntityViewDisplay::create([
+      'targetEntityType' => 'comment',
+      'bundle' => 'entity_test',
+      'mode' => 'default',
+      'status' => TRUE,
+      'third_party_settings' => [
+        'entity_test_third_party' => [
+          'key' => 'value',
+        ],
+      ],
+    ]);
+    $comment_display->save();
+    $field_storage = FieldStorageConfig::create([
+      'entity_type' => 'entity_test',
+      'field_name' => 'test_field',
+      'type' => 'comment',
+      'settings' => [
+        'comment_type' => 'entity_test',
+      ],
+    ]);
+    $field_storage->save();
+    $field = FieldConfig::create([
+      'field_storage' => $field_storage,
+      'label' => $this->randomMachineName(),
+      'bundle' => 'entity_test',
+    ]);
+    $field->save();
+
+    // Create an entity view display for entity_test.
+    $entity_display = EntityViewDisplay::create([
+      'targetEntityType' => 'entity_test',
+      'bundle' => 'entity_test',
+      'mode' => 'default',
+      'status' => TRUE,
+      'content' => [
+        'test_field' => ['type' => 'comment_default', 'region' => 'content', 'settings' => ['view_mode' => 'default'], 'label' => 'hidden', 'third_party_settings' => []],
+      ],
+      'third_party_settings' => [
+        'entity_test_third_party' => [
+          'key' => 'value',
+        ],
+      ],
+    ]);
+    $entity_display->save();
+
+    $expected_component = [
+      'type' => 'comment_default',
+      'region' => 'content',
+      'settings' => ['view_mode' => 'default'],
+      'label' => 'hidden',
+      'third_party_settings' => [],
+    ];
+    $entity_display->getComponent('test_field');
+    $this->assertEquals($expected_component, $entity_display->getComponent('test_field'));
+    $expected_dependencies = [
+      'config' => [
+        'core.entity_view_display.comment.entity_test.default',
+        'field.field.entity_test.entity_test.test_field',
+      ],
+      'module' => [
+        'comment',
+        'entity_test',
+        'entity_test_third_party',
+      ],
+    ];
+    $this->assertSame($expected_dependencies, $entity_display->getDependencies());
+
+    // Uninstall the third-party settings provider and reload the display.
+    $this->container->get('module_installer')->uninstall(['entity_test_third_party']);
+    $entity_display = EntityViewDisplay::load('entity_test.entity_test.default');
+
+    // The component should remain unchanged.
+    $this->assertEquals($expected_component, $entity_display->getComponent('test_field'));
+    // The dependencies should no longer contain 'entity_test_third_party'.
+    $expected_dependencies['module'] = [
+      'comment',
+      'entity_test',
+    ];
+    $this->assertSame($expected_dependencies, $entity_display->getDependencies());
+  }
+
 }