Problem/Motivation

In certain situations uninstalling a module results in unexpected changes to configuration entities. For example, if you uninstall the Scheduler module entity display information for fields is removed from the form displays.

This issue was originally created in the Scheduler queue, so comments 1-9 are background only. Jump to #10 for the details now that this is in the core queue.

Steps to reproduce

  1. Install standard
  2. Install Scheduler
  3. Edit Page content type and enable for scheduled publishing
  4. Add plain text field to the Page content type
  5. Uninstall Scheduler

Now when you create or edit a 'page' node the body and text fields are not shown in the form.

Proposed resolution

The problem is occurring because \Drupal\Core\Config\Entity\ConfigEntityInterface::onDependencyRemoval() is being called with an incorrect list of dependencies which includes dependencies that will not be affected by the removal. This is because in \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() when we call $this->callOnDependencyRemoval() we're using the original list - we are removing the fixed dependencies from the list but not the things that are just not affected anymore.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kferencz91 created an issue. See original summary.

kferencz91’s picture

jonathan1055’s picture

Hi kferencz91,

Sorry to hear you have a problem. I wil try to reproduce it, given your steps above. No one has noticed this before, and you are right that it is important to be able to uninstall the Scheduler module without breaking anything else. I am sure I have done this OK with D8 before. What Core version are you using?

Jonathan

kferencz91’s picture

Sorry for the delayed response, @jonathan1055. I did not get notified and therefore had no idea you responded!

The version of core that is being used on this particular website is 8.2.2. I have not attempted to replicate this on a completely fresh install, so it is possible that some other module may be conflicting, but I appreciate your trying to replicate the problem. Please follow-up with what you are able to find! :)

ericpugh’s picture

I wasn't able to reproduce on 8.2.6

kferencz91’s picture

I just tried with a completely fresh 8.2.6 install with no other modules installed besides core, and it did the exact same thing.

Steps:

1. Install the Scheduler module
2. Create a content type with additional fields beyond the 'body'. (I created a single, plain-text field called "Test")
3. Enable scheduler for that content type
4. Create a node of that content type. Save the content type without setting the scheduled date.
5. Edit the node again to save the scheduled date.
6. Edit the node again and remove the scheduled date completely.
7. Disable scheduler options for the content type.
8. Uninstall the module
9. Try to edit the content type. No other fields will appear in the edit interface, but they will appear in the overall node page.

kferencz91’s picture

Any luck in replicating this? Happening again on another totally fresh 8.2.6 install. No other modules installed, just Scheduler.

Including a screen grab of the warning message right before uninstall to see what exactly is modified during an uninstall.

jonathan1055’s picture

Yes, I can replicate this on 8.2.2 and 8.2.6. The steps are even simpler:

  1. Install clean 8.2.6
  2. Install Scheduler (I used the latest alpha2+25)
  3. Create a new content type and enable for scheduled publishing
  4. Add a plain text field to the content type
  5. Create a node for this type, adding value for the title. body and plain text field
  6. Uninstall Scheduler (no need to enter any publish-on date)

Now when you edit the node the body and text field are not shown in the form. But when browsing the node the data and values are shown.

We had something like this before, in #2751495: Uninstalling Scheduler 8.x-1.0-alpha1 deletes all fields on content types which was closed because the core problem was fixed in #2754477: Unexpected config entity delete due to dependency calculations.

Needs some more investigation ...

jonathan1055’s picture

If you look at the uninstall screenshot I uploaded in #2754477-35: Unexpected config entity delete due to dependency calculations after the fix, it was much cleaner. It seems that that fix has been reverted or regressed somehow.

jonathan1055’s picture

Project: Scheduler » Drupal core
Version: 8.x-1.0-alpha2 » 8.3.x-dev
Component: Code » configuration entity system
Priority: Major » Normal
FileSize
160.87 KB
147.97 KB

I've done some more investigation on this problem and I've reduced it to just five elements for the reproduction steps, listed A-E to make it easier to refer to in different sequences.

A = install core (either 8.2.6 or 8.3-rc1 or 8.3.x dev)
B = install Scheduler module (any 8.x)
C = edit Page content type and enable for scheduled publishing
D = add plain text field to the Page content type
E = uninstall Scheduler.

Executing ABCDE in this order causes the problem. This can be demonstrated by attempting to add a Page node and the body field will not be displayed in the form.

Executing ABDCE also has the same failure, i.e if the text field is added before the Page content type is enabled then we still get the problem.

For both of the above, the uninstall process makes unwanted updates to the entity form display for node.page.default
uninstall deletes form display

Executing ABCE causes no problem, i.e. if the text field is not added then it all works fine.

Executing ADBCE also has no problem. This is important, because it shows that if the text field is added before Scheduler is installed, then we do not get the problem. i.e. it is not just the fact that the field is added which causes the fault, it is the timing of that operation relative to when the contrib module is installed.

When there is no problem the uninstall does not make any changes to the form display:
uninstall leaves form display

I am moving this into the Core 'configuration entity system' queue as it does feel very similar to #2754477: Unexpected config entity delete due to dependency calculations

jonathan1055’s picture

Issue summary: View changes

Altered summary to point new users to the latest investigation in #10

Here is the Scheduler module page

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sassafrass’s picture

I experienced the same issue using D8.3.7. Upon removing all scheduling values from content, then uninstalling the module, fields are disabled from on the Content type edit form. The content is still viewable but no longer editable because the fields are disabled. This can be recitfied be re-enabling the fields but it is unexpected behavior.

alexpott’s picture

I think this is because the \Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval() is doing:

    foreach ($dependencies['config'] as $entity) {
      if ($entity->getEntityTypeId() == 'field_config') {
        // Remove components for fields that are being deleted.
        $this->removeComponent($entity->getName());
        unset($this->hidden[$entity->getName()]);
        $changed = TRUE;
      }
    }

when a configurable field is disappearing but we're not doing the same for base fields - and now that #2282119: Make the Entity Field API handle field purging is fixed and base fields can be purged this is a bigger problem.

alexpott’s picture

Priority: Normal » Major
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
5.41 KB

This is at least a major issue because there is unexpected data loss.

Here is a patch that fixes the issue. Now for tests...

jonathan1055’s picture

Thanks @alexpott for looking at this. I will re-run my manual testing from #10 above with the patch applied and see what happens.

alexpott’s picture

Here's a test for the config dependency part of the change. The problem is happening because we're calling \Drupal\Core\Config\Entity\ConfigEntityInterface::onDependencyRemoval() with an incorrect list of dependencies.

Still need tests for the entity display part but that is a bit simpler.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Title: Uninstalling Module Breaks Node Edit Interface » ConfigEntityInterface::onDependencyRemoval() called with incorrect dependency list
Issue summary: View changes
Issue tags: -Needs tests

I've created #2924256: Entity displays need to depend on the module that provides base fields for the base field stuff so we can just concentrate on the configuration side of the issue here. Also I've updated the issue summary to be only about the configuration issue.

jonathan1055’s picture

Good idea to raise the separate issue. I had noticed that the Scheduler base fields do not get removed on uninstall, but not got round to raising the issue. Now that I have just released Scheduler 8.x-1.0 it will be timely to have these two issues fixed.

I've tested patch 15 on my manual steps reported in #10 and both of the failing scenarios are fixed. Excellent - thank you. Interestingly the sequence ABCDE still shows "Entity form display - node.page.default" on the confirmation page as in my first image whereas with ABDCE (where the text field is added before the content type is enabled) we now get a confirmation without this, as in the second image. But in both cases the body and text fields remain available in subsequent node editing.

I have tidied up the issue summary, as per the simplified instructions in #10, removed the original summary (as this can be found when viewing history) and put back the link to comment #10 to save people wasting time on 1-9.

alexpott’s picture

Here's a new patch with the base field stuff removed and more docs to make it a bit easier to understand what is going on in \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval().

@jonathan1055 thanks for confirming the patch fixes the problem. The reason that ABCDE still shows "Entity form display - node.page.default" is because when you do things that way the entity form display gets a dependency on the scheduler module and has to be fixed.

chr.fritsch’s picture

I have tested that patch with Thunder, which contains the scheduler module, and now uninstalling scheduler works without breaking the article form.

Just found one nitpick:

+++ b/core/lib/Drupal/Core/Config/ConfigManager.php
@@ -296,8 +296,18 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
+    // Store the list in three variables so we can determine the how the
+    // dependency list has changed as things are fixed by calling the

That sounds a bit strange

The last submitted patch, 22: 2850973-21.test-only.patch, failed testing. View results

alexpott’s picture

Thanks for the review @chr.fritsch - how about this?

jonathan1055’s picture

Nit pick time:

+    // us to determine the how the dependency graph changes as entities are

Remove first 'the'

alexpott’s picture

@jonathan1055 thanks for the review. I'd be really interested in comments from any of the reviewers about the documentation in \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval() once this patch has been applied. I think this needs looking at the entire method rather than just the patch context so I'm going to paste the whole lot here to make it easy for everyone...

  public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names, $dry_run = TRUE) {
    // Determine the current list of dependent configuration entities and set up
    // initial values.
    $dependency_manager = $this->getConfigDependencyManager();
    $dependents = $this->findConfigEntityDependentsAsEntities($type, $names, $dependency_manager);
    // Store the list of dependencies in three separate variables. This allows
    // us to determine how the dependency graph changes as entities are fixed by
    // calling the onDependencyRemoval() method. The variables are:
    // - $dependents is the list of dependencies to process. This list changes
    //   as entities processed and are fixed or deleted.
    // - $current_dependencies is the list of dependencies on $names. This list
    //   changes as entities processed and are fixed or deleted. This list is
    //   passed to onDependencyRemoval() as the list of affected entities.
    // - $original_dependencies is the list of original dependencies on $names.
    //   This list never changes.
    $original_dependencies = $current_dependencies = $dependents;
    $affected_uuids = [];

    $return = [
      'update' => [],
      'delete' => [],
      'unchanged' => [],
    ];

    // 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
    // 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) {
        // Clone the entity so any changes do not change any static caches.
        $dependent = clone $dependent;
      }
      $fixed = FALSE;
      if ($this->callOnDependencyRemoval($dependent, $current_dependencies, $type, $names)) {
        // Recalculate dependencies and update the dependency graph data.
        $dependent->calculateDependencies();
        $dependency_manager->updateData($dependent->getConfigDependencyName(), $dependent->getDependencies());
        // Based on the updated data rebuild the list of current dependencies.
        // This will remove entities that are no longer dependent after the
        // recalculation.
        $current_dependencies = $this->findConfigEntityDependentsAsEntities($type, $names, $dependency_manager);
        // Rebuild the list of entities that we need to process using the new
        // list of current dependencies and removing any entities that we've
        // already processed.
        $dependents = array_filter($current_dependencies, function ($current_dependency) use ($affected_uuids) {
          return !in_array($current_dependency->uuid(), $affected_uuids);
        });
        // Ensure that the dependency has actually been fixed. It is possible
        // that the dependent has multiple dependencies that cause it to be in
        // the dependency chain.
        $fixed = TRUE;
        foreach ($dependents as $key => $entity) {
          if ($entity->uuid() == $dependent->uuid()) {
            $fixed = FALSE;
            unset($dependents[$key]);
            break;
          }
        }
        if ($fixed) {
          $affected_uuids[] = $dependent->uuid();
          $return['update'][] = $dependent;
        }
      }
      // If the entity cannot be fixed then it has to be deleted.
      if (!$fixed) {
        $affected_uuids[] = $dependent->uuid();
        // 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 list of affected UUIDs to filter the original list to work out
    // which configuration entities are unchanged.
    $return['unchanged'] = array_filter($original_dependencies, function ($dependent) use ($affected_uuids) {
      return !(in_array($dependent->uuid(), $affected_uuids));
    });

    return $return;
  }
alexpott’s picture

Issue tags: +Configuration system
jonathan1055’s picture

In response to your request for docs review, this is a complex bit of processing but the comments read well and I think I follow what you are doing. If I had to really understand the details behind the calls it would take a while but I think it would make sense.

I noticed one minor thing which you could change (if my understanding is correct)

// - $dependents is the list of dependencies to process. This list changes
//   as entities processed and are fixed or deleted.

change the second line to "as entities are processed and either fixed or deleted.

Likewise with the identical comment for $current_dependencies

alexpott’s picture

@jonathan1055 thanks for the review. I've tried to make the $current_dependencies more explicit as to when it changes.

dawehner’s picture

Just to be clear, I think these kind of reviews are super hard.

I think the overall fix totally make sense, we basically constantly need to take into account the full dependency tree.

  1. $original_dependencies = $current_dependencies = $dependents;
    First we call it dependencies than dependents. At least as a non native speaker this let's me stop and think: Are these two different things?
  2. array_filter($original_dependencies, function ($dependent) use ($affected_uuids) {
          return !(in_array($dependent->uuid(), $affected_uuids));
        })

    is repetitive, but @alexpott said, there is a potential optimization to be done later.

alexpott’s picture

@dawehner thanks for the review - yeah this is a hard one to review!

@dawehner I agree with #31.1 - does this patch make it clearer?

Re #31.2 opened #2925815: Optimise code in \Drupal\Core\Config\ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval()

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@dawehner I agree with #31.1 - does this patch make it clearer?

This makes it more readable, thank you!

It is sad that there is no way to trigger this problem using core itself. This could have made a perfect regression testcase. For me at least having a kernel test though totally makes sense. It still has the full configuration environment available, so you can actually control each bit of input perfectly.

Yeah, for having follow ups to make it more readable.

jonathan1055’s picture

I think the name change in #32 is good - makes it clearer to understand.
Re-tested the steps in #10 with patch #32, all fixed and still good.

catch’s picture

Not really enough to mark this needs work for, but enough to not commit it yet:

  1. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -295,9 +295,20 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
    +    $original_dependencies = $this->findConfigEntityDependentsAsEntities($type, $names, $dependency_manager);
    

    We might want to open a follow-up to rename this method as well, since it uses dependents rather than dependencies.

  2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -295,9 +295,20 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
    +    // us to determine how the dependency graph changes as entities are fixed by
    +    // calling the onDependencyRemoval() method. The variables are:
    +    // - $current_dependencies is the list of dependencies on $names. This list
    +    //   is recalculated when calling an entity's onDependencyRemoval() method
    

    Very minor, but this reads as 'The variables are $current_dependencies is'.

    What about:

    The variables are:
    
    - $current_dependencies
        The list of...
    

    So formatted more like a definition list.

    Also, the order of the variables in the comment is different to the order they get assigned.

  3. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -305,65 +316,60 @@ public function getConfigEntitiesToChangeOnDependencyRemoval($type, array $names
    -    while ($dependent = array_pop($dependents)) {
    +    while ($dependent = array_pop($dependencies_to_process)) {
           /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $dependent */
           if ($dry_run) {
             // Clone the entity so any changes do not change any static caches.
             $dependent = clone $dependent;
           }
           $fixed = FALSE;
    -      if ($this->callOnDependencyRemoval($dependent, $original_dependencies, $type, $names)) {
    +      if ($this->callOnDependencyRemoval($dependent, $current_dependencies, $type, $names)) {
             // Recalculate dependencies and update the dependency graph data.
             $dependent->calculateDependencies();
    

    Should this be $dependency? Especially since we're using dependent as an adjective in the comment.

alexpott’s picture

Thanks for the review @catch

  1. Created #2926729: Rename ConfigManager::findConfigEntityDependentsAsEntities() to be ::findConfigEntityDependenciesAsEntities
  2. Reworked comments to not be a list as it is clearer and adheres better to the documentation standards. I'm not sure that we can do the suggested list format in #35.2
  3. I agree... using dependency consistently as a noun instead of dependent makes the code easier to read.

As everything was a rename or moving code comments leaving at rtbc.

catch’s picture

+++ b/core/lib/Drupal/Core/Config/ConfigManager.php
@@ -292,78 +292,86 @@ public function findConfigEntityDependentsAsEntities($type, array $names, Config
-
     // 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
     // node types are fixed before processing the node type's other
     // dependencies.
-    while ($dependent = array_pop($dependents)) {
-      /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $dependent */
+    while ($dependency = array_pop($dependencies_to_process)) {
+      /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $dependency */
       if ($dry_run) {

I'm confused by this now. A dependent is something that depends on something else. A dependency is something that is depended on. So shouldn't this actually be dependents after all?

alexpott’s picture

@catch good point. Dependent is correct - what we asking here is what depends on me - hence dependent so you're right. All the lists are lists of dependents. I've swapped everything around and closed the followup since that is named correctly.

One of the problems here is that dependency and dependent feel very similar.

  • catch committed f9cde09 on 8.5.x
    Issue #2850973 by alexpott, kferencz91, jonathan1055, dawehner:...
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs re-roll

Committed/pushed to 8.5.x, thanks!

Needs a re-roll for 8.4.x

jonathan1055’s picture

Thanks @catch.

I noticed in the commit there are extra changes not from patch #38. These are to do with changing RouteEnhancer to DeprecatedRouteEnhancer. I do not know if you intended this here, but just wondering if you could put a note to explain, so we know what is what when doing the 8.4 re-roll.

Jonathan

  • catch committed 07c81f1 on 8.5.x
    Revert "Issue #2850973 by alexpott, kferencz91, jonathan1055, dawehner:...
catch’s picture

Status: Needs work » Fixed

Argh good spot. Reverted and re-committed to both branches.

andypost’s picture

  • catch committed e382945 on 8.5.x
    Issue #2850973 by alexpott, kferencz91, jonathan1055, catch, dawehner:...

  • catch committed 32c3122 on 8.4.x
    Issue #2850973 by alexpott, kferencz91, jonathan1055, catch, dawehner:...

Status: Fixed » Closed (fixed)

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

jonathan1055’s picture

Just re-tested and can confim that this is fixed with the latest 8.5 dev and 8.4 dev
Thanks catch and alexpott

Jonathan
(ps sorry for late reply, been away offline during December)