Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
Follow-up to #1002164: The Book module can be uninstalled with nodes with a book node type still existing: If you locally delete the node.type.book config and push changes to a site with book nodes, it's going to delete the node type and break your book nodes. The problem exists generally for all content based upon an entity bundle that is defined in config.
Proposed resolution
Write a config import validator that checks for content before deleting an entity bundle.
Remaining tasks
- Review
- Commit
User interface changes
-
API changes
-
Comment | File | Size | Author |
---|---|---|---|
#23 | 2392351-2.23.patch | 8.51 KB | alexpott |
#23 | 19-23-interdiff.txt | 1.64 KB | alexpott |
#19 | interdiff.txt | 2.24 KB | swentel |
#19 | 2392351-19.patch | 8.53 KB | swentel |
#18 | interdiff.txt | 4.13 KB | swentel |
Comments
Comment #1
alexpottComment #2
alexpottPatch attached adds a event subscriber to check any delete of config entities that are bundles. If they are in use it prevent config import.
Comment #3
yched CreditAttribution: yched commentedDon't we have a hadData() method for that already ?
Side note : that is a bit verbose :-)
Accessing the list of changes is likely to be the primary task for any event class reacting to "config import". Could ConfigImporterEvent provide a helper ->getChangeList($op) method to simplify that ?
Makes sense for a "config import" event to provide direct access to the list of changes in the import ?
Comment #4
alexpottRe #3
Comment #5
fagoPatch looks great, only found some nitpicks:
Minor but it are $ids, not $entities.
they are used?
Comment #6
yched CreditAttribution: yched commented#3.1 - ah, true, the existing one is not by bundle :-/
That could be easily added without an API break, though ?
#3.2 - awesome :-)
Yay, I hesitated to suggest that change earlier on.
s/Is/Does, though :-)
Comment #7
alexpott@fago thanks for the review
Comment #8
fagoLooks like entities was wrongly replaced in the message with entity_ids.
Comment #10
alexpottalexpott--
Comment #11
yched CreditAttribution: yched commentedTypo in #6 still needs adressing :-)
Other than that, looks RTBC ?
Comment #12
swentel CreditAttribution: swentel commentedMaybe 'There are existing entities of type .. ' instead of existing at the end ? Sounds better in my mind.
Comment #13
alexpottAddressed #12 and #6. Thanks for all the reviews.
Comment #14
swentel CreditAttribution: swentel commentedLooks great.
Comment #15
Wim LeersBeautiful patch: beautiful code & beautiful test.
Two nits that could be fixed on commit:
Nit: "list of differences" vs. "changelist".
Nit: Needs fully qualified classname.
Comment #16
webchickActually, I think I'd love to see that test hardened a little bit, if possible. It was actually not obvious except from having swentel walk me through it why the presence or absence of the string "There are no configuration changes to import." meant things were working. And from looking at other tests in that file, most of them both assertText on the UI but also assertFoo to make sure things are working properly under the hood as well.
In other words:
Apart from that, and the things Wim mentioned, this looks good to go.
Comment #17
swentel CreditAttribution: swentel commentedI'm quickly updating the test
Comment #18
swentel CreditAttribution: swentel commentedMore asserts. Also address the first from Wim's review. Couldn't come up with something for 2 though ..
Comment #19
swentel CreditAttribution: swentel commentedGave the assert on '5 removed' another thought and removed it. It's kind of silly to be looking for this number, especially if at some point we change something in the generation of fields, displays etc. Having to change the number is not worth asserting for.
Comment #20
Wim LeersComment #21
catchExtreme nitpick but why 'differences' and not just 'changes'?
Sorry don't love the assignment here - can we just move that to the line above too?
This seems like something we'd probably want to re-use elsewhere too?
Comment #22
yched CreditAttribution: yched commented@catch:
Made that same remark in #3 / #6. ContentEntityStorage::hasData() should have an optional $bundle param.
But IMO it would be better to do as part of or after #2391829: ContentUninstallValidator relies on the not required ContentEntityStorage::hasData() method (left a note in comment #12 over there), keeping the manual implementation here for now.
Comment #23
alexpottre #21
Comment #24
alexpottSetting to rtbc since the changes in #23 are so minor
Comment #26
alexpottrandom unrelated fail.
Comment #29
catchAvoiding the duplication of code in the other issue seems fine.
Committed/pushed to 8.0.x, thanks!