Optional profile configuration can be unexpectedly recreated after being deleted

I have replicated this and I believe I've found the source of the bundles reappearing. They're being recreated from the optional configuration in the standard install profile. If you delete /core/profiles/standard/optional/media.type.audio.yml file and then delete the audio entity. Then install and enable the webform module (can be any module), the audio bundle is not installed. This seems like an issue with configuration management and unrelated to the media module.

ooh nice find. I'd guess this is an unintended impact of #2930996: Config installer doesn't install possible installable config

So it doesn't happen with any module but from the core modules it does happen with Forum - I think it is any module with optional configuration... so Book causes it too.

Here's a fix. We only should be checking the profile's optional config for new things to install when we call installOptionalConfig() for the second time during a module install.

This is a major bug. Configuration coming back after being deleted is very confusing and potentially very bad for a site.

Here's a test that problem.

The more I think about this I think we have a new critical on our hands. Yes there is a work around - delete the config again - but that would mean you'd have to realise this has happened and you might not. And this type of unexpected change could have consequences for your site.

+1 RTBC on Patch #9. Solution looks good in code as well. Can we set a target for this to be merged in 8.6.x-dev too?

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -171,7 +171,11 @@ public function installOptionalConfig(StorageInterface $storage = NULL, $depende
+      if (!empty($dependency)) {
+        // If we're processing a new dependency then add all the optional
+        // profile config into the list.
+        $optional_profile_config = $profile_storage->listAll();

is it possible that some dependencies will enable profile provided optional configuration to be required - just not all of it?

@larowlan yep that can totally occur when installing a new module. For example, when views is installed. We already have tests for this in ConfigInstallProfileOverrideTest - see the entity with the ID completely_new.

I've tried to make what's going on easier to understand by:

• Improving the docs
• Having less variables in the method

This patch is better to review in the full context of the \Drupal\Core\Config\ConfigInstaller::installOptionalConfig().

Here's is the whole function context:

diff --git a/core/lib/Drupal/Core/Config/ConfigInstaller.php b/core/lib/Drupal/Core/Config/ConfigInstaller.php
index abcfc8b426..4a198a7252 100644
--- a/core/lib/Drupal/Core/Config/ConfigInstaller.php
+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -158,97 +158,104 @@ public function installDefaultConfig($type, $name) {
   /**
    * {@inheritdoc}
    */
   public function installOptionalConfig(StorageInterface $storage = NULL, $dependency = []) {
     $profile = $this->drupalGetProfile();
-    $optional_profile_config = [];
+    $enabled_extensions = $this->getEnabledExtensions();
+    $existing_config = $this->getActiveStorages()->listAll();
+
+    // Create the storages to read configuration from.
     if (!$storage) {
       // Search the install profile's optional configuration too.
       $storage = new ExtensionInstallStorage($this->getActiveStorages(StorageInterface::DEFAULT_COLLECTION), InstallStorage::CONFIG_OPTIONAL_DIRECTORY, StorageInterface::DEFAULT_COLLECTION, TRUE, $this->installProfile);
       // The extension install storage ensures that overrides are used.
       $profile_storage = NULL;
     }
     elseif (!empty($profile)) {
       // Creates a profile storage to search for overrides.
       $profile_install_path = $this->drupalGetPath('module', $profile) . '/' . InstallStorage::CONFIG_OPTIONAL_DIRECTORY;
       $profile_storage = new FileStorage($profile_install_path, StorageInterface::DEFAULT_COLLECTION);
-      if (!empty($dependency)) {
-        // If we're processing a new dependency then add all the optional
-        // profile config into the list.
-        $optional_profile_config = $profile_storage->listAll();
-      }
     }
     else {
       // Profile has not been set yet. For example during the first steps of the
       // installer or during unit tests.
       $profile_storage = NULL;
     }
 
-    $enabled_extensions = $this->getEnabledExtensions();
-    $existing_config = $this->getActiveStorages()->listAll();
+    // Build the list of possible configuration to create.
+    $list = $storage->listAll();
+    if ($profile_storage && !empty($dependency)) {
+      // Only add the optional profile configuration into the list if we are
+      // have a dependency to check. This ensures that optional profile
+      // configuration is not unexpectedly re-created after being deleted.
+      $list = array_unique(array_merge($list, $profile_storage->listAll()));
+    }
 
-    $list = array_unique(array_merge($storage->listAll(), $optional_profile_config));
+    // Filter the list of configuration to only include configuration that
+    // should be created.
     $list = array_filter($list, function ($config_name) use ($existing_config) {
       // Only list configuration that:
       // - does not already exist
       // - is a configuration entity (this also excludes config that has an
       //   implicit dependency on modules that are not yet installed)
       return !in_array($config_name, $existing_config) && $this->configManager->getEntityTypeIdByName($config_name);
     });
 
     $all_config = array_merge($existing_config, $list);
     $all_config = array_combine($all_config, $all_config);
     $config_to_create = $storage->readMultiple($list);
     // Check to see if the corresponding override storage has any overrides or
     // new configuration that can be installed.
     if ($profile_storage) {
       $config_to_create = $profile_storage->readMultiple($list) + $config_to_create;
     }
     // Sort $config_to_create in the order of the least dependent first.
     $dependency_manager = new ConfigDependencyManager();
     $dependency_manager->setData($config_to_create);
     $config_to_create = array_merge(array_flip($dependency_manager->sortAll()), $config_to_create);
     if (!empty($dependency)) {
       // In order to work out dependencies we need the full config graph.
       $dependency_manager->setData($this->getActiveStorages()->readMultiple($existing_config) + $config_to_create);
       $dependencies = $dependency_manager->getDependentEntities(key($dependency), reset($dependency));
     }
 
     foreach ($config_to_create as $config_name => $data) {
       // Remove configuration where its dependencies cannot be met.
       $remove = !$this->validateDependencies($config_name, $data, $enabled_extensions, $all_config);
       // Remove configuration that is not dependent on $dependency, if it is
       // defined.
       if (!$remove && !empty($dependency)) {
         $remove = !isset($dependencies[$config_name]);
       }
 
       if ($remove) {
         // Remove from the list of configuration to create.
         unset($config_to_create[$config_name]);
         // Remove from the list of all configuration. This ensures that any
         // configuration that depends on this configuration is also removed.
         unset($all_config[$config_name]);
       }
     }
+
+    // Create the optional configuration if there is any left after filtering.
     if (!empty($config_to_create)) {
       $this->createConfiguration(StorageInterface::DEFAULT_COLLECTION, $config_to_create, TRUE);
     }
   }

Making the title match the issue and sound as critical as this is.

Edited title for clarity.

I was wrong about when this was introduced. We caused this in #2513604: Create default responsive image styles - when we added the ability for profiles to provide completely new optional configuration that would be installed when their dependencies were installed.

Looks good to me, nice cleanup @alexpott - much easier to work out what is going on.

Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!