Problem/Motivation

In some cases, we don't want to delete the whole dependency tree if one configuration entity is being removed.

The Drupal core example are view and form displays.

Assume that you have a module that provides a field (as default configuration, or by providing the field type plugin). That field is added to node articles, together with 15 other fields. The view and form display modes are configured to display that field with a specific formatter/widget.

Right now, if you uninstall that module, it will forcefully remove all your display configurations that contain that field. But that's not what you want, you still want to display all the other fields with their configuration.

Another example is the Search API module, which is ported to 8.x here: https://drupal.org/sandbox/daeron/2091893

It has search servers/backends, for example database or solr. Then you have search indexes which use a specific server, have data sources with fields (can be configurable fields and then need to depend on them) and processors, which are plugins, so we need to depend on all those things to make sure that it's installed/synced in the right order. This is happening here: https://drupal.org/node/2257081.

But if you remove the module that provides the server, we want to update the index to disable itself and remove the dependency on the server, and if you remove a plugin, we want to remove just that, and not delete the whole index, and views that depend on it and so on.

Proposed resolution

Add ConfigEntityInterface::onDependencyRemoval() to allow configuration entities to respond if their any of dependencies is going to be removed (ie. if an entity is going to be deleted or a module uninstalled). This method allows configuration entities to remove dependencies instead of being deleted themselves. When a module is being uninstalled configuration entities can use this method to avoid being unnecessarily deleted. Implementations should save the entity if dependencies have been successfully removed.

For example, entity displays remove references to widgets and formatters if the plugin that supplies them depends on a module that is being uninstalled.

Remaining tasks

Review patch

User interface changes

The message about deleting configuration entities as a result of module uninstall is changed. As a result of this patch we don't know if they will be deleted or just updated so we list affected entities.

API changes

Add ConfigEntityInterface::onDependencyRemoval()

Original discussion of a resolution

Discussed with @alexpott, what we should do is introduce an API that lets us ask a config entity if can continue to exist without that dependency and heal itself. If so, we let it do it, otherwise we delete it.

Deletion happens top down, if you have a search server, a search index that depends on that server and a view against that index, we first need to ask the index what we should do if the server is removed, and let it self-heal, otherwise we'd delete the view because the view can not self-heal itself from the index being removed before we'd learn that we don't have to delete the index.

Not sure if we need two separate methods to check if it can remove the dependency or if we just have on that does it and returns TRUE or FALSE?

We will probably also at least update the uninstall page to explain that not all config would be deleted? But it would be better if we'd already know then what will be deleted and what not, would be very confusing/fragile/scary to silently rely on this. But then we certainly need two methods...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Issue summary: View changes
Berdir’s picture

Issue summary: View changes
alexpott’s picture

Priority: Major » Critical
Issue summary: View changes
Issue tags: +beta target, +Configurables

I think this is a critical issue since unexpectedly and unnecessarily deleting configuration entities is going to cause significant headaches. But since this is not changing data structures and is an API addition I feel this should only be a beta target.

catch’s picture

So self-heal means in the case of a display mode we'd remove the field configuration from it rather than deleting? If so completely agreed on this.

yched’s picture

Ooh yes. Deleting the whole EntityDisplay if one of its field disappears is pretty serious - do we really do that currently ?

Berdir’s picture

Yeah, that's exactly what we do.

catch’s picture

Title: Allow config entities to self-heal themself when dependencies are deleted » Allow config entities to remove dependent configuration keys when dependencies are deleted
Category: Task » Bug report

Re-titling and the current behaviour means we should probably treat this as a bug.

Anonymous’s picture

Issue summary: View changes
Anonymous’s picture

FileSize
3.38 KB

first cut patch, still much to do.

worked on this with alexpott at pre-Drupalcon code sprint.

Anonymous’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: 2260457-9.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
7.61 KB

Let's add a test and fix some of the failures.

Status: Needs review » Needs work

The last submitted patch, 12: 2260457.12.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
8.87 KB

And now fixing the exceptions and updating UI text. Unfortunately I can't see a sensible way of informing the user which entities are going to be "healed" and which are going to be deleted. We could stored what we've done on the ConfigManager and then use a drupal_set_message in ModulesUninstallConfirmForm::submitForm() but this message will quickly become unwieldy if there are lots of entities involved.

The only example I can think of where we need to do this is core is entity displays but at the moment this can never happen since field instances have to be deleted before a module can be installed that would removed them :). This also means that the implementation in EntityTestBase is impossible to test. Potentially we could fix a view that depends on a field - but I'm not sure this is the correct behaviour since this could result in information disclosure depending on what the view is being used for.

Status: Needs review » Needs work

The last submitted patch, 14: 2260457.14.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
10.39 KB

I thought there was a test for that text somewhere :)

alexpott’s picture

Status: Needs review » Needs work
     // Create a dependency on the module that provides the formatter or
      // widget.
      if (isset($component['type']) && $definition = $this->pluginManager->getDefinition($component['type'], FALSE)) {
        $this->addDependency('module', $definition['provider']);
      }

We also can remove components from entity display if they (the component) have a dependency on the module that is being uninstalled.

yched’s picture

// Create a dependency on the module that provides the formatter or
// widget.

In D7 and D8 so far, we fall back to the "default widget/formatter for the field type" when the configured widget or formatter is unavailable.
If we add strict dependencies, then we should remove that fallback mechanism in Widget/FormatterPluginManager::getInstance().

mtift’s picture

+++ b/core/modules/config/tests/config_test/src/Entity/ConfigTest.php
@@ -150,4 +150,23 @@ public function calculateDependencies() {
+    $changed = FALSE;
...
+    if ($changed) {
+      $this->save();
+    }
+  }

This won't ever get called because there is no $changed = TRUE;

Berdir’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
@@ -20,6 +20,13 @@
   /**
    * Enables the configuration entity.
    *
+   * @param \Drupal\Core\Config\Entity\ConfigEntityInterface[]
+   */
+  public function preDeleteFixDependencies(array $dependent_entities);
+
+  /**
+   * Enables the configuration entity.
+   *

Something went wrong here ;)

This added a new method, kept the descripion of the one below and then added a new one for the existing method.

xjm’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
19.24 KB
23.65 KB

Added extensive testing of falling back to default widgets and formatters when a module that provides one that's in use is uninstalled.

Changed method name from preDeleteFixDependencies() to onDependencyRemoval after discussing at Austin.

xjm queued 22: 2260457.22.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2260457.22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
23.66 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 25: 2260457.25.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.13 KB
23.65 KB

Updated patch for field_config -> field_storage_config rename.

alexpott’s picture

FileSize
23.67 KB

Rerolled

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -178,14 +179,47 @@ public function createSnapshot(StorageInterface $source_storage, StorageInterfac
    +    // Give config entities a chance to become independent of the entities we
    +    // are going to delete.
    +    foreach (array_reverse($extension_dependent_entities) as $entity) {
    

    Can we explain the reason for array_reverse() in the comment?

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -157,4 +157,35 @@ public function calculateDependencies();
    +   * This method allows configuration entities to remove dependencies instead
    +   * of being deleted themselves. For example, when a module is being
    +   * uninstalled configuration entities can use this method to avoid being
    +   * unnecessarily deleted.
    

    Can we explain a bit more here what config entities are supposed to do in here? (remove references + save itself)

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -157,4 +157,35 @@ public function calculateDependencies();
    +   * @see \Drupal\Core\Config\ConfigManager::uninstall()
    

    nitpick: see should be below the @params I think, separated with an empty line.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -378,4 +378,32 @@ private function fieldHasDisplayOptions(FieldDefinitionInterface $definition) {
    +          // Revert to the defaults if the plugin that supplies the widget or
    +          // formatter depends on a module that is being uninstalled.
    +          $this->setComponent($name);
    

    In case it already is the default, this would then not remove the dependency and we would still be removed.

    I think that is working correctly with this logic, maybe add a comment to explain this?

  5. +++ b/core/modules/config/src/Tests/ConfigDependencyTest.php
    @@ -146,6 +146,69 @@ public function testDependencyMangement() {
    +    $config_manager->uninstall('module', 'node');
    

    Interesting that we can uninstall node twice ;) It's really "uninstallConfig(), so it's ok.

    Would it be easier to understand if we would make this two test methods?

  6. +++ b/core/modules/field/tests/modules/field_plugins_test/src/Plugin/Field/FieldFormatter/TestTextTrimmedFormatter.php
    @@ -0,0 +1,31 @@
    + * Plugin implementation of the 'field_plugins_test_text_formatter'' formatter.
    

    Double ' ;)

    I also find those "Plugin implementation of the 'X' formatter." weird and useless, but it's everywhere else, so fine ;)

  7. +++ b/core/modules/field/tests/modules/field_plugins_test/src/Plugin/Field/FieldFormatter/TestTextTrimmedFormatter.php
    @@ -0,0 +1,31 @@
    + * @see \Drupal\text\Field\Formatter\TextTrimmedFormatter
    

    Do we really need a @see when we're extending from it? I think that's only useful for things which are not already linked explicitly through code?

  8. +++ b/core/modules/system/src/Form/ModulesUninstallConfirmForm.php
    @@ -147,8 +147,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -      '#title' => $this->t('Configuration deletions'),
    -      '#description' => $this->t('The listed configuration will be deleted.'),
    +      '#title' => $this->t('Affected configuration'),
    +      '#description' => $this->t('The listed configuration will be updated, if possible, or deleted.'),
    

    Hm, so we obviously don't know at this point. Unfortunate, but I'm sure you would have found a way if there would be one ;)

    "This might or might not be deleted. Please confirm." ;)

    The issue summary is still my original one, and it describes that we hoped to implement, a system where we can ask a config entity if it can continue to live, so that we could explicitly separate those here. We should update that.

alexpott’s picture

FileSize
3.69 KB
23.91 KB
  1. Don't think we need to do that actaully. c&p from the loop below - which has an explanation.
  2. Fixed
  3. Fixed
  4. I think that this will never occur because if the default formatter or widget is being removed then the module the provides the field type will be being removed so then the component will be removed already :)
  5. I think it is okay to call $config_manager->uninstall('module', 'node'); twice in the same test since that is precisely the code under test.
  6. Fixed
  7. Fixed
  8. Well this is so tricky in order to work out what can be fixed we need to do a lot of work - I could not see a way to do this elegantly.

Will update the issue summary soon.

dawehner’s picture

Title: Allow config entities to remove dependent configuration keys when dependencies are deleted » Allow config entities to remove dependent configuration keys when dependencies are deleted due to module uninstall

One thing I realized while reading the IS is that it would be nice to have that feature on the actual API level.
So you remove a field instance (via API or UI), it should also adapt the configuration entities much like it now should happen on uninstall.
As discussed with berdir this could be even leveraged to build also a nice UI for the delete confirm forms, but for sure
this is NOT part of the issue. (adapted the issue title to be sure about that).

  1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -178,14 +179,47 @@ public function createSnapshot(StorageInterface $source_storage, StorageInterfac
    +    // Give config entities a chance to become independent of the entities we
    +    // are going to delete.
    +    foreach ($extension_dependent_entities as $entity) {
    

    Once we do things on the actual API level, we would not longer have to do that code in uninstall but it would just bubble up when you remove the entity itself.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -375,8 +375,21 @@ protected function addDependency($type, $name) {
    +  public function getDependencies() {
    +    return $this->dependencies;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function onDependencyRemoval(array $entities = array(), array $modules = array(), array $themes = array()) {
    +  }
    

    I don't really think we should couple the interface to those three dependency types. As berdir said, we hardcode that in multiple places probably already, though there is no reason to hardcode it here. getDependencies() itself also already returns all of them, so the onDependencyRemoval should not try to special case things.

  3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -157,4 +157,40 @@ public function calculateDependencies();
    +   * This method allows configuration entities to remove dependencies instead
    +   * of being deleted themselves. When a module is being uninstalled
    +   * configuration entities can use this method to avoid being unnecessarily
    +   * deleted. Implementations should save the entity if dependencies have been
    +   * successfully removed. For example, entity displays remove references to
    +   * widgets and formatters if the plugin that supplies them depends on a
    +   * module that is being uninstalled.
    

    It would be great to add a todo somewhere, maybe here, to explain that this doesn't happen if an entity is deleted but just if the module is uninstalled atm.

  4. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -157,4 +157,40 @@ public function calculateDependencies();
    +   * @param \Drupal\Core\Config\ConfigEntityInterface[] $entities
    +   *   An array of entity dependencies.
    

    Just a general note, in case we just can depend on config entities we should also rename the variable so that this is clear as well.

  5. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
    @@ -157,4 +157,40 @@ public function calculateDependencies();
    +   * @param $type
    +   *   (optional) The type of dependencies to return. Can be either 'module',
    +   *   'theme', or 'entity'
    ...
    +  public function getDependencies();
    

    I assume this @param is a leftover or you were to lazy to implement it.

  6. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -378,4 +378,32 @@ private function fieldHasDisplayOptions(FieldDefinitionInterface $definition) {
    +      if ($entity instanceof FieldInstanceConfig) {
    

    Let's use FieldInstanceConfigInterface instead

  7. +++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
    @@ -378,4 +378,32 @@ private function fieldHasDisplayOptions(FieldDefinitionInterface $definition) {
    +        $this->removeComponent($entity->getName());
    +        unset($this->hidden[$entity->getName()]);
    

    Is there a reason why removeComponent also doesn't drop it from $this->hidden?

alexpott’s picture

Issue summary: View changes
FileSize
6.34 KB
23.88 KB
  1. Yep this has been a dream of mine for a while - all the custom code in FieldConfig, FieldStorage disappears :)
  2. Agreed - fixed
  3. Added @todo for #2336727: Use configuration dependencies and onDependencyRemoval to standardise dependency management on configuration entity delete
  4. Well this rename would be way beyond the scope of this issue
  5. Yep a leftover of a different approach - fixed
  6. Fixed
  7. Well removing a component moves it to hidden so we also need to remove it from there - changing this behaviour or adding a hideComponent and making removeComponent behave as probably expected is beyond the scope (i think)

I updated the issue summary too.

swentel’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityInterface.php
@@ -157,4 +157,37 @@ public function calculateDependencies();
+   * of being deleted themselves. When a module is being uninstalled
+   * configuration entities can use this method to avoid being unnecessarily

nitpick, could probably use a comma after uninstalled, got confused when reading this

alexpott’s picture

FileSize
1.14 KB
23.88 KB

Switched order of the sentence around.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Seems legit.

catch queued 34: 2260457.34.patch for re-testing.

Status: Reviewed & tested by the community » Needs work

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

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
24.07 KB

Rerolled... minor conflict in EntityDisplayBase due to #2333501: Implement ThirdPartySettingsInterface in EntityView|FormDisplay - I've reordered use statements to be alphabetical. Setting back to rtbc as per #35

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed f022dec on 8.0.x
    Issue #2260457 by alexpott, beejeebus: Fixed Allow config entities to...

Status: Fixed » Closed (fixed)

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

colan’s picture