Blocks: #111715: Convert node/content types into configuration

Problem

  • Data loss due to config_install_default_config() overwriting preexisting configuration.

Details

  • Scope and scenario:
    1. Module A provides configuration for module B.
    2. The user customizes the configuration.
    3. Module A gets uninstalled, leaving its configuration for module B intact (or in a disabled state).
    4. Module A gets re-installed.
    5. The existing configuration is overwritten with module A's default configuration.
  • Example:
    1. Poll module provides node.type.poll for Node module.
    2. node.type.poll is customized.
    3. Poll module gets uninstalled.
    4. Poll module gets re-installed.
    5. node.type.poll is overwritten with Poll module's default node.type.poll configuration.

Proposed solution

  1. Change config_install_default_config() to not overwrite existing configuration.

Related issues

Files: 
CommentFileSizeAuthor
#40 1889620-install-overwrites-config-40.patch7.79 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 51,344 pass(es). View
#39 1889620-install-overwrites-config-39.patch11.25 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,991 pass(es). View
#39 1889620-diff-35-39.txt6.27 KBvijaycs85
#35 1889620-install-overwrites-config-35.patch17.05 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,762 pass(es). View
#35 1889620-diff-34-35.txt925 bytesvijaycs85
#34 1889620-install-overwrites-config-34.patch17.96 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,577 pass(es). View
#34 1889620-diff-32-34.txt15.34 KBvijaycs85
#32 1889620-install-overwrites-config-32.patch12.98 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,443 pass(es). View
#32 1889620-diff-30-32.txt1.25 KBvijaycs85
#30 1889620-install-overwrites-config-30.patch12.36 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,537 pass(es). View
#25 1889620-install-overwrites-config-25.patch12.41 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 50,673 pass(es). View
#25 1889620-diff-18-25.txt3.83 KBvijaycs85
#18 1889620-install-overwrites-config-18.patch12.17 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 49,445 pass(es). View
#18 1889620-diff-16-18.txt1.71 KBvijaycs85
#16 1889620-install-overwrites-config-16.patch10.46 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] 49,094 pass(es), 6 fail(s), and 0 exception(s). View
#9 config.reinstall.9.patch12.92 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,695 pass(es). View
#9 interdiff.txt5.13 KBsun
#2 config.reinstall.2.patch7.79 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,717 pass(es), 1 fail(s), and 0 exception(s). View

Comments

tim.plunkett’s picture

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)

sun’s picture

Status: Active » Needs review
FileSize
7.79 KB
FAILED: [[SimpleTest]]: [MySQL] 50,717 pass(es), 1 fail(s), and 0 exception(s). View

TDD: Here's full test coverage.

heyrocker’s picture

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.

Status: Needs review » Needs work

The last submitted patch, config.reinstall.2.patch, failed testing.

sun’s picture

@heyrocker:
The problem is: Existing persistence tests for node types in HEAD disagree. (See the last test assertion. That's why this is blocking #111715: Convert node/content types into configuration)

Essentially, the node type + node data should be retained as-is, if the module that created the node type is disabled or uninstalled.

sun’s picture

Node types (+ nodes) are actually a rather poor example.

Text formats are a much better example. Whatever you do, text formats never cease to exist. (Text formats cannot be deleted.)

heyrocker’s picture

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.

catch’s picture

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.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
5.13 KB
12.92 KB
PASSED: [[SimpleTest]]: [MySQL] 50,695 pass(es). View

For now, I'd prefer to move forward with the existing expectations - especially due to the case of text formats.

This was supposed to be a one line fix, but the new manifest file handling (which still needs follow-up work) made it a bit more complex.

For now, I helped myself out with a new parameter for config_sync_get_changes() that allows to force-ignore manifest files.

xjm’s picture

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.

tim.plunkett’s picture

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.

sun’s picture

#1887654: Creating a config entity with an existing machine name shouldn't work does not conflict with this issue - they complement each other.

The specific case of #22 over there (PHP text format being re-installed + overwritten upon re-installation of PHP module) is exactly the case that this patch here fixes, and which shouldn't be touched over there.

I think #9 is ready, and comes with full test coverage (with expected failure in #2).

sun’s picture

Can we move forward with the patch in #9?

That would allow me to continue work on #111715: Convert node/content types into configuration

sun’s picture

Feedback, anyone? Or RTBC?

tim.plunkett’s picture

Status: Needs review » Needs work

#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

heyrocker’s picture

Status: Needs work » Needs review
FileSize
10.46 KB
FAILED: [[SimpleTest]]: [MySQL] 49,094 pass(es), 6 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 1889620-install-overwrites-config-16.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
12.17 KB
PASSED: [[SimpleTest]]: [MySQL] 49,445 pass(es). View

Adding test module from #9.

Status: Needs review » Needs work
Issue tags: -Configuration system, -Configurables

The last submitted patch, 1889620-install-overwrites-config-18.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Configurables
sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

This is a critical issue. Given that others have re-rolled this patch without raising major concerns against the actual change or disagreeing with them, I think it is safe to say that this is RTBC.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

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

alexpott’s picture

Status: Needs review » Needs work
+++ 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.

alexpott’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
12.41 KB
PASSED: [[SimpleTest]]: [MySQL] 50,673 pass(es). View

Updating patch with review comments in #23.

alexpott’s picture

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

sun’s picture

Let's remove the DUTB changes here and move the new test method into a new ConfigInstallWebTest class.

We can optimize it and potentially convert it back to a DUTB test later on, when time permits.

alexpott’s picture

#27 sounds like a great idea.

vijaycs85’s picture

FileSize
1.93 KB
12.41 KB
PASSED: [[SimpleTest]]: [MySQL] 50,435 pass(es). View

Just moving uninstallModules to ConfigInstallTest class.

vijaycs85’s picture

FileSize
12.36 KB
PASSED: [[SimpleTest]]: [MySQL] 50,537 pass(es). View

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

alexpott’s picture

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.

vijaycs85’s picture

FileSize
1.25 KB
12.98 KB
PASSED: [[SimpleTest]]: [MySQL] 50,443 pass(es). View

Updating test case as specified in #31

sun’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallTest.php
@@ -84,4 +84,120 @@ function testModuleInstallation() {
+  function testIntegrationModuleReinstallation() {
...
+  protected function disableModules(array $modules) {
...
+  protected function uninstallModules(array $modules) {

Let's move this test method into a new ConfigInstallWebTest that extends WebTestBase (instead of DUTB).

That inherently means that we can remove the test helper methods and use the regular module_enable(), module_disable(), and module_uninstall() instead.

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

All changes to this file should be reverted.

vijaycs85’s picture

FileSize
15.34 KB
17.96 KB
PASSED: [[SimpleTest]]: [MySQL] 50,577 pass(es). View

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

vijaycs85’s picture

FileSize
925 bytes
17.05 KB
PASSED: [[SimpleTest]]: [MySQL] 50,762 pass(es). View

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

alexpott’s picture

Status: Needs review » Postponed

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

sun’s picture

Status: Postponed » Needs work
-class ConfigInstallTest extends DrupalUnitTestBase {
...
-  function testModuleInstallation() {

+class ConfigInstallWebTest extends WebTestBase {
...
+  function testModuleInstallation() {

Let's keep the existing test method and ConfigInstallTest as-is, so that the new (and slow) web/integration test only contains tests that actually need the full environment.

sun’s picture

Assigned: sun » Unassigned

Also, you've clearly taken this over by now :)

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.27 KB
11.25 KB
PASSED: [[SimpleTest]]: [MySQL] 50,991 pass(es). View

Updating changes for #37.

vijaycs85’s picture

FileSize
7.79 KB
PASSED: [[SimpleTest]]: [MySQL] 51,344 pass(es). View

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.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

This looks great!

sun’s picture

Thanks! This also looks great to me now.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.