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.
Files: 
CommentFileSizeAuthor
#18 17-18-interdiff.txt1.22 KBalexpott
#18 config.uninstall.18.patch3.54 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,449 pass(es), 17 fail(s), and 1 exception(s).
[ View ]
#17 config.uninstall.17.patch2.25 KBblackra
FAILED: [[SimpleTest]]: [MySQL] 59,593 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#2 config.uninstall.2.patch1.67 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,096 pass(es).
[ View ]
drupal8.config-uninstall.0.patch1.69 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.config-uninstall.0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Assigned:sun» Unassigned

Priority:Major» Normal
Status:Active» Needs review
StatusFileSize
new1.67 KB
PASSED: [[SimpleTest]]: [MySQL] 41,096 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Looks straight forward.

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

No tests?

Looks like test would belong in UninstallTest class.

Issue tags:+VDC

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.

Status:Needs work» Active

In any case, this patch is moot.

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.

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.

Status:Postponed» Active

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.

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

Updating title

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

Updating title

StatusFileSize
new2.25 KB
FAILED: [[SimpleTest]]: [MySQL] 59,593 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Title:Add tests that the config entity API is called on module uninstall when config entities are removedInvoke ConfigStorageController::delete on module uninstall and add tests
Status:Active» Needs review
StatusFileSize
new3.54 KB
FAILED: [[SimpleTest]]: [MySQL] 59,449 pass(es), 17 fail(s), and 1 exception(s).
[ View ]
new1.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.

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

Issue summary:View changes

Update

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?