config_install_default_config() overwrites existing configuration

With #1887654: Creating a config entity with an existing machine name shouldn't work, ConfigEntities would throw an exception. In what other case would there be conflicting config? (Not saying there isn't one, I just can't think of it)

It actually seems perfectly natural to me that this get overwritten. Uninstalling a module is *supposed* to destroy all the data that it provides and this is consistent with D7 behavior (I'm pretty sure.)

I know that over in #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API we have been hashing out this issue, but this is the one part of it that I'm still not a fan of because it feels really unintuitive and I think its the one part that remains unresolved (both xjm and I have spoken out about this.) So I'd like to postpone this until that is sorted out.

Huh I had no idea that was expected behavior. You're right that Text Formats are a better example too. Given that, we should get this done, and if we want to have a greater discussion about the expectations behind config and uninstalled modules, it should continue in the other issue.

The node type persistence test is due to the comment body deletion upgrade path bug that was live for 6 months after Drupal 7 release. That's only a workaround for disabled modules making no sense and I think we can change the assumption if we want as long as it's consistent.

So it comes down to:

1. When you uninstall a module, should configuration and content belong to that module be deleted (ideally it should but it ought to warn about deleting content and/or force you to delete it manually first).

2. Should there be a way to decouple configuration + content from a module if you want to - i.e. a module installs a node type and you want to keep the node type (because it has content in it) but get rid of the module otherwise.

I added one test for this scenario in #1887654: Creating a config entity with an existing machine name shouldn't work; I assumed it had the same cause.

The blocker I hit specifically in #1887654-22: Creating a config entity with an existing machine name shouldn't work sounds like it makes this issue a duplicate.

#1887654: Creating a config entity with an existing machine name shouldn't work does indeed cover this issue, but it is not and will not be ready very soon, and this is a well scoped issue.
I'm fine with this going in first.

+++ b/core/includes/config.incundefined
@@ -29,27 +29,27 @@ function config_install_default_config($type, $name) {
-      $manifest_data = array();
       foreach ($source_storage->listAll($entity_info['config_prefix']) as $config_name) {
         list(, , $id) = explode('.', $config_name);
-        $manifest_data[$id]['name'] = $config_name;
+        $manifest_config->set("$id.name", $config_name);
       }
-      $manifest_config->setData($manifest_data)->save();
+      $manifest_config->save();

What is the reason for this change? It's not clear to me.

+++ b/core/includes/config.incundefined
@@ -112,44 +112,51 @@ function config($name) {
-  $source_config_data = array();
-  $target_config_data = array();
-  foreach ($source_storage->listAll('manifest') as $name) {
-    if ($source_manifest_data = $source_storage->read($name)) {
-      $source_config_data = array_merge($source_config_data, $source_manifest_data);
+  if ($require_manifest) {
+    $source_config_data = array();
+    $target_config_data = array();
+    foreach ($source_storage->listAll('manifest') as $name) {
+      if ($source_manifest_data = $source_storage->read($name)) {
+        $source_config_data = array_merge($source_config_data, $source_manifest_data);
+      }
+      if ($target_manifest_data = $target_storage->read($name)) {
+        $target_config_data = array_merge($target_config_data, $target_manifest_data);
+      }
     }
-
-    if ($target_manifest_data = $target_storage->read($name)) {
-      $target_config_data = array_merge($target_config_data, $target_manifest_data);
+    foreach (array_diff_key($target_config_data, $source_config_data) as $name => $value) {
+      $config_changes['delete'][] = $value['name'];
+    }
+    foreach (array_diff_key($source_config_data, $target_config_data) as $name => $value) {
+      $config_changes['create'][] = $value['name'];
     }
   }

Note to reviewers, this is just indented under if ($require_manifest) { now

+++ b/core/includes/config.incundefined
@@ -112,44 +112,51 @@ function config($name) {
+  else {
+    $config_changes['delete'] = array_diff($all_target_names, $all_source_names);
+    $config_changes['create'] = array_diff($all_source_names, $all_target_names);

This else should have a comment

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.phpundefined
@@ -211,11 +211,9 @@ protected function installSchema($module, $table) {
   protected function enableModules(array $modules, $install = TRUE) {
-    // Set the modules in the fixed module_list().
-    $new_enabled = array();
     foreach ($modules as $module) {
+      // Set the module in the fixed module_list().
       $this->moduleList[$module]['filename'] = drupal_get_filename('module', $module);
-      $new_enabled[$module] = dirname($this->moduleList[$module]['filename']);
       module_list(NULL, $this->moduleList);

@@ -230,4 +228,35 @@ protected function enableModules(array $modules, $install = TRUE) {
+  /**
+   * Disables modules in this test.
+   *
+   * module_disable() disables the modules in the system.module configuration
+   * only, but that has no effect, since we are operating with a fixed module
+   * list.
+   *
+   * @param array $modules
+   *   A list of modules to disable. Dependencies are not resolved; i.e.,
+   *   multiple modules have to be specified with dependent modules first.
+   * @param bool $uninstall
+   *   (optional) Whether to uninstall the modules via module_uninstall().
+   *   Defaults to FALSE. If TRUE, the modules are only disabled.
+   *
+   * @todo Remove this method as soon as there is an Extensions service
+   *   implementation that is able to manage a fixed module list.
+   */
+  protected function disableModules(array $modules, $uninstall = FALSE) {

These hunks were rejected, will need a reroll

This is a reroll. Some larger changes happened to config.inc and DrupalUnitTestBase since this patch was posted but I think this is right. No interdiff as I merged HEAD too.

What is the reason for this change? It's not clear to me.

This change is now irrelevant as all that code was removed as part of #1889752: Remove unnecessary manifest creation in config_install_default_config().

This else should have a comment

I'd actually like more comments in general around the $require_manifest parameter, in particular what situations there are where you might want to set this as FALSE.

Adding test module from #9.

#18: 1889620-install-overwrites-config-18.patch queued for re-testing.

@heyrocker asks for more feedback in #16, and RTBCing your own patch--. I'll look at this with my coffee. :)

+++ b/core/includes/config.incundefined
@@ -106,44 +106,51 @@ function config($name) {
+function config_sync_get_changes(StorageInterface $source_storage, StorageInterface $target_storage, $require_manifest = TRUE) {

I think the variable here should be $use_manifest as this is more explicit... we are not requiring a manifest we are using them. I don't think this is bikeshed-y since it is about meaning...

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.phpundefined
@@ -223,4 +223,35 @@ protected function enableModules(array $modules, $install = TRUE) {
+  protected function disableModules(array $modules, $uninstall = FALSE) {
+    foreach ($modules as $module) {
+      // Call module_disable() to disable the module.
+      module_disable(array($module), FALSE);
+
+      // Unset the module in the fixed module_list().
+      unset($this->moduleList[$module]);
+      module_list(NULL, $this->moduleList);
+    }
+    if ($uninstall) {
+      module_uninstall($modules, FALSE);
+    }
+  }

Hmmm... not sure about this function in light of #1891516: Remove $install parameter from DrupalUnitTestBase::enableModules(), encourage individual schema tables where we are removing the $install paramater from the equivalent enableModules() function. I think disableModules() should just manage the module list and leave disable / uninstall up to the test itself.

Tricky.

Especially calling it with $uninstall = TRUE as once #1891516: Remove $install parameter from DrupalUnitTestBase::enableModules(), encourage individual schema tables it will be the test's responsibility to create all the database tables declared by a module's schema hook.

One possible use-case for setting $require_manifest/$use_manifest to FALSE would be #1515312: Add snapshots of last loaded config for tracking whether config has changed where we need to be able to compare the snapshot and active storages.

Updating patch with review comments in #23.

hmm.... the changes in #25 for DrupalUnitTestBase don't really address the issues raised in #23. @sun what do you think we should do?

#27 sounds like a great idea.

Just moving uninstallModules to ConfigInstallTest class.

Moving disableModules changes from DUTB to new class and updating docblock for $use_manifest change.

I think that this is very nearly ready to go... I think we need to add to the test.

Before we uninstall the integration module we should disable and then enable it again and then test that both the simple config and config entity have the non default values.

Updating test case as specified in #31

As per #33:
1. Renamed ConfigInstallTest to ConfigInstallWebTest and extends WebTestBase.
2. Removed helpers for install/uninstall/disable.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.phpundefined
@@ -137,7 +137,7 @@ public function containerBuild($container) {
-      // together here, it still might a keyvalue storage for anything (for 

3. I this, it is valid fix (trailing space) in DrupalUnitTestBase.php

Reverting all changes from DUTB. Space got its own issue #1919134: Remove space from comment in DrupalUnitTestBase.php

This patch is totally ready to fly but it contains exactly the same change to config_sync_get_changes as #1515312: Add snapshots of last loaded config for tracking whether config has changed and that issue is rtbc so I think should postpone on that issue and then reroll...

Updating changes for #37.

As advised by @alexpott on IRC, Removing changes from config_sync_get_changes() as #1515312: Add snapshots of last loaded config for tracking whether config has changed made it to core.

This looks great!

Had to think about this a few times but I can't really find any objections to the patch, so committed/pushed to 8.x. In the back of my head there's a nagging feeling this will be brought up again when someone expects it to do the opposite but I guess it's a case of deciding in this case.