From e91e2874a6afbd9b794f54d1cad0787ff5f86ba9 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Fri, 11 Jan 2019 20:56:48 +0000 Subject: [PATCH] Issue #2955457 by pfrenssen, Chewie, unrealauk, alexpott, Pol: ConfigFactory static cache gets polluted with data from config overrides --- core/lib/Drupal/Core/Config/ConfigFactory.php | 21 +++- .../src/Kernel/OverriddenConfigImportTest.php | 99 +++++++++++++++++++ .../Core/Config/ConfigCRUDTest.php | 35 ++++++- 3 files changed, 152 insertions(+), 3 deletions(-) create mode 100644 core/modules/language/tests/src/Kernel/OverriddenConfigImportTest.php diff --git a/core/lib/Drupal/Core/Config/ConfigFactory.php b/core/lib/Drupal/Core/Config/ConfigFactory.php index e43a36c1f1..e4496f88b7 100644 --- a/core/lib/Drupal/Core/Config/ConfigFactory.php +++ b/core/lib/Drupal/Core/Config/ConfigFactory.php @@ -335,10 +335,18 @@ public function listAll($prefix = '') { * 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 @@ public function onConfigSave(ConfigCrudEvent $event) { * 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 0000000000..954245856f --- /dev/null +++ b/core/modules/language/tests/src/Kernel/OverriddenConfigImportTest.php @@ -0,0 +1,99 @@ +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 99a94d182f..1237e72487 100644 --- a/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php @@ -4,6 +4,7 @@ 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 @@ public function testCRUD() { $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 @@ public function testCRUD() { // 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(); -- GitLab