Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Uninstalling modules can result in configuration entities being deleted that should not be.
Steps to reproduce:
- Install standard install profile
- Create a content type
- Uninstall the Menu UI module
- See that the body field on the new content type will be deleted!!!!
Proposed resolution
Fix config entity dependency graph to sort by number of vertices from the thing being removed.
The way the current directed acyclic graph is built does not have enough information to produce a consistent topographical sort. We need to add the module and theme information to the graph and change the direction to make the sort consistent.
HEAD behaviour
PATCH behaviour
Remaining tasks
User interface changes
None
API changes
?
Data model changes
?
Comment | File | Size | Author |
---|---|---|---|
#32 | 2754477-8.2-32.patch | 611 bytes | alexpott |
#32 | 2754477-32.patch | 15.99 KB | alexpott |
#25 | 2754477-25.patch | 15.4 KB | alexpott |
#23 | 2754477-23.patch | 15.07 KB | alexpott |
#23 | 20-23-interdiff.txt | 3.33 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottComment #4
alexpottThe graph created by the ConfigDependency was the wrong way around - so that the topographical sort didn't quite work as expected.
Comment #5
mr.baileysJust tried to install 8.2.x with this patch applied, and am getting
Comment #7
alexpottComment #8
alexpottHere's a test only to show that the current test only passes because the entity IDs created in a alphabetic sorting order :)
I think all we need is a web test of the steps-to-reproduce and a better issue summary to explain how the fix is done.
Comment #12
alexpottFixing the tests and improves ConfigDependencyTest to ensure alphabetical order has not impact.
Comment #14
alexpott#2692247: Pre-existing 'forum' content type prevents forum module installation means we need to do this on 8.2.x first as
core.entity_form_display.taxonomy_term.forums.default.yml
moved.Comment #15
alexpottComment #18
mr.baileysApplied the patch and went through the STR in #2751495-7: Uninstalling Scheduler 8.x-1.0-alpha1 deletes all fields on content types, scheduler is now properly uninstalled without removing fields and other configuration from the article content type.
Comment #19
alexpottComment #20
alexpottComment #21
alexpottSo this variation of sorting covers the Menu UI uninstall case so I think adding a specific test for that is unnecessary.
Comment #22
alexpottComment #23
alexpott@swentel pointed out that the test talks about Entity A, B, C and D - this was because I renamed them in one version of the patch. However I realised that we wanted to test several different alphabetical orders so I reverted back to numbered variables and introduced the data provider approach.
Comment #24
swentel CreditAttribution: swentel as a volunteer commentedWent over the patch a couple of times, with @alexpott
Also did a manual test trying to uninstall menu ui and it now behaves like it should.
Only one more nit, but can be fixed on commit. Moving to RTBC to get more eyes on it.
$entity_id_suffixes isn't in the docblock, it also doesn't really say what it will test
Comment #25
alexpott@swentel good point.
Comment #26
alexpottComment #27
alexpottCreditting @swentel and @mr.baileys because they brought this issue to my attention and I discussed it with them. Plus they both manually tested the patch.
Comment #28
catchAll the individual lines in the patch look fine, and conceptually this makes sense, but this is a tricky issue to be certain about.
However the added test coverage looks good, and if we find another edge case, we can add even more test coverage then. Also generally it's cool that we have a system robust enough that this is fixable, makes a nice change from the 6-8 migrations having to deal with bizarre data-inconsistencies in 6.x databases.
Leaving RTBC a bit (and would like this to sit in 8.2.x before it goes into 8.1.x), but happy with the RTBC.
Comment #30
alexpottHere's the patch for 8.1.x
Comment #32
alexpottInteresting the 8.1.x patch found another missing dependency.
Comment #33
alexpottBack to rtbc because the missing dependency in the default config means we don't sort in the correct order. The non-deterministic nature of topographical sorts when the dependencies are not correct means that in 8.1.x core.entity_view_display.taxonomy_term.forums.default is created before field.field.taxonomy_term.forums.forum_container. In 8.2.x this is not the case because the config is optional so the graph is different.
Comment #34
alexpott#32 added a missing dependency to 8.2.x - @xjm correctly pointed out that this shows missing test coverage - i opened #2755843: The order in which config is saved affects dependency calculations to cover this case. If that patch doesn't land by the time we commit this to 8.1.x we should commit the 8.2 patch in #32 to keep the branches aligned.
Comment #35
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedFor D8.1.3 I have manually re-tested the steps in #2751495-7: Uninstalling Scheduler 8.x-1.0-alpha1 deletes all fields on content types with 2754477-32.patch. The uninstall works fine and the fields are not deleted.
Comment #36
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedJust for info, this core patch also fixes #2652036: Field widget set incorrectly after Scheduler module re-install and the un-install is much cleaner. Great news, thank you. Hopefully it can be committed to 8.1.x soon.
Comment #39
catchCommitted df49801 and pushed to 8.2.x. Committed 84c3578 and pushed to 8.1.x. Thanks!
Comment #40
socketwench CreditAttribution: socketwench at FFW commentedSomehow this change has broken one of Flag's submodules: #2757395: Flag 8.x HEAD is broken because views need to depend on the flag that provides the relationship
Comment #41
alexpott#2757395: Flag 8.x HEAD is broken because views need to depend on the flag that provides the relationship is occurring because of a missing dependency. I've posted a patch on the issue.
Comment #42
gclicon CreditAttribution: gclicon at Achieve Internet commentedWas there a specific reason for changing the structure of the return value for getDependentEntities()? Previously an array of arrays was return. With this update the an array of objects is returned instead. This breaks features #2761927: Fatal error: Cannot use object of type ConfigEntityDependency as array. If it was on purpose then changes will need to be made to the corresponding modules. If not, lets look into obtaining the same results without changing the structure of the return value.
Comment #43
alexpott@gclicon I changed it to match the documentation - my bad. It was done because the graph now contains more objects which are not config entities. Prior to this change it only had config entities.
Comment #44
gclicon CreditAttribution: gclicon at Achieve Internet commentedAh that makes sense. It was hard to follow why the change actually happened. So I wanted to check in first before fixing features. Thanks for the info and following up in the features issue as well. I'll jump over there to test the patch that was posted there.
Comment #45
ayalon CreditAttribution: ayalon commentedI think this patch introduced huge performance issues. It seems, that we are not the only on affected. But using page with more than 8 content types is almost unusable with features.
https://www.drupal.org/node/2765911
Someone has an idea what could be the reason for that?
Comment #46
arknoll CreditAttribution: arknoll commentedThis change is causing our site installs to fail. It is failing on the view mentioned and any view with contains a relationship to a taxonomy term.
Comment #47
arknoll CreditAttribution: arknoll commentedIt appears that my issue in #46 was due to the new way dependencies are discovered/arranged. I moved the offending views to a later enabled module and the dependency problem was resolved.
To be clear this was not a problem in 8.1.3 and before. It became a problem when updating to 8.1.4 and later.