Configuration file name collisions silently ignored for default configuration

So, scenario 1 in D7 is hilarious. You end up with one content type called forum that is a hybrid of both content types. So I don't think anyone is relying on a particular behavior there. ;)

2: 2140511.2.patch queued for re-testing.

Rerolled to apply again.

This is the output after copying node.type.page.yml into actions/config and trying to enable actions module.

Closed #2205845: Configuration file name collisions silently ignored as duplicate. Applying same priority and beta target tag.

Here is a proof of concept of a more strict approach. This checks before installing a module whether the default config will overlap with anything in active config. If so, the module is not installed and all the module installation is stopped (because further modules may have requirement to install the prior one).

The error message certainly needs work, and should not be a warning because it does stop the installation from happening. Here is an updated patch with that and some more minor fixes. Here is how it looks like:

#1986090: Profile config does not overwrite module default config on install (system.cron.yml) is related although the problem there is the overwrite is *desired*.

modules cannot rely on configuration entities. config entities can be arbitrarily deleted/renamed/updated so that the name is the same, but everything else about them is different.

This isn't strictly true either from an API standpoint or the status quo in core. Forum module for example creates locked configuration entities:

id: taxonomy_term.forum_container
status: true
langcode: en
name: forum_container
type: list_boolean
settings:
  allowed_values:
    - ''
    - ''
  allowed_values_function: ''
module: options
entity_type: taxonomy_term
locked: true
cardinality: 1
translatable: false
indexes: {  }

@beejeebus: the problem lies within dependencies in configuration, you don't even need to have any line of PHP code. Let's say your module ships with field_product which is an image field. It is included in one or more displays of your products_view, that you also ship with the module. Now in Drupal 8, let's say you already have a field_product which is an SKU text field. Your module will be installed, the new (image field) field_product will not be installed because it conflicts, but that is silently ignored. The view that has the image field specific configuration for the field that is now a text field will be. Now what?

@beejeebus I don't think that would solve it, the dependencies are by name, so if my view depends on field_product, that does not really help, since a different field_product may be there. If the dependency is by UUID then we can ensure the concrete config, but then the UUID is not a UUID because it would be the same on each site :D

Well, one option is config names are namespaced. Eg. instead of field_product, it would be field_mymodule_product and field_othermodule_product. That would require for core to follow the best practice and ship with default fields like field_node_body, view.node_frontpage, etc.

That said we would still need to check this prior to module install and the only reason this will fail is that an import a configuration entity will not occur because one with the same name already exists.

As a naive user, this part raises alarm bells. The error prevents catastrophic data loss, but it also prevents me from building a site. I don't know, even now, what the remediation would be. Should I rename configuration files? Is this still an open issue and I just haven't found that discussion yet?

I'm trying to understand the problem better. So, here is a scenario:

1. I create a View that exposes as a block the image field in the default Article node type.
2. I export that View.
3. I import that View into another site.
4. Will that import fail because a field named image already exists? Will it fail before an Article node type already exists?

@jeesebeach: I have a complete example with contrib modules in #25, please see that :) Happy to elaborate.

If a field already existed with the machine name forum_container before the module was installed this would be very messy and the installation would create all the other default configuration and use the existing forum_container field even though nothing what-so-ever mandates that is is the same type of field as defined in the forum module's default configuration.

I also reread #25 and realized I'm using the wrong words. I shouldn't have written import; rather, I meant install.

So, given the clarifications in #25 and #35, here's the scenario as I understand it. There is really only one use case we're trying to defend against.

The core Forum module is not installed. A contrib module is installed and creates a Config Entity called forum_container. The user then enables the core Forum module which fails in some way (doesn't install or uses the wrong forum_container Config Entity).

Would it be possible to prevent installation of a module that would create a Config Entity, if a Config Entity with the same name existed in any other module installed or not? Like even if just the code were in the site, but the other module is not yet enabled? Here's what would happen in this case.

Module developer Zed is creating AwesomeForums module. The Forum module is not enabled on their development site. This developer creates a Config Entity called forum_container in their new module. The developer attempts to enable the module and whamo, it won't enable. The error message on the install screen explains that their Config Entity forum_container has the same name as a Config Entity of the Forum module and could lead to conflict -- please namespace the Config Entity name.

This would prevent module developers from creating Config Entities that conflict with core Config Entities. Now what about contrib-to-contrib Config Entity name conflicts? The same restriction applies. Except in contrib, developers will just need to namespace their Config Entities with the module namespace to prevent collisions.

And we don't do anything more complex that this. In the case where a Config Entity being installed has dependencies to what it expects to be existing Config Entities (field_image or field_product for example), we can simply list any missing dependencies on an install confirmation screen (field_image was deleted!), state that the missing fields may cause behavior problems with the new module, and ask the site builder if they'd like to proceed.

The "configuration depends on other configuration that may be deleted already" case is handled by #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall. This issue is about depending on a configuration that *exists* but may or may not be as expected. When you ship with configuration but part of your shipped configuration is not installed so your other config may or may not use dependents that it expected to use.

When you ship with configuration but part of your shipped configuration is not installed...

Right, and there are three ways this could happen.

1. A module developer uses a name for a Config Entity that a Core module would also use.
2. A site builder creates a field with a name that a Core module would also use.
3. A module developer uses a name for a Config Entity that a contrib module also uses.

If we can prevent (1) from happening by shutting down module enabling in a module under development and (2) by disallowing a field to be given a name that a Core module might also use, then we avoid the downstream conflicts for site builders who would enable a Core module like Forum and run into conflicts.

And these restrictions, although not preventing (3), would raise conflicts quickly and promote proper namespacing in contrib.

Sorry to harp on this, but I'm trying to find a solution that would simply prevent the conflict from arising in the first place by making it really difficult to post a contrib module to d.o. that defines Config Entities with names that Core might potentially use. We want the decision point to exist primarily at development time so that developers are forced to confront these issues, not at site building time, when a site builder, who may not have a deep knowledge of Drupal, would be confronted with some tough decisions and little in terms of information to make them correctly.

So, all that being said, I think the approach in this patch is fine for what it seeks to prevent. What I'm arguing is that we should strive to never let a site builder get to the point where this install error is raised.

With the patch in #20, the Standard profile can no longer be installed :)

Lol, that looks like a testament to how successful this is :D

I guess that means that this is blocked on #1986090: Profile config does not overwrite module default config on install (system.cron.yml)?

The following code can be removed from ConfigInstallWebTest::testInstallProfileConfigOverwrite(), which is added by #1986090: Profile config does not overwrite module default config on install (system.cron.yml).

// Turn on the test module, which will attempt to replace the
// configuration data. This attempt to replace the active configuration
// should be ignored.
\Drupal::moduleHandler()->install(array('config_override_test'));

// Verify that the test module has not been able to change the data.
$config = \Drupal::config($config_name);
$this->assertIdentical($config->get(), $expected_profile_data);

// Disable and uninstall the test module.
\Drupal::moduleHandler()->uninstall(array('config_override_test'));

// Verify that the data hasn't been altered by removing the test module.
$config = \Drupal::config($config_name);
$this->assertIdentical($config->get(), $expected_profile_data);

Instead, installing a module or theme that attempts to install a default configuration file that already exists in active configuration should fail with an error message.

So, when the profile is installed after the basic modules are installed, we run into fatal errors because the profile tries to apply its default configuration files. I think what should happen, given #1986090: Profile config does not overwrite module default config on install (system.cron.yml) will already have taken the Profiles default configuration into account, is that a profile should not attempt to install any default configuration; it should only be installed in the context of a module installing.

Nice! I jus wrote the same-ish block of code in ConfigInstaller to deal with filtering out already installed configs.

// If this is a profile module, remove any configuration that has already
// been transferred to active configuration because a module that shares
// the default configuration was already installed.
if ($type === 'profile') {
  $config_already_installed = array();
  foreach ($config_to_install as $name) {
    if ($this->activeStorage->exists($name)) {
      $config_already_installed[] = $name;
    }
  }
  $config_to_install = array_diff($config_to_install, $config_already_installed);
}

Let's see where the bot ends up #46.

The patch in #46 applied on #1986090: Profile config does not overwrite module default config on install (system.cron.yml) now gets us through site installation with the default configs from the standard profile without any errors.

What does "update verbiage" mean in the summary? What verbiage where?

I believe that was in reference to the error message.

#1986090: Profile config does not overwrite module default config on install (system.cron.yml) is committed, but the patch from #46 didn't apply because of significant other changes since then. I tried to re-roll.

The resulting patch applies OK, but now I get an odd sort of WSOD with nothing in the logs, anywhere. It happens on line 1299 of core/modules/system/system.module , during system_rebuild_module_data. The offending line is

\Drupal::state()->set('system.module.files', $files);

But I don't know why! This causes a problem right at the beginning of moduleHandler->install(). I'll work more on this in the coming days, but any advice would be great.

I've attached the patch such as it is for now.

Let's use testbot for some more insight.

I tracked this down a bit further, but I still don't know why it's stopping like this. Since there's no error behavior from PHP, it might not even be an error. The values received by \Drupal::state()->set() seem just fine. It tries to set

key: system.module.files
value:

Array
(
    [action] => core/modules/action/action.info.yml
    [aggregator] => core/modules/aggregator/aggregator.info.yml
    [ban] => core/modules/ban/ban.info.yml
    [basic_auth] => core/modules/basic_auth/basic_auth.info.yml
    [block] => core/modules/block/block.info.yml
    [book] => core/modules/book/book.info.yml
    [breakpoint] => core/modules/breakpoint/breakpoint.info.yml
    [ckeditor] => core/modules/ckeditor/ckeditor.info.yml
    [color] => core/modules/color/color.info.yml
    [comment] => core/modules/comment/comment.info.yml
    [config] => core/modules/config/config.info.yml
    [config_translation] => core/modules/config_translation/config_translation.info.yml
    [contact] => core/modules/contact/contact.info.yml
    [content_translation] => core/modules/content_translation/content_translation.info.yml
    [contextual] => core/modules/contextual/contextual.info.yml
    [custom_block] => core/modules/block/custom_block/custom_block.info.yml
    [datetime] => core/modules/datetime/datetime.info.yml
    [dblog] => core/modules/dblog/dblog.info.yml
    [editor] => core/modules/editor/editor.info.yml
    [entity] => core/modules/entity/entity.info.yml
    [entity_reference] => core/modules/entity_reference/entity_reference.info.yml
    [field] => core/modules/field/field.info.yml
    [field_ui] => core/modules/field_ui/field_ui.info.yml
    [file] => core/modules/file/file.info.yml
    [filter] => core/modules/filter/filter.info.yml
    [forum] => core/modules/forum/forum.info.yml
    [hal] => core/modules/hal/hal.info.yml
    [help] => core/modules/help/help.info.yml
    [history] => core/modules/history/history.info.yml
    [image] => core/modules/image/image.info.yml
    [language] => core/modules/language/language.info.yml
    [link] => core/modules/link/link.info.yml
    [locale] => core/modules/locale/locale.info.yml
    [menu_link] => core/modules/menu_link/menu_link.info.yml
    [menu_ui] => core/modules/menu_ui/menu_ui.info.yml
    [migrate] => core/modules/migrate/migrate.info.yml
    [migrate_drupal] => core/modules/migrate_drupal/migrate_drupal.info.yml
    [minimal] => core/profiles/minimal/minimal.info.yml
    [node] => core/modules/node/node.info.yml
    [options] => core/modules/options/options.info.yml
    [path] => core/modules/path/path.info.yml
    [quickedit] => core/modules/quickedit/quickedit.info.yml
    [rdf] => core/modules/rdf/rdf.info.yml
    [responsive_image] => core/modules/responsive_image/responsive_image.info.yml
    [rest] => core/modules/rest/rest.info.yml
    [search] => core/modules/search/search.info.yml
    [serialization] => core/modules/serialization/serialization.info.yml
    [shortcut] => core/modules/shortcut/shortcut.info.yml
    [simpletest] => core/modules/simpletest/simpletest.info.yml
    [statistics] => core/modules/statistics/statistics.info.yml
    [syslog] => core/modules/syslog/syslog.info.yml
    [system] => core/modules/system/system.info.yml
    [taxonomy] => core/modules/taxonomy/taxonomy.info.yml
    [telephone] => core/modules/telephone/telephone.info.yml
    [text] => core/modules/text/text.info.yml
    [toolbar] => core/modules/toolbar/toolbar.info.yml
    [tour] => core/modules/tour/tour.info.yml
    [tracker] => core/modules/tracker/tracker.info.yml
    [update] => core/modules/update/update.info.yml
    [user] => core/modules/user/user.info.yml
    [views] => core/modules/views/views.info.yml
    [views_ui] => core/modules/views_ui/views_ui.info.yml
    [xmlrpc] => core/modules/xmlrpc/xmlrpc.info.yml
)

This looks totally normal and valid as far as I can tell. By this point in the Install process, we've already used this same function several times without issue. But when I call it inside ModuleHandler->install(), it dies on keyValueStore->set($key, $value); .

I don't understand why this should be a problem for this particular value in this particular place. In HEAD we call the system_rebuild_module_data() 1 line later, inside if ($enable_dependencies), which works fine for the installer because during install $enable_dependencies == FALSE. It seems like the keyValueStore, and therefore system_rebuild_module_data(), isn't useful this early in the install process.

We are using system_rebuild_module_data() to populate $module_data to skip already installed modules, and, specific to this patch, to run this key snippet:

        // Validate default configuration of this module. Bail if unable to
        // install. Should not continue installing more modules because those
        // may depend on this one.
        if (!\Drupal::service('config.installer')->validatePreInstall($module_data[$module]->getType(), $module)) {
          break;
        }

which is part of the point of this whole patch.

TL;DR: I'm not sure what to do here.

• We don't want to run only when $enable_dependencies == TRUE. If the default configuration fails validation, we should break out of the install process, just like we would with the module enabling process.
• Is there an alternative way to populate $module_data that might work this early in the install process?

I've attached an updated version of the patch which gets through the install process with the default profile. All I did is move $module_data = system_rebuild_module_data() and the validatePreInstall() into the $enable_dependencies == TRUE if statement, so they don't fire during Install. I don't think that's Right, but it works.

#2090115: Don't install a module when its default configuration has unmet dependencies is postponed on this issue since the validation step would be useful there.

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstallerInterface.php
   @@ -84,4 +84,18 @@ public function setSyncing($status);
   +   * Validate default configuration before installation.
   

   Certainly is not doing any validation.

2. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
   @@ -0,0 +1,68 @@
   + * An exception thrown in case of config already already existing errors.
   

   Sentence needs cleanup, eg. repetition.

3. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
   @@ -0,0 +1,68 @@
   +   * The configuration names that already exists, keyed by collection.
   +   *
   +   * @var string
   +   */
   +  protected $configNames = array();
   

   Not a string.

4. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
   @@ -0,0 +1,68 @@
   +   * Sets the extensions being installed.
   +   *
   +   * @param array $extensions
   +   *   The extensions being installed.
   ...
   +   * Gets the extensions being installed.
   +   *
   +   * @return array
   +   *   The extensions being installed.
   

   Would be good to document the structure of the array.

5. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
   @@ -0,0 +1,68 @@
   +   * Sets the configuration names.
   +   *
   +   * @param array $config_names
   +   *   The configuration names keyed by collection.
   

   Is that a nested associative array?

6. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
   @@ -0,0 +1,68 @@
   +   * Gets the configuration names keyed by collection.
   +   *
   +   * @return string
   +   *   The configuration names.
   

   Not a string.

7. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -676,6 +677,20 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +      if (count($existing_configuration)) {
   +        $exception = new PreExistingConfigException();
   +        $exception->setConfigNames($existing_configuration);
   +        $exception->setExtensions($module_list);
   +        throw $exception;
   +      }
   

   How does this show up on the UI, what is the user feedback now?

8. +++ b/core/modules/config/tests/config_install_fail/config_install_fail.module
   @@ -0,0 +1,6 @@
   + * Empty module to test providing a default config entity with the same name.
   

   With the same name as? :)

Re-roll.

Per discussion with @alexpott and @larowlan, postponing on #2224581: Delete forum data on uninstall.

Actually postponing. :)

Initial review.

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -156,16 +157,17 @@ protected function listDefaultConfigCollection($collection, $type, $name, array
   +        // Ensure that we have not already discovered the config to install.
   

   Nit: Wouldn't something like "Ensure that the config to install isn't already installed." make more sense?
   After reading this hunk a few times, it made sense to me though.

2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -156,16 +157,17 @@ protected function listDefaultConfigCollection($collection, $type, $name, array
   +        // Ensure the configuration is provided by an enabled module
   

   Nits:

   1. s/configuration/needed configuration/
   2. Missing trailing period.

3. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -330,4 +332,32 @@ public function setSyncing($status) {
   +    // dependencies with ModuleHandler and ThemeHandler.
   

   Nit: s/with/on/

4. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
   @@ -0,0 +1,14 @@
   + * An exception thrown if configuration already exists with the same name.
   

   Nit: s/configuration already exists with the same name/configuration with the same name already exists/

   ?

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -127,6 +130,19 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        // Profiles can not have config clashes.
   +        if ($module != drupal_get_profile()) {
   

   I first read "profiles can not have config clashes" as meaning "ensure that profiles don't have config clashes", but apparently this is saying that we only need to deal with actual modules, and not with install profiles, which are treated internally as modules also but cannot have config clashes by definition.

6. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -127,6 +130,19 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +          // Validate default configuration of this module. Bail if unable to
   +          // install. Should not continue installing more modules because those
   +          // may depend on this one.
   

   For the UX, it'd be ideal if this would be checked *before* actually kicking of the module installation logic, and warning the user immediately.

   Why is this necessary if ModulesListForm::submitForm() checks this already? I'm guessing it's necessary for e.g. Drush, i.e. when not using the UI, but the API, to install modules?

7. +++ b/core/modules/system/src/Controller/ConfigClashController.php
   @@ -0,0 +1,164 @@
   +   * @param \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface $key_value_expirable
   +   *   The key value expirable factory.
   +   * @param \Drupal\Core\Config\ConfigManagerInterface $config_manager
   +   *   The configuration manager.
   +   */
   +  public function __construct(KeyValueStoreExpirableInterface $key_value_expirable, ConfigManagerInterface $config_manager, ConfigInstallerInterface $config_installer) {
   

   Third parameter missing in docblock.

8. +++ b/core/modules/system/src/Controller/ConfigClashController.php
   @@ -0,0 +1,164 @@
   +            // Super weird.
   

   :)

9. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -496,6 +508,19 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +    // Check if we have any pre existing configuration.
   

   Nit: s/pre existing/pre-existing/

   ?

10. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -496,6 +508,19 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      $this->keyValueExpirable->setWithExpire($account, $modules, 60);
    

    This is weird, but I think this is part of the hack you mention in ConfigClashController::moduleReport(), which probably still needs to be resolved as you say in #85.

11. +++ b/core/modules/system/system.routing.yml
    @@ -285,6 +285,14 @@ system.theme_install:
    +    _content: 'Drupal\system\Controller\ConfigClashController::moduleReport'
    

    _controller :)

needs te indeed.

adding blocker tag, since this blocks #2090115: Don't install a module when its default configuration has unmet dependencies, so that the d8 blockers is accurate. Remove the blocker tag when this issue is fixed, and unpostpone 2090115.

Tagging - looking at the UI and tests.

Let's be honest.

Did some cleanups and added tests in ConfigInstallWebTest.

The UI now also catches the exception and shows a basic message. Depending on how you toggle modules in the UI, different things happen:

• Toggle config test and config install fail test: Then submitForm() in ModulesListForm is called and an exception is thrown and catched with config test enabled.
• Toggle config install fail test: you are redirect to ModulesListConfirmForm because of the dependency and then submitForm() in this class installs the modules. Exceptions is thrown and catched, with config test enabled.
• Toggle config install fail test when config test is already enabled: you go to the clash screen.

One consequence is that the check for preexisting configuration needs to happen in both submit methods which is kind of stupid right now.

Also should we check on pre existing configuration on new modules or not ?

Added test scenario's and screenshots to summary.

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -7,6 +7,7 @@
   +use Drupal\Component\Utility\String;
    use Drupal\Component\Utility\Unicode;
   

   This is not actually used.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -128,6 +131,20 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        // Install profiles can not have config clashes as if they contain
   +        // configuration with the same name as a module it will override it.
   +        if ($module != drupal_get_profile()) {
   +          // Validate default configuration of this module. Bail if unable to
   +          // install. Should not continue installing more modules because those
   +          // may depend on this one.
   +          $existing_configuration = \Drupal::service('config.installer')->findPreExistingConfiguration('module', $module);
   +          if (count($existing_configuration)) {
   +            throw new PreExistingConfigException(String::format('Configuration @config_names provided by @extension already exist in active configuration',
   +              array('@config_names' => implode(',', $existing_configuration[StorageInterface::DEFAULT_COLLECTION]), '@extension' => $module)
   +            ));
   +          }
   +        }
   

   This documentation is initially a bit hard to understand, unless you already know that we are going to hunt for config clashes, and that this is the only thing that will cause configuration to be considered invalid.

   I also think !empty() would be better than count() here.

3. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -18,9 +19,17 @@
   +  protected $admin_user;
   +
   

   According to coding standards, object properties should be lowercamelcased. This is not strictly followed in core though.

4. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -29,7 +38,7 @@ protected function setUp() {
   -  function testIntegrationModuleReinstallation() {
   +  function _testIntegrationModuleReinstallation() {
   

   s/_//

5. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -87,16 +106,16 @@ function testIntegrationModuleReinstallation() {
   -  function testInstallProfileConfigOverwrite() {
   +  function _testInstallProfileConfigOverwrite() {
   

   s/_//

6. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -146,4 +165,25 @@ function testInstallProfileConfigOverwrite() {
   +  /**
   +   * Tests pre existing configuration detection.
   +   */
   

   "pre-existing" instead of "pre existing".

7. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -146,4 +165,25 @@ function testInstallProfileConfigOverwrite() {
   +    // Try to install config_install_fail_test without config_test enabled.
   +    $this->drupalPostForm('admin/modules', array('modules[Testing][config_install_fail_test][enable]' => TRUE), t('Save configuration'));
   +    $this->drupalPostForm(NULL, array(), t('Continue'));
   +    $this->assertText('Configuration config_test.dynamic.dotted.default provided by config_install_fail_test already exist in active configuration');
   

   This can only work if config_install_fail_test depends on config_test. I looked this up in the info.yml file to verify an this is indeed the case. Maybe explain this here to avoid confusion?

8. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -146,4 +165,25 @@ function testInstallProfileConfigOverwrite() {
   +    // Try to install config_install_fail_dependency_test.
   +    $this->drupalPostForm('admin/modules', array('modules[Testing][config_install_fail_dependency_test][enable]' => TRUE), t('Save configuration'));
   

   config_install_fail_dependency_test depends on config_install_fail_test which depends on config_test. This is well tested :)
   Also worth a line of documentation.

Got half way through the review. Chimay Blue is clouding my judgement so I better stop :P

Back to needs work.

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -329,4 +331,32 @@ public function setSyncing($status) {
   +    $extension_config = $this->configFactory->get('core.extension');
   +    $enabled_extensions = array_keys((array) $extension_config->get('module'));
   +    $enabled_extensions += array_keys((array) $extension_config->get('theme'));
   +    // Add the extensions that will be enabled to the list of enabled
   +    // extensions.
   

   Don't we have to take into account install profiles here as well?

2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -110,6 +110,19 @@ public function getEntityTypeIdByName($name) {
   +  public function loadConfigEntityByName($name) {
   +    $entity_type_id = $this->getEntityTypeIdByName($name);
   +    if ($entity_type_id) {
   +      $entity_type = $this->entityManager->getDefinition($entity_type_id);
   +      $id = substr($name, strlen($entity_type->getConfigPrefix()) + 1);
   +      return $this->entityManager->getStorage($entity_type_id)->load($id);
   +    }
   +    return NULL;
   +  }
   +
   

   Note: This code is pretty much exactly the same as in ::findConfigEntityDependentsAsEntities() so I would argue we should better replace the usage there. The only difference is that the other implementation supports multi-loading.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -128,6 +131,20 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +          $existing_configuration = \Drupal::service('config.installer')->findPreExistingConfiguration('module', $module);
   

   Note: In that context you certainly have $config_installer available as variable.

4. +++ b/core/modules/system/system.routing.yml
   @@ -285,6 +285,14 @@ system.theme_install:
   +system.modules_config_clash:
   +  path: 'admin/modules/config-clash'
   +  defaults:
   +    _controller: 'Drupal\system\Controller\ConfigClashController::moduleReport'
   +    _title: 'Configuration clash'
   +  requirements:
   +    _permission: 'administer modules'
   +
   

   It is a little bit odd that this is not part of the config module, but well, it will otherwise require the config module to solve some of those conflicts.

Rerolled patch from #93.
Applied proposed changes from comments #96 and #98.

Added back try-catch block around $this->moduleInstaller->install.

Aw, shoot. It's a 0 byte patch. :( Any chance you've still got the changes laying around?

Overall, this looks great!

Code review

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -329,4 +330,32 @@ public function setSyncing($status) {
   +    // Add the extensions that will be enabled to the list of enabled
   +    // extensions.
   +    $enabled_extensions[] = $name;
   

   s/extensions/extension/

2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -110,6 +110,19 @@ public function getEntityTypeIdByName($name) {
   +  public function loadConfigEntityByName($name) {
   +    $entity_type_id = $this->getEntityTypeIdByName($name);
   +    if ($entity_type_id) {
   +      $entity_type = $this->entityManager->getDefinition($entity_type_id);
   +      $id = substr($name, strlen($entity_type->getConfigPrefix()) + 1);
   +      return $this->entityManager->getStorage($entity_type_id)->load($id);
   +    }
   +    return NULL;
   +  }
   

   Elegant :)

3. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
   @@ -24,6 +24,17 @@
   +   * Loads a configuration entity from the configuration name.
   

   s/from/by/, to match the function name?

4. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
   @@ -0,0 +1,73 @@
   +  protected $module;
   ...
   +  public function getModule() {
   ...
   +   * Creates an exception for a module name and a list of configuration objects.
   ...
   +      array('@config_names' => implode(', ', $config_objects), '@extension' => $module)
   

   s/module/extension/

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -149,6 +151,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        // Install profiles can not have config clashes as if they contain
   

   s/as if/if

6. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -149,6 +151,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        // configuration with the same name as a module it will override it.
   

   s/as a module it will override it/because it would be overridden/

   ?

7. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -149,4 +167,53 @@ function testInstallProfileConfigOverwrite() {
   +    // Try to install config_install_fail_test and config_test. This installs
   

   "This" === config_install_fail_test. Not as clear as it could be in the comment.

   EDIT: it becomes clear 12 lines further though.

8. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -149,4 +167,53 @@ function testInstallProfileConfigOverwrite() {
   +    // configuration clash before installation so the user will be taken to...
   

   "to..." -> unfinished?

9. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -149,4 +167,53 @@ function testInstallProfileConfigOverwrite() {
   +  }
    }
   

   Should have a blank line in between them.

10. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    @@ -0,0 +1,176 @@
    +  /**
    +   * The module handler service.
    +   *
    +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
    +   */
    +  protected $moduleHandler;
    ...
    +  /**
    +   * An associative list of modules to enable or disable.
    +   *
    +   * @var array
    +   */
    +  protected $modules = array();
    

    Unused; probably copy/paste leftover.

11. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    @@ -0,0 +1,176 @@
    +   * @param \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface $key_value_expirable
    +   *   The key value expirable factory.
    

    This doesn't match; you don't get the factory, but a specific store.

    It's correct in the class property docs.

12. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    @@ -0,0 +1,176 @@
    +            // @todo
    

    ?

13. +++ b/core/modules/system/src/Controller/ConfigClashController.php
    index 24f1b4b..d7234f4 100644
    --- a/core/modules/system/src/Form/ModulesListConfirmForm.php
    
    +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
    index 2486ca4..8347543 100644
    --- a/core/modules/system/src/Form/ModulesListForm.php
    

    There's a lot of code duplication between these two. Can we change it so it's DRY? Or why don't we want that? Because it's out of scope?

14. +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
    @@ -151,13 +164,37 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      $this->keyValueExpirable->setWithExpire($account, $this->modules, 60);
    
    +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -496,6 +509,19 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      $this->keyValueExpirable->setWithExpire($account, $modules, 60);
    

    Why 60 seconds? Why not 5, why not 300?

15. +++ b/core/modules/system/system.module
    @@ -145,6 +145,9 @@ function system_help($route_name, RouteMatchInterface $route_match) {
    +      return '<p>' . t('Here you can find a list of configuration that already exists. You need to either rename it or delete it before installing modules which have configuration with the same name.') . '</p>';
    

    "list of configuration" versus "it"

Manual testing

Having tried this, I now understand the 60-second caching (upon trying to install a module and a config clash being detected, the user is redirected to admin/modules/config-clash); but I don't understand why you did it this way. Why not make it part of the configuration form's output? Why a separate route? Because that's the only reason why there's this (IMHO awkward) 60-second caching AFAICT.

#108: great find! And simple fix in #109.

I can only find nitpicks this time.

1. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -175,45 +179,51 @@
   +    // The user is shown a confirm form becuase the config_test module is a
   

   s/becuause/because/

2. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -175,45 +179,51 @@
   +      ->set('label', 'Je suis Charlie')
   

   :)

3. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -175,45 +179,51 @@
   +    $this->assertRaw('Unable to install Configuration install fail test, <em class="placeholder">config_test.dynamic.dotted.default, language/fr/config_test.dynamic.dotted.default</em> already exist in active configuration.');
   

   Great expansion of this patch! Very valuable information indeed.

4. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -175,45 +179,51 @@
   +    // Test installing a theme through UI that has existing configuration.
   

   s/through UI/through the UI/

5. +++ b/core/modules/config/tests/config_collection_clash_install_test/config/install/another_collection/config_collection_install_test.test.yml
   @@ -0,0 +1 @@
   \ No newline at end of file
   
   +++ b/core/modules/config/tests/config_collection_clash_install_test/config/install/collection/test1/config_collection_install_test.test.yml
   @@ -0,0 +1 @@
   \ No newline at end of file
   
   +++ b/core/modules/config/tests/config_collection_clash_install_test/config/install/collection/test2/config_collection_install_test.test.yml
   @@ -0,0 +1 @@
   \ No newline at end of file
   

   .

6. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -517,7 +518,24 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   diff --git a/core/modules/system/src/Tests/Module/InstallUninstallTest.php b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   
   diff --git a/core/modules/system/src/Tests/Module/InstallUninstallTest.php b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   index 125b754..a2c8cd1 100644
   
   index 125b754..a2c8cd1 100644
   --- a/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   
   --- a/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   
   +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   @@ -7,7 +7,10 @@
   
   @@ -7,7 +7,10 @@
    
    namespace Drupal\system\Tests\Module;
    
   +use Drupal\Core\Entity\Entity\EntityViewMode;
   +use Drupal\Core\Field\Entity\BaseFieldOverride;
    use Drupal\Core\Logger\RfcLogLevel;
   +use Drupal\node\Entity\NodeType;
    
    /**
     * Install/uninstall core module and confirm table creation/deletion.
   diff --git a/core/modules/system/system.module b/core/modules/system/system.module
   
   diff --git a/core/modules/system/system.module b/core/modules/system/system.module
   index 255449e..5b30997 100644
   
   index 255449e..5b30997 100644
   --- a/core/modules/system/system.module
   
   --- a/core/modules/system/system.module
   +++ b/core/modules/system/system.module
   
   +++ b/core/modules/system/system.module
   +++ b/core/modules/system/system.module
   @@ -145,6 +145,7 @@ function system_help($route_name, RouteMatchInterface $route_match) {
   
   @@ -145,6 +145,7 @@ function system_help($route_name, RouteMatchInterface $route_match) {
    
        case 'system.status':
          return '<p>' . t("Here you can find a short overview of your site's parameters as well as any problems detected with your installation. It may be useful to copy and paste this information into support requests filed on drupal.org's support forums and project issue queues. Before filing a support request, ensure that your web server meets the <a href=\"@system-requirements\">system requirements.</a>", array('@system-requirements' => 'http://drupal.org/requirements')) . '</p>';
   +
      }
    }
   

   These are now unnecessary/unintentional changes.

7. +++ b/core/modules/config/src/Tests/ConfigInstallWebTest.php
   @@ -149,4 +171,59 @@ function testInstallProfileConfigOverwrite() {
   +    // Try to install config_install_fail_test without selecting config_test.
   +    // The user is shown a confirm form becuase the config_test module is a
   +    // dependency.
   ...
   +    // Test that collection configuration clashes are reported correctly.
   ...
   +    // Test installing a theme through UI that has existing configuration.
   ...
   +    // Theme install through the API.
   

   Ideally the symmetry here would also be reflected in the comments.

Went over it one more time. Found two very very silly nitpicks, that can easily be fixed upon commit. Patch looks great :)

1. +++ b/core/lib/Drupal/Core/Config/PreExistingConfigException.php
   @@ -0,0 +1,102 @@
   +  public static function create($extension, array $config_objects) {
   +
   +    $message = String::format('Configuration objects (@config_names) provided by @extension already exist in active configuration',
   

   Unnecessary blank line.

2. +++ b/core/modules/config/tests/config_clash_test_theme/config_clash_test_theme.info.yml
   @@ -0,0 +1,11 @@
   +#
   

   This is a copy/paste error; the other test extensions' info files don't have a comment. Can be deleted.

No major complaints but two questions on things that confused me a bit.

1. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -79,7 +79,7 @@ class ConfigInstaller implements ConfigInstallerInterface {
   @@ -155,16 +155,17 @@ protected function listDefaultConfigCollection($collection, $type, $name, array
   

   There's a lot of code shared between this and ConfigInstaller::installCollectionDefaultConfig().

   And ConfigInstaller::findPreExistingConfiguration() als does some similar things as installCollectionDefaultConfig(). Any way some of that code can be factored out?

2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -329,4 +330,31 @@ public function setSyncing($status) {
   +  public function findPreExistingConfiguration($type, $name) {
   

   So just to confirm findPreExistingConfiguration() includes any configuration shipped by install profiles?

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -149,6 +151,18 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +          $existing_configuration = $config_installer->findPreExistingConfiguration('module', $module);
   

   OK so install profiles are skipped before this code runs. But install profiles are included in findPreExistingConfiguration().

   I'm not really clear from this what happens in the following cases:

   1. Install profile and module are installed at the same time.

   2. Install profile is installed first, then module is enabled later.

   3. Install profile is installed with module at the same time. Module is uninstalled. Module is re-installed.

Boing!

Hmm OK. I'd somewhat assumed that profile overrides would continue to work if a module is uninstalled and re-installed, but quite happy this isn't the case.

Committed/pushed to 8.0.x, thanks!

Adding related issue #2431157: Review PreExistingConfigException special casing for install profile.