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
- When a module is uninstalled, all of its configuration is deleted and no other modules are informed about the change.
Proposed solution
- Write tests to clarify and set expectations in stone.
Comment | File | Size | Author |
---|---|---|---|
#18 | 17-18-interdiff.txt | 1.22 KB | alexpott |
#18 | config.uninstall.18.patch | 3.54 KB | alexpott |
#17 | config.uninstall.17.patch | 2.25 KB | blackra |
#2 | config.uninstall.2.patch | 1.67 KB | sun |
drupal8.config-uninstall.0.patch | 1.69 KB | sun | |
Comments
Comment #1
sunComment #2
sunLatest code extracted from #1626584: Combine configuration system changes to verify they are compatible
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedLooks straight forward.
Comment #4
webchickNo tests?
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedLooks like test would belong in UninstallTest class.
Comment #6
tim.plunkettThis code directly conflicts with #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API, where xjm is planning on adding some tests.
Comment #7
sunSleeping a couple of more nights over this detail (now that the big architectural issues are out of the way)...
I actually think this patch is wrong. ;)
As "usual", we cannot invoke module APIs when uninstalling modules, because the module to be uninstalled is already disabled, so its code is no longer loaded.
That leaves us with a nice problem space.
Comment #8
sunIn any case, this patch is moot.
Comment #9
catchThis looks like yet another case for not allowing disabled modules. Then we could invoke the module APIs on uninstall, because it'd be installed right up until the last moment.
Comment #10
alexpottPostponing on #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed as without that this can not be solved.
This is actually critical because it involves data consistency.
If we have a config entity type that as well as maintaining a file also maintains additional data somewhere else (ie. a database, another file, a webservice, anything) then when we uninstall the module that provides this config entity type none of the hooks to maintain the data will be fired when it removes any existing config entities.
Comment #11
mtiftRelated: #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall
Comment #12
catchComment #13
catchComment #14
alexpottNow that #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed has landed ConfigEntity methods and hooks are firing during module uninstallation. So we just need to add tests to ensure this functionality is tested.
These tests can be added to
Drupal\config\Tests\ConfigInstallTest
,Drupal\config\Tests\ConfigInstallTest
, andDrupal\config\Tests\ConfigOtherModuleTest
.Comment #15
alexpottUpdating title
Comment #16
mtiftUpdating title
Comment #17
blackra CreditAttribution: blackra commentedHere is a patch to attempt to test this. It currently fails because delete() on the StorageController doesn't appear to be being called during the module uninstall.
Comment #18
alexpottYou're correct the hooks are not fired!
Comment #20
alexpott@tim.plunkett and I discussed at BADcamp and realised that the current behaviour is probably correct as this is equivalent for what we do for content entities. When a module that provides a content entity type the database table is removed - no entity hooks are fired for each of the content entities and the table is just dropped. Similarly we should just delete the files and move on.
So the question becomes should we actually even be calling Config::delete here? Since this invokes the Config delete event.
Comment #20.0
alexpottUpdate
Comment #21
catchI'm not sure it's actually correct for content entities. If there's a generic module that handles data for content entities, then it'll end up with stale data. We'd need an equivalent to field_attach_delete_bundle() or similar to get it right.
This is yet another one that comes down to soft-dependencies - i.e. if we don't invoke these hooks on uninstall, then can we actually guarantee that there won't be conflicts when the module is re-installed later?
Comment #22
tim.plunkettI'm not sure if we want to postpone this on #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall or not.
Comment #23
effulgentsia CreditAttribution: effulgentsia commented#2278017: When a content entity type providing module is uninstalled, the entities are not fully deleted, leaving broken reference
Regarding the rest of this issue, I think it got fixed, with tests, in #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall, so closing as a dupe of that.