Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted

I think we have to delete, yes.

I think it should be possible to delete a node type with fields for example. It is (almost, completely?) impossible to first delete all fields, view and form displays, translation settings, ... that depend on that.

Trying a re-title.

Also this is at least major.

Also I think this should be tagged D8 upgrade path.

Thinking of situations like:

1. Delete a config entity with dependencies that this patch would affect, such as a node type.

2. Core adds an update that needs to resave a set of config entities (like form displays) that depend on the node type.

3. The form displays for the deleted node type can't be saved, so can't be updated.

We don't yet have patches doing #2, but a lot of that is because we're not writing hook_update_N() yet. This can happen just with clicking around the UI and no contrib modules.

Patch itself looks good to me so far.

Incredibly elegant patch! Mostly nits below.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -452,4 +454,34 @@ protected static function invalidateTagsOnDelete(EntityTypeInterface $entity_typ
   +  public static function preDelete(EntityStorageInterface $storage, array $entities) {
   

   Missing docblock.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -452,4 +454,34 @@ protected static function invalidateTagsOnDelete(EntityTypeInterface $entity_typ
   +        // During extension uninstall and configuration synchronisation
   

   synchronization… sorry!

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -452,4 +454,34 @@ protected static function invalidateTagsOnDelete(EntityTypeInterface $entity_typ
   +        // Because we are only passing in one dependency to the
   +        // onDependencyRemoval method returns TRUE then we assume that the
   +        // dependency has been removed.
   

   This needs rephrasing; the grammar doesn't really work.

4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -452,4 +454,34 @@ protected static function invalidateTagsOnDelete(EntityTypeInterface $entity_typ
   +  }
   +
   +
    }
   

   Should be only one blank line.

5. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -16,4 +17,51 @@ class EntityDeleteForm extends EntityConfirmFormBase {
   +      '#description' => $this->t('The listed configuration will be updated if possible, or deleted.'),
   

   So how does the user know which will be deleted, which will be updated?

6. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -16,4 +17,51 @@ class EntityDeleteForm extends EntityConfirmFormBase {
   +      '#access' => FALSE,
   ...
   +      $form['other_entities']['#access'] = TRUE;
   

   This is a bit strange.

7. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -16,4 +17,51 @@ class EntityDeleteForm extends EntityConfirmFormBase {
   +        // Store the ID and label to sort the dependent_entity types and entities later.
   

   80 cols.

8. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -16,4 +17,51 @@ class EntityDeleteForm extends EntityConfirmFormBase {
   +        // Sort the list of dependent_entity labels alphabetically.
   

   The underscore can be removed.

9. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -405,4 +405,20 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state);
   +   * Informs the plugin that a dependency of the field will be deleted.
   

   Is it really only passed dependencies that are known to be dependencies? AFAICT this is passed every deleted dependency?

10. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
    @@ -286,4 +286,23 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
    +  public static function onDependencyRemoval(FieldDefinitionInterface $field_definition, array $dependencies) {
    

    Missing @inheritdoc.

11. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    @@ -745,7 +740,7 @@ public static function loadByName($entity_type_id, $field_name) {
         // The field storage is not deleted, is configured to be removed when there
         // are no fields and the field storage has no bundles.
    -    return !$this->deleted && !$this->persist_with_no_fields && count($this->getBundles()) == 0;
    +    return !$this->deleted && !$this->persist_with_no_fields && count($this->getBundles()) == 0 && !self::$in_deletion;
    

    I think the comment should be updated.

That looks reasonable to me as well.

No longer applies after #2361775: Third party settings dependencies cause config entity deletion (minor conflicts in ConfigEntityBase and ConfigEntityInterface), here's a reroll.

EDIT: Tests failed due to "MySQL has gone away". Haven't seen that on our testbot before.

#17.5: sure!

+++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
@@ -23,6 +24,14 @@ class EntityDeleteForm extends EntityConfirmFormBase {
+    // given to more users. The in this method should not make assumptions that

"The in this"

#2014955: Deleted bundles do not have their language configuration deleted implemented this one-off for language configuration and added a test. I think the fix itself needs to be removed here because it would happen automatically but the test should be kept to prove it still works.

Alex told me in IRC that he'd prefer to land #2420107: Determine which config entities can be fixed and which will be deleted when a dependency is removed first, so that #17.5 can be fully addressed in this patch. But this patch can still address #31 in the mean time, it'll just have to wait to solve #17.5, any other aspects are still up for review.

#34: AWESOME!!! :)

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyDeleteForm.php
   @@ -0,0 +1,119 @@
   + * Lists affected configuration entities during a during a dependency removal.
   

   during a during a

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyDeleteForm.php
   @@ -0,0 +1,119 @@
   +trait ConfigDependencyDeleteForm {
   

   as much as I don't agree with it, don't our traits have to have the Trait suffix? Same fashion as Interfaces?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigDependencyDeleteForm.php
   @@ -0,0 +1,119 @@
   +  abstract protected function t($string, array $args = array(), array $options = array());
   

   traits can include other traits - I assume this is here to prevent complaints about missing methods by an IDE - any reason we can't embed the string translation trait direct in here?

4. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -13,7 +15,26 @@
   +    $dependent_entities = \Drupal::service('config.manager')->getConfigEntitiesToChangeOnDependencyRemoval($entity->getConfigDependencyKey(), [$entity->getConfigDependencyName()]);
   

   Fairly sure this can be injected, happy to be wrong.

5. +++ b/core/modules/field/src/Entity/FieldConfig.php
   @@ -230,29 +231,6 @@ public static function postDelete(EntityStorageInterface $storage, array $fields
   -
   -    // Cleanup entity displays.
   -    $displays_to_update = array();
   -    foreach ($fields as $field) {
   -      if (!$field->deleted) {
   -        $view_modes = \Drupal::entityManager()->getViewModeOptions($field->entity_type, TRUE);
   -        foreach (array_keys($view_modes) as $mode) {
   -          $displays_to_update['entity_view_display'][$field->entity_type . '.' . $field->bundle . '.' . $mode][] = $field->getName();
   -        }
   -        $form_modes = \Drupal::entityManager()->getFormModeOptions($field->entity_type, TRUE);
   -        foreach (array_keys($form_modes) as $mode) {
   -          $displays_to_update['entity_form_display'][$field->entity_type . '.' . $field->bundle . '.' . $mode][] = $field->getName();
   -        }
   -      }
   -    }
   -    foreach ($displays_to_update as $type => $ids) {
   -      foreach (entity_load_multiple($type, array_keys($ids)) as $id => $display) {
   -        foreach ($ids[$id] as $field_name) {
   -          $display->removeComponent($field_name);
   -        }
   -        $display->save();
   -      }
   

   sweet

6. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -374,29 +381,13 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    self::$inDeletion = TRUE;
   
   @@ -424,6 +415,8 @@ public static function postDelete(EntityStorageInterface $storage, array $fields
   +    self::$inDeletion = FALSE;
   

   Is there any circumstances in which someone would sub-class this? If so should we use static instead?

7. +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   @@ -794,8 +787,9 @@ public static function loadByName($entity_type_id, $field_name) {
   +    return !$this->deleted && !$this->persist_with_no_fields && count($this->getBundles()) == 0 && !self::$inDeletion;
   

   Should we have static properties bleeding into non-static methods? That seems like it might open a world of pain.

   start deleting one field config - flag is set - attempt to delete another one, but its skipped because the flag from the previous one?

Do we need more tests to go with the new functionality?

Also #34 is the business - @alexpott++

#38: Shortcut Strikes Again. (Off the top of my head.)

3. To expand on that, I had the same problem with my delete form trait. The problem is that you can not have the same trait multiple times in your class. So if you have Trait X and Z and class A extends from B and uses trait X, then you get php errors if both the parent class B and the trait X use Trait Z.

#43: thanks, that's helpful info! It also makes PHP traits less useful/less widely applicable :/

(Just clarifying that the other issue is not, in fact, sentient.)

Not clear on this bit though:

The problem is there are many ways to get core inconsistent. Active configuration should never get into an inconsistent state with config depending on things that do not exist.

Get "core" inconsistent with what? "Inconsistent" how? Maybe examples of how "core" becomes "inconsistent" would help? Presumably, deleting something and leaving around dependent things is one of the ways (based on the title of this issue). :)

Embedding all the screenshots. :) Making someone leave the issue to see the screenshots is, well, like not having an indoor bathroom. Sure, outhouses work, but...

I think the truly correct thing to do is to move the field storage deletion if the last field has been removed to the UI layer.

Seems like a good change to make regardless.

Only found two very minor issues. The screenshots look great. I think the 'Field' deletion will be a bit jarring for people who are coming from Drupal 7's field instances (I had to do a double take) but that terminology is not introduced here.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -286,4 +286,26 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
   +    if (is_array($field_definition->default_value) && count($field_definition->default_value)) {
   

   Nit: why bother counting? The foreach would be a no-op on an empty array anyway. If we want to save the call to field definition then could still skip the count and just do normal bool condition here.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -286,4 +286,26 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
   +          // If the entity does not exist do not create the dependency.
   

   Is it not creating a dependency or cleaning up a previous dependency?

Looks great, some feedback below, mostly minor stuff, but the save() in field config, and the default value change might be incorrect?

I didn't read all the previous reviews/comments, ignore me if something has been discussed already.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -233,7 +234,8 @@ public function createDuplicate() {
      /**
   -   * Helper callback for uasort() to sort configuration entities by weight and label.
   +   * Helper callback for uasort() to sort configuration entities by weight and
   +   * label.
       */
   

   The changed version isn't really more correct, it just violates other coding standards, why touch this here?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -517,4 +519,36 @@ public function getThirdPartyProviders() {
   +  public static function preDelete(EntityStorageInterface $storage, array $entities) {
   +    foreach ($entities as $entity) {
   +      if ($entity->isUninstalling() || $entity->isSyncing()) {
   +        // During extension uninstall and configuration synchronization
   +        // deletions are already managed.
   +        break;
   +      }
   

   Should this call the parent or not? I think you prefer to not doing that unless there is a specific reason ;)

3. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -7,13 +7,45 @@
    class EntityDeleteForm extends EntityConfirmFormBase {
   -
      use EntityDeleteFormTrait;
   

   Do we have a coding standard for this?

4. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -7,13 +7,45 @@
   +  public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form = parent::buildForm($form, $form_state);
   

   entity forms also have the form() method, I don't really know when it is better to overwrite which, I guess this is fine..

5. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteForm.php
   @@ -7,13 +7,45 @@
   +    // Only do dependency processing for configuration entities. Whilst it is
   +    // possible for a configuration entity to be dependent on a content entity,
   +    // these dependencies are soft and content delete permissions are often
   +    // given to more users. This method should not make assumptions that $entity
   +    // is a configuration entity in case we decide to remove the following
   +    // condition.
   +    if (!($entity instanceof ConfigEntityInterface)) {
   +      return $form;
   +    }
   

   Did you have to add this, or just to be explicit? Content entities have their own delete form class and won't work with this anyway, but I guess there could be other types of entities, theoretically ;)

6. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -264,6 +264,20 @@ public function calculateDependencies() {
   +  public function onDependencyRemoval(array $dependencies) {
   +    $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
   +    $definition = $field_type_manager->getDefinition($this->getType());
   +    $changed = $definition['class']::onDependencyRemoval($this, $dependencies);
   +    if ($changed) {
   +      $this->save();
   +    }
   +    return $changed;
   

   "Implementations should not save the entity" ?

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -286,4 +286,26 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
   +      foreach ($field_definition->default_value as $default_value) {
   +        if (is_array($default_value) && isset($default_value['target_uuid'])) {
   ...
   +          if ($entity && in_array($entity, $dependencies[$target_entity_type->getConfigDependencyKey()])) {
   +            $field_definition->default_value = [];
   +            $changed = TRUE;
   +          }
   

   Shouldn't we only remove the specific item instead of resetting the complete array?

8. +++ b/core/modules/config/src/Tests/ConfigDependencyWebTest.php
   @@ -0,0 +1,133 @@
   + * @file
   + * Definition of Drupal\config\Tests\ConfigDependencyWebTest.
   

   Contains ;)

9. +++ b/core/modules/config/src/Tests/ConfigImporterTest.php
   @@ -416,10 +420,10 @@ function testSecondaryUpdateDeletedDeleteeFirst() {
   -    $this->assertEqual($deleter->label(), $values_deleter['label']);
   +    // Both entities are deleted. ConfigTest::postSave() causes updates of the
   +    // deleter entity to delete the deletee entity. Since the deleter depends on
   +    // the deletee, removing the deletee causes the deleter to be removed.
   +    $this->assertFalse($entity_storage->load('deleter'));
        // @todo The deletee entity does not exist as the update worked but the
        //   entity was deleted after that. There is also no log message as this
        //   happened outside of the config importer.
   

   Do we also need to update the @todo below, maybe just remove it?

10. +++ b/core/modules/field/src/Entity/FieldConfig.php
    @@ -182,6 +182,7 @@ public function calculateDependencies() {
     
    +    parent::preDelete($storage, $fields);
    

    Nice example why it might be good to call the parent, eventually we might add something to Entity::preDelete() ;)

11. +++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityStorageTest.php
    @@ -728,6 +739,11 @@ public function testDeleteRevision() {
    +    // Dependencies are tested elsewhere.
    +    // @see \Drupal\config\Tests\ConfigDependencyTest
    

    Why not use write "Dependencies are tested in \class\name" ? :)

+++ b/core/lib/Drupal/Core/Config/ConfigManager.php
@@ -433,6 +434,14 @@ protected function callOnDependencyRemoval(ConfigEntityInterface $entity, array
+        array_map(function ($entity) { return $entity->getConfigDependencyName();}, $affected_dependencies[$dependency_type]),

not sure about coding standard for inline anonymous functions, should there be a space after ;?

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceFieldDefaultValueTest.php
@@ -137,18 +140,22 @@ function testEntityReferenceDefaultConfigValue() {
     // Check that the field does not have a dependency on the default value
     // after deleting the node type.
     $referenced_node_type->delete();
     \Drupal::configFactory()->reset('field.field.node.reference_content.' . $field_name);

weird that we need the reset her, why is that exactly? anything is reset after the drupalPostForm(), so it is the node type delete that somehow doesn't update the cache? but resaving the field should update that?

Thanks. Nothing left to complain about, this looks great, awesome work, took us a long time to get there, was not sure we'd make it :)

Committed/pushed to 8.0.x, thanks!

#2657734: calculateDependencies() return value is double documented. FYI, in case anyone wants a different comment. Actually, we can use your opinion on what the wording should be.