Issue #2955457 by pfrenssen, Chewie, unrealauk, alexpott, Pol: ConfigFactory static cache gets polluted with data from config overrides

3 files changed, 152 insertions, 3 deletions

diff --git a/core/lib/Drupal/Core/Config/ConfigFactory.php b/core/lib/Drupal/Core/Config/ConfigFactory.php
index e43a36c..e4496f8 100644
--- a/core/lib/Drupal/Core/Config/ConfigFactory.php
+++ b/core/lib/Drupal/Core/Config/ConfigFactory.php
@@ -335,10 +335,18 @@ class ConfigFactory implements ConfigFactoryInterface, EventSubscriberInterface
    *   The configuration event.
    */
   public function onConfigSave(ConfigCrudEvent $event) {
+    $saved_config = $event->getConfig();
+
+    // We are only concerned with config objects that belong to the collection
+    // that matches the storage we depend on. Skip if the event was fired for a
+    // config object belonging to a different collection.
+    if ($saved_config->getStorage()->getCollectionName() !== $this->storage->getCollectionName()) {
+      return;
+    }
+
     // Ensure that the static cache contains up to date configuration objects by
     // replacing the data on any entries for the configuration object apart
     // from the one that references the actual config object being saved.
-    $saved_config = $event->getConfig();
     foreach ($this->getConfigCacheKeys($saved_config->getName()) as $cache_key) {
       $cached_config = $this->cache[$cache_key];
       if ($cached_config !== $saved_config) {
@@ -356,8 +364,17 @@ class ConfigFactory implements ConfigFactoryInterface, EventSubscriberInterface
    *   The configuration event.
    */
   public function onConfigDelete(ConfigCrudEvent $event) {
+    $deleted_config = $event->getConfig();
+
+    // We are only concerned with config objects that belong to the collection
+    // that matches the storage we depend on. Skip if the event was fired for a
+    // config object belonging to a different collection.
+    if ($deleted_config->getStorage()->getCollectionName() !== $this->storage->getCollectionName()) {
+      return;
+    }
+
     // Ensure that the static cache does not contain deleted configuration.
-    foreach ($this->getConfigCacheKeys($event->getConfig()->getName()) as $cache_key) {
+    foreach ($this->getConfigCacheKeys($deleted_config->getName()) as $cache_key) {
       unset($this->cache[$cache_key]);
     }
   }
diff --git a/core/modules/language/tests/src/Kernel/OverriddenConfigImportTest.php b/core/modules/language/tests/src/Kernel/OverriddenConfigImportTest.php
new file mode 100644
index 0000000..9542458
--- /dev/null
+++ b/core/modules/language/tests/src/Kernel/OverriddenConfigImportTest.php
@@ -0,0 +1,99 @@
+<?php
+
+namespace Drupal\Tests\language\Kernel;
+
+use Drupal\Core\Config\ConfigImporter;
+use Drupal\Core\Config\StorageComparer;
+use Drupal\KernelTests\KernelTestBase;
+
+/**
+ * Tests importing of config with language overrides.
+ *
+ * @group language
+ */
+class OverriddenConfigImportTest extends KernelTestBase {
+
+  /**
+   * Config Importer object used for testing.
+   *
+   * @var \Drupal\Core\Config\ConfigImporter
+   */
+  protected $configImporter;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['system', 'language'];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    $this->installConfig(['system']);
+    $this->copyConfig($this->container->get('config.storage'), $this->container->get('config.storage.sync'));
+
+    // Set up the ConfigImporter object for testing.
+    $storage_comparer = new StorageComparer(
+      $this->container->get('config.storage.sync'),
+      $this->container->get('config.storage'),
+      $this->container->get('config.manager')
+    );
+    $this->configImporter = new ConfigImporter(
+      $storage_comparer->createChangelist(),
+      $this->container->get('event_dispatcher'),
+      $this->container->get('config.manager'),
+      $this->container->get('lock'),
+      $this->container->get('config.typed'),
+      $this->container->get('module_handler'),
+      $this->container->get('module_installer'),
+      $this->container->get('theme_handler'),
+      $this->container->get('string_translation')
+    );
+  }
+
+  /**
+   * Tests importing overridden config alongside config in the default language.
+   */
+  public function testConfigImportUpdates() {
+    $storage = $this->container->get('config.storage');
+    $sync = $this->container->get('config.storage.sync');
+    /** @var \Drupal\language\ConfigurableLanguageManagerInterface $language_manager */
+    $language_manager = $this->container->get('language_manager');
+
+    // Make a change to the site configuration in the default collection.
+    $data = $storage->read('system.site');
+    $data['name'] = 'English site name';
+    $sync->write('system.site', $data);
+
+    // Also make a change to the same config object, but using a language
+    // override.
+    /* @var \Drupal\Core\Config\StorageInterface $overridden_sync */
+    $overridden_sync = $sync->createCollection('language.fr');
+    $overridden_sync->write('system.site', ['name' => 'French site name']);
+
+    // Before we start the import, the change to the site name should not be
+    // present. This action also primes the cache in the config factory so that
+    // we can test whether the cached data is correctly updated.
+    $config = $this->config('system.site');
+    $this->assertNotEquals('English site name', $config->getRawData()['name']);
+
+    // Before the import is started the site name should not yet be overridden.
+    $this->assertFalse($config->hasOverrides());
+    $override = $language_manager->getLanguageConfigOverride('fr', 'system.site');
+    $this->assertTrue($override->isNew());
+
+    // Start the import of the new configuration.
+    $this->configImporter->reset()->import();
+
+    // Verify the new site name in the default language.
+    $config = $this->config('system.site')->getRawData();
+    $this->assertEquals('English site name', $config['name']);
+
+    // Verify the overridden site name.
+    $override = $language_manager->getLanguageConfigOverride('fr', 'system.site');
+    $this->assertEquals('French site name', $override->get('name'));
+  }
+
+}
diff --git a/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php b/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php
index 99a94d1..1237e72 100644
--- a/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php
@@ -4,6 +4,7 @@ namespace Drupal\KernelTests\Core\Config;
 
 use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Render\FormattableMarkup;
+use Drupal\Core\Config\Config;
 use Drupal\Core\Config\ConfigNameException;
 use Drupal\Core\Config\ConfigValueException;
 use Drupal\Core\Config\InstallStorage;
@@ -38,14 +39,19 @@ class ConfigCRUDTest extends KernelTestBase {
    * Tests CRUD operations.
    */
   public function testCRUD() {
+    $event_dispatcher = $this->container->get('event_dispatcher');
+    $typed_config_manager = $this->container->get('config.typed');
+
     $storage = $this->container->get('config.storage');
+    $collection_storage = $storage->createCollection('test_collection');
+
     $config_factory = $this->container->get('config.factory');
     $name = 'config_test.crud';
 
+    // Create a new configuration object in the default collection.
     $config = $this->config($name);
     $this->assertIdentical($config->isNew(), TRUE);
 
-    // Create a new configuration object.
     $config->set('value', 'initial');
     $config->save();
     $this->assertIdentical($config->isNew(), FALSE);
@@ -54,6 +60,25 @@ class ConfigCRUDTest extends KernelTestBase {
     $actual_data = $storage->read($name);
     $this->assertIdentical($actual_data, ['value' => 'initial']);
 
+    // Verify the config factory contains the saved value.
+    $actual_data = $config_factory->get($name)->getRawData();
+    $this->assertIdentical($actual_data, ['value' => 'initial']);
+
+    // Create another instance of the config object using a custom collection.
+    $collection_config = new Config(
+      $name,
+      $collection_storage,
+      $event_dispatcher,
+      $typed_config_manager
+    );
+    $collection_config->set('value', 'overridden');
+    $collection_config->save();
+
+    // Verify that the config factory still returns the right value, from the
+    // config instance in the default collection.
+    $actual_data = $config_factory->get($name)->getRawData();
+    $this->assertIdentical($actual_data, ['value' => 'initial']);
+
     // Update the configuration object instance.
     $config->set('value', 'instance-update');
     $config->save();
@@ -71,6 +96,14 @@ class ConfigCRUDTest extends KernelTestBase {
     // Pollute the config factory static cache.
     $config_factory->getEditable($name);
 
+    // Delete the config object that uses a custom collection. This should not
+    // affect the instance returned by the config factory which depends on the
+    // default collection storage.
+    $collection_config->delete();
+    $actual_config = $config_factory->get($name);
+    $this->assertIdentical($actual_config->isNew(), FALSE);
+    $this->assertIdentical($actual_config->getRawData(), ['value' => 'instance-update']);
+
     // Delete the configuration object.
     $config->delete();