Problem

  • When a module is uninstalled, all of its configuration is deleted and no other modules are informed about the change.

Proposed solution

  1. Write tests to clarify and set expectations in stone.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Assigned: sun » Unassigned
sun’s picture

Priority: Major » Normal
Status: Active » Needs review
FileSize
1.67 KB
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks straight forward.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

No tests?

moshe weitzman’s picture

Looks like test would belong in UninstallTest class.

tim.plunkett’s picture

Issue tags: +VDC
sun’s picture

Sleeping 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.

sun’s picture

Status: Needs work » Active

In any case, this patch is moot.

catch’s picture

This 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.

alexpott’s picture

Priority: Normal » Critical
Status: Active » Postponed

Postponing 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.

mtift’s picture

catch’s picture

Status: Postponed » Active
catch’s picture

alexpott’s picture

Now 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, and Drupal\config\Tests\ConfigOtherModuleTest.

alexpott’s picture

Title: Upon module uninstallation, Configurables are deleted without invoking module API » Test that upon module uninstallation Configurables are deleted without invoking module API

Updating title

mtift’s picture

Title: Test that upon module uninstallation Configurables are deleted without invoking module API » Add tests that the config entity API is called on module uninstall when config entities are removed

Updating title

blackra’s picture

FileSize
2.25 KB

Here 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.

alexpott’s picture

Title: Add tests that the config entity API is called on module uninstall when config entities are removed » Invoke ConfigStorageController::delete on module uninstall and add tests
Status: Active » Needs review
FileSize
3.54 KB
1.22 KB

You're correct the hooks are not fired!

Status: Needs review » Needs work

The last submitted patch, config.uninstall.18.patch, failed testing.

alexpott’s picture

@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.

alexpott’s picture

Issue summary: View changes

Update

catch’s picture

I'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?

tim.plunkett’s picture

effulgentsia’s picture

Status: Needs work » Closed (duplicate)