Problem/Motivation

In #2416109: Validate configuration dependencies before importing configuration we are considering validating config dependencies before import. The problem is there are many ways to get your active configuration to contain config entities that depend on config entities that do not exist. For example, if you install the standard profile and delete the article content type its corresponding RDF mapping config entity will not be deleted. Active configuration should never get into a state where config entities depend on other config entities that do not exist.

Proposed resolution

Fix dependencies when configuration is deleted. Fixing means either deleting or updating the configuration so the dependency no longer exists. The user will be informed what is going on on the confirmation form.

deleting a view showing config dependencies

More screenshots are available in #53.

Remaining tasks

  • Add tests
  • Review
  • Commit

User interface changes

New list on configuration entity delete confirm forms.

API changes

None.

Files: 
CommentFileSizeAuthor
#65 2416409.65.patch44.52 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,297 pass(es). View
#65 63-65-interdiff.txt2.03 KBalexpott
#63 2416409.63.patch44.61 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#63 62-63-interdiff.txt3.76 KBalexpott
#62 2416409.62.patch43.54 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#62 61-62-interdiff.txt5.91 KBalexpott
#61 2416409.61.patch40.2 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,206 pass(es). View
#61 49-61-interdiff.txt4.23 KBalexpott
#49 2416409.49.patch40.45 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,308 pass(es). View
#49 47-49-interdiff.txt2.17 KBalexpott
#47 2416409.47.patch40.43 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,289 pass(es), 5 fail(s), and 1 exception(s). View
#47 45-47-interdiff.txt10.34 KBalexpott
#45 2416409.45.patch39.42 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,294 pass(es), 11 fail(s), and 2 exception(s). View
#45 42-45-interdiff.txt5.62 KBalexpott
#42 2416409.42.patch29.88 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,261 pass(es). View
#42 38-42-interdiff.txt7.27 KBalexpott
#38 34-38-interdiff.txt1.81 KBalexpott
#38 2416409.38.patch29.01 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,084 pass(es). View
#34 2416409.34.patch30.83 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,990 pass(es), 2 fail(s), and 0 exception(s). View
#34 26-34-interdiff.txt15.12 KBalexpott
#26 2416409.26.patch19.68 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,202 pass(es). View
#26 24-26-interdiff.txt2.08 KBalexpott
#24 2416409.23.patch19.68 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,928 pass(es). View
#24 20-23-interdiff.txt2.73 KBalexpott
#20 2416409.20.patch20.84 KBbojanz
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#17 2416409.17.patch21.07 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,854 pass(es). View
#17 13-17-interdiff.txt6.34 KBalexpott
#13 2416409.13.patch20.52 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,182 pass(es). View
#13 12-13-interdiff.txt2.48 KBalexpott
#12 2416409.12.patch18.04 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,153 pass(es). View
#12 10-12-interdiff.txt2.4 KBalexpott
#10 2416409.10.patch18.56 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,156 pass(es), 1 fail(s), and 0 exception(s). View
#10 6-10-interdiff.txt5.28 KBalexpott
#6 2416409.6.patch13.28 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,159 pass(es). View
#6 3-6-interdiff.txt10.49 KBalexpott
#3 2416409.3.patch5.45 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,859 pass(es), 1 fail(s), and 1 exception(s). View
#1 2416409.1.patch4.04 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,971 pass(es), 35 fail(s), and 238 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
4.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,971 pass(es), 35 fail(s), and 238 exception(s). View

Here's an implementation - let's see how much breaks.

Status: Needs review » Needs work

The last submitted patch, 1: 2416409.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,859 pass(es), 1 fail(s), and 1 exception(s). View

Going a step further and just deleting the dependencies if they can't be fixed.

Status: Needs review » Needs work

The last submitted patch, 3: 2416409.3.patch, failed testing.

Berdir’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.49 KB
13.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,159 pass(es). View

So the only test fails in #3 were due to testing a situation that should not occur and entity reference default values causing dependencies that can not be fixed. Patch attached fixes both of these things. It adds the ability for field type plugins to fix fields on dependency removal - I don't think we should be deleting a field if the default value of an ER field is removed.

catch’s picture

Title: Prevent deletion of a config entity if another config entity depends on it and it can not fixed » Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted

Trying a re-title.

catch’s picture

Priority: Normal » Major

Also this is at least major.

alexpott’s picture

Issue tags: +Needs tests

The changes to entity reference are implicitly covered in EntityReferenceIntegrationTest but need an explicit test. We need testing of ConfigEntityBase::preDelete too.

Another thought is should we add something to ContentEntity::preDelete() as well. All content dependencies are soft and we implicitly guarantee your config will not crash your site if the content is missing - but things like an ER field could clean themselves up if the default value goes missing.

alexpott’s picture

FileSize
5.28 KB
18.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,156 pass(es), 1 fail(s), and 0 exception(s). View

Fields can use this code to have less custom code.

Status: Needs review » Needs work

The last submitted patch, 10: 2416409.10.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.4 KB
18.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,153 pass(es). View

So we have deletes happening inside of deletes which change the dependency chains :)

alexpott’s picture

FileSize
2.48 KB
20.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,182 pass(es). View

OMG... this actually works...

Just apply the patch - install standard and go and delete a content type! You'll see a list of affected configuration on the confirm form.

alexpott’s picture

Category: Task » Bug report
Priority: Major » Critical

Talked about this with @catch on IRC - I've come round to thinking this is critical. Configuration in your active store that has missing dependencies is an indication of a broken site. It also makes reliable configuration synchronisation impossible.

catch’s picture

Issue tags: +D8 upgrade path

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.

Wim Leers’s picture

Status: Needs review » Needs work

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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
6.34 KB
21.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,854 pass(es). View

Thanks for the review @Wim Leers

  1. Fixed
  2. Fixed
  3. No longer relevant - we recalculate the dependencies after every loop - things are either fixed or deleted.
  4. Fixed
  5. They don't which is a problem - we have this in the installer. The only way to resolve this is to make this take two steps. Maybe we can explore doing two steps in both the installer and entity delete in a follow up?
  6. Well it works :)
  7. Fixed
  8. Fixed
  9. It is in the module installer and it is here too.
  10. Fixed
  11. Fixed

Also prevented the dependency stuff from appearing on content entity delete forms for now.

A potential elephant in the room is permissions - what happens if the user making the change does not have the permissions to make changes to or delete the dependent configuration entities?

alexpott’s picture

@xjm and I talked about the potential elephant. If you have the permission to delete a node type (administer content types) and you don't have the ability to administer fields for that particular content type (administer node fields) then deleting the node type will still delete the fields. I think the same rule applies here. If you have the permission to delete a config entity, then, in the process of doing the delete, you also have the permission to make the system handle the consequences.

catch’s picture

That looks reasonable to me as well.

bojanz’s picture

FileSize
20.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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.

Status: Needs review » Needs work

The last submitted patch, 20: 2416409.20.patch, failed testing.

Status: Needs work » Needs review

bojanz queued 20: 2416409.20.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 20: 2416409.20.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.73 KB
19.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,928 pass(es). View

re #20 we now need to save if onDependency removal does something.

Wim Leers’s picture

#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"

alexpott’s picture

FileSize
2.08 KB
19.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,202 pass(es). View

Fixed #25.

Re #17.5 turns out it is already possible and will significantly improve the performance of the code that loops around - see #2420107: Determine which config entities can be fixed and which will be deleted when a dependency is removed

Status: Needs review » Needs work

The last submitted patch, 26: 2416409.26.patch, failed testing.

Status: Needs work » Needs review

alexpott queued 26: 2416409.26.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 26: 2416409.26.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work

#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.

Wim Leers’s picture

Title: Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted » [PP-1] Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted
Related issues: +#2420107: Determine which config entities can be fixed and which will be deleted when a dependency is removed

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.

alexpott’s picture

alexpott’s picture

Title: [PP-1] Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted » Delete dependent config entities that don't implement onDependencyRemoval() when a config entity is deleted
Issue summary: View changes
Status: Needs work » Needs review
FileSize
15.12 KB
30.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,990 pass(es), 2 fail(s), and 0 exception(s). View
113.28 KB
85.13 KB
145.71 KB
  1. Rerolled
  2. Remove unnecessary code
  3. Fixed a bad assumption about EntityManager::loadEntityByConfigTarget()
  4. Still needs tests
  5. Made some screenshots showing how lovely this change is. I especially like the one deleting the view that shows that the block placement is going.
alexpott’s picture

Issue summary: View changes
Wim Leers’s picture

#34: AWESOME!!! :)

Status: Needs review » Needs work

The last submitted patch, 34: 2416409.34.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
29.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,084 pass(es). View
1.81 KB

So bundles do not have to be configuration entities... I'm a sad panda :(

larowlan’s picture

  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?

larowlan’s picture

Also #34 is the business - @alexpott++

Wim Leers’s picture

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

alexpott’s picture

FileSize
7.27 KB
29.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,261 pass(es). View

re #41 nope not shortcut - it's the test entity type - it uses state for bundles.

re #39

  1. Fixed
  2. Fixed
  3. That won;t be possible because of where StringTranslationTrait is used
  4. We then have to change every subclass of EntityDeleteForm - i've added a protected method to make this easier to test.
  5. :)
  6. & 7. This is really painful - the past we were adding a property to each field whilst we are deleting - this is no longer possible because they are being deleted by the dependency deleter. Adding the property to each field was hacky and so is this but I don't think it is risky at all. A process is never going to concurrently delete two field storages at the same time. And I'm not sure it matters if the flag gets stuck to be honest. 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.

Yes we need tests.

Berdir’s picture

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.

Wim Leers’s picture

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

alexpott’s picture

Issue tags: -Needs tests
FileSize
5.62 KB
39.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,294 pass(es), 11 fail(s), and 2 exception(s). View

Added tests of both the API and UI - found that the preDelete was not using the right method to find and fix/delete config entities :)

Status: Needs review » Needs work

The last submitted patch, 45: 2416409.45.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.34 KB
40.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,289 pass(es), 5 fail(s), and 1 exception(s). View

So... KeyValueConfigEntityStorageTest swaps out config entity storage which makes the assumptions about how we can load all config from the configuration store incorrect. To be able to manage dependencies we need to be able to assess all config together. I think the expectations of being able to swap out the storage on the config entity level are realistic at this point. To quote @Berdir

just like it doesn't support overrides, and all the additional methods that config entity storage requires aren't implemented..

I think we should repurpose #2393751: Move KeyValueEntityStorage to a test folder to remove the ability to use a storage class that does not extend ConfigEntityStorage.

Status: Needs review » Needs work

The last submitted patch, 47: 2416409.47.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
40.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,308 pass(es). View

What we want is the url object :(

Sachini queued 47: 2416409.47.patch for re-testing.

The last submitted patch, 47: 2416409.47.patch, failed testing.

xjm’s picture

Issue summary: View changes

(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). :)

xjm’s picture

Issue summary: View changes

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...

xjm’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
catch’s picture

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.

catch’s picture

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?

Berdir’s picture

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" ? :)

alexpott’s picture

FileSize
4.23 KB
40.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,206 pass(es). View

Thanks @Berdir re #60

  1. Fixed - have no idea why I made that unrelated change
  2. Fixed
  3. Fixed - have no idea why I made that unrelated change
  4. We're building the form so I chose buildForm
  5. Yeah I just wanted to be really really explicit - especially since the class doc does not say this is just for config entities.
  6. Great catch - fixed.
  7. There can be only one default value - we're not searching the default values here we're search the dependencies that are being removed.
  8. Fixed
  9. Removed @todo - it's not relevant now we have the new comment
  10. :)
  11. Fixed
alexpott’s picture

FileSize
5.91 KB
43.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

After discussing with @Berdir decided to key the array of config and content dependencies to make it easy to search. Added a test for EntityReferenceItem::onDependencyRemoval().

Address @catch's point in #59 too

alexpott’s picture

FileSize
3.76 KB
44.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Fixed the test and improved it.

Berdir’s picture

+++ 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?

alexpott’s picture

FileSize
2.03 KB
44.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,297 pass(es). View

Fixed both. The reset was not necessary - over thinking.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

The last submitted patch, 63: 2416409.63.patch, failed testing.

The last submitted patch, 62: 2416409.62.patch, failed testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 59d5c09 on 8.0.x
    Issue #2416409 by alexpott, bojanz: Delete dependent config entities...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

lokapujya’s picture

#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.