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
- Install standard
- Install Scheduler
- Edit Page content type and enable for scheduled publishing
- Add plain text field to the Page content type
- 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
Comment | File | Size | Author |
---|---|---|---|
#38 | 36-38-interdiff.txt | 7.15 KB | alexpott |
#38 | 2850973-38.patch | 13.35 KB | alexpott |
Comments
Comment #2
kferencz91 CreditAttribution: kferencz91 commentedComment #3
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi 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
Comment #4
kferencz91 CreditAttribution: kferencz91 commentedSorry 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! :)
Comment #5
ericpughI wasn't able to reproduce on 8.2.6
Comment #6
kferencz91 CreditAttribution: kferencz91 commentedI 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.
Comment #7
kferencz91 CreditAttribution: kferencz91 commentedAny 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.
Comment #8
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedYes, I can replicate this on 8.2.2 and 8.2.6. The steps are even simpler:
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 ...
Comment #9
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedIf 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.
Comment #10
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI'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.
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
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:
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
Comment #11
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAltered summary to point new users to the latest investigation in #10
Here is the Scheduler module page
Comment #13
sassafrass CreditAttribution: sassafrass as a volunteer commentedI 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.
Comment #14
alexpottI think this is because the \Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval() is doing:
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.
Comment #15
alexpottThis is at least a major issue because there is unexpected data loss.
Here is a patch that fixes the issue. Now for tests...
Comment #16
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks @alexpott for looking at this. I will re-run my manual testing from #10 above with the patch applied and see what happens.
Comment #17
alexpottHere'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.
Comment #18
alexpottComment #19
alexpottComment #20
alexpottI'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.
Comment #21
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedGood 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.
Comment #22
alexpottHere'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.
Comment #23
chr.fritschI 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:
That sounds a bit strange
Comment #25
alexpottThanks for the review @chr.fritsch - how about this?
Comment #26
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedNit pick time:
Remove first 'the'
Comment #27
alexpott@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...
Comment #28
alexpottComment #29
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedIn 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)
change the second line to "as entities are processed and either fixed or deleted.
Likewise with the identical comment for
$current_dependencies
Comment #30
alexpott@jonathan1055 thanks for the review. I've tried to make the
$current_dependencies
more explicit as to when it changes.Comment #31
dawehnerJust 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.
$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?
is repetitive, but @alexpott said, there is a potential optimization to be done later.
Comment #32
alexpott@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()
Comment #33
dawehnerThis 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.
Comment #34
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI 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.
Comment #35
catchNot really enough to mark this needs work for, but enough to not commit it yet:
We might want to open a follow-up to rename this method as well, since it uses dependents rather than dependencies.
Very minor, but this reads as 'The variables are $current_dependencies is'.
What about:
So formatted more like a definition list.
Also, the order of the variables in the comment is different to the order they get assigned.
Should this be $dependency? Especially since we're using dependent as an adjective in the comment.
Comment #36
alexpottThanks for the review @catch
dependency
consistently as a noun instead ofdependent
makes the code easier to read.As everything was a rename or moving code comments leaving at rtbc.
Comment #37
catchI'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?
Comment #38
alexpott@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.
Comment #40
catchCommitted/pushed to 8.5.x, thanks!
Needs a re-roll for 8.4.x
Comment #41
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks @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
Comment #43
catchArgh good spot. Reverted and re-committed to both branches.
Comment #44
andypostStrange commit message http://cgit.drupalcode.org/drupal/commit/?id=9bcfe6d59f16720eb7e536cca6a...
Comment #48
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedJust 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)