Problem

  • Configuration of uninstalled modules may get orphan and persist forever, if a module does not manually delete all of its configuration.

Goal

  • Automate removal of configuration upon uninstalling a module, as much as possible.

Details

  • Many Drupal sites contain plenty of orphan configuration variables belonging to modules that no longer exist.
  • We should have a way to automatically uninstall configuration when a module is uninstalled without having to rely on the module's author implementing hook_uninstall().

Proposed resolution

  1. When a module is uninstalled, get a list of the configuration files it supplies by default.
  2. Scan through the live configuration directory and see if any of these files exist.
  3. If so, delete them.

Notes

  • This would not catch any orphan live configuration objects, in case a module changes its default configuration objects over time. (assuming the module does not implement a module update that renames or delete the old configuration objects)
  • This would also not clean up configuration created through other methods that depend on the module. For example:
    1. module Foo provides views.view.foo as a default view.
    2. views.view.foo gets moved to the live config directory when the module is installed.
    3. Now someone goes into the UI and creates a view called bar (views.view.bar) that depends on module Foo.
    4. When module Foo is uninstalled, the default view it provides would get cleaned up, but 'bar' view created through the UI would not.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Configuration system
moshe weitzman’s picture

IMO each config file should be owned by a module in some fashion. It should not be supported to add configuration that is not owned by an active module. This is consistent with advances in recent years where core automatically cleans up menu tables, permission tables, etc when modules are uninstalled. We have not had good luck with forcing this burden on hook_uninstall() module authors. Thats an option when automation is not possible.

gdd’s picture

I believe the proposal above will do that. Any config provided by a module will be cleaned up when the module is uninstalled. There will still be situations where config exists that is not provided by a module (for instance a View created through the UI) however in those cases the config should belong to (and be deleted by) the module that created it (in this case Views.)

sun’s picture

acrollet’s picture

Assigned: Unassigned » acrollet

working on this at the Drupalcon Denver code sprint.

acrollet’s picture

Status: Active » Needs review
FileSize
1.36 KB

Patch attached. It follows the exact method in 'Proposed Resolution' in the issue summary. As a result, it will *not* currently remove custom configurations not provided by a module. (e.g. a custom image style)

moshe weitzman’s picture

Is an image style created with the web GUI a 'custom image style'? If so, then I consider this patch incomplete.

acrollet’s picture

moshe: the answer to your question is yes.

To handle the use case of deleting non-default configurations, we need to do one of:
a) rely on a de-facto namespace based on the module name (e.g. Delete config files named image.*)
b) create a method for modules to register/store the names of non-default configurations, which heyrocker said he didn't want to do earlier today
c) insert solution here

gdd’s picture

Status: Needs review » Needs work

This patch may be incomplete, but it is an improvement over what we have now, which is nothing. Modules can still create a hook_uninstall() to clean up config on their own. I don't see any reason we shouldn't commit it and work out the next issue in a followup.

+++ b/core/includes/install.incundefined
@@ -393,6 +394,22 @@ function drupal_uninstall_modules($module_list = array(), $uninstall_dependents
+    $module_config_dir = drupal_get_path('module', $module) . '/config';
+    if (is_dir(drupal_get_path('module', $module) . '/config')) {

The drupal_get_path() code is repeated here. You can actually just remove the $module_config_dir line.

+++ b/core/includes/install.incundefined
@@ -393,6 +394,22 @@ function drupal_uninstall_modules($module_list = array(), $uninstall_dependents
+      foreach ($files as $key => $file) {

This is really a nitpick, but since you're not using $key at all, I'd rather see this just be foreach ($files as $file)

3 days to next Drupal core point release.

acrollet’s picture

Status: Needs work » Needs review
FileSize
1.28 KB

updated patch attached.

Status: Needs review » Needs work

The last submitted patch, drupal-clean_config_files_on_uninstallation-1444766-10.patch, failed testing.

gdd’s picture

Ah you're using $module_config_file in the loop. I guess you should add the variable back and use it in the is_dir().

It also occurs to me that we should add tests for this, I guess EnableDisableTestCase in system.test is the best place. It should verify that a) a module's default config did in fact get copied to the config directory on installation and b) that it is gone when the module gets uninstalled.

acrollet’s picture

Today turned into a busy day, but I'm hoping to get a patch with tests posted by tomorrow evening or so, leaving self-assigned.

acrollet’s picture

Status: Needs work » Needs review
FileSize
4.88 KB

patch attached re-using $module_config_dir, with tests.

gdd’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me, and it will be really useful. We can extend it in followups. Thanks for working on it.

acrollet’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work
+  /**
+   * Assert that none of a module's default config files are loaded.
+   *
+   * @param $module
+   *   The name of the module.
+   */
+  function assertModuleConfigFilesDoNotExist($module) {

These two need @return docs, apart from that this looks fine - quick re-roll?

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1013 bytes
5.08 KB

Here we go.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed/pushed to 8.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.