Skip to content

Commit 4bcd11e

Browse files
alexpottalexpott
committed
Issue #2955457 by pfrenssen, Chewie, unrealauk, alexpott, Pol: ConfigFactory static cache gets polluted with data from config overrides
1 parent ebb7029 commit 4bcd11e

File tree

3 files changed

+152
-3
lines changed

3 files changed

+152
-3
lines changed

‎lib/Drupal/Core/Config/ConfigFactory.php‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -335,10 +335,18 @@ public function listAll($prefix = '') {
335335
* The configuration event.
336336
*/
337337
public function onConfigSave(ConfigCrudEvent $event) {
338+
$saved_config = $event->getConfig();
339+
340+
// We are only concerned with config objects that belong to the collection
341+
// that matches the storage we depend on. Skip if the event was fired for a
342+
// config object belonging to a different collection.
343+
if ($saved_config->getStorage()->getCollectionName() !== $this->storage->getCollectionName()) {
344+
return;
345+
}
346+
338347
// Ensure that the static cache contains up to date configuration objects by
339348
// replacing the data on any entries for the configuration object apart
340349
// from the one that references the actual config object being saved.
341-
$saved_config = $event->getConfig();
342350
foreach ($this->getConfigCacheKeys($saved_config->getName()) as $cache_key) {
343351
$cached_config = $this->cache[$cache_key];
344352
if ($cached_config !== $saved_config) {
@@ -356,8 +364,17 @@ public function onConfigSave(ConfigCrudEvent $event) {
356364
* The configuration event.
357365
*/
358366
public function onConfigDelete(ConfigCrudEvent $event) {
367+
$deleted_config = $event->getConfig();
368+
369+
// We are only concerned with config objects that belong to the collection
370+
// that matches the storage we depend on. Skip if the event was fired for a
371+
// config object belonging to a different collection.
372+
if ($deleted_config->getStorage()->getCollectionName() !== $this->storage->getCollectionName()) {
373+
return;
374+
}
375+
359376
// Ensure that the static cache does not contain deleted configuration.
360-
foreach ($this->getConfigCacheKeys($event->getConfig()->getName()) as $cache_key) {
377+
foreach ($this->getConfigCacheKeys($deleted_config->getName()) as $cache_key) {
361378
unset($this->cache[$cache_key]);
362379
}
363380
}

‎modules/language/tests/src/Kernel/OverriddenConfigImportTest.php‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
<?php
2+
3+
namespace Drupal\Tests\language\Kernel;
4+
5+
use Drupal\Core\Config\ConfigImporter;
6+
use Drupal\Core\Config\StorageComparer;
7+
use Drupal\KernelTests\KernelTestBase;
8+
9+
/**
10+
* Tests importing of config with language overrides.
11+
*
12+
* @group language
13+
*/
14+
class OverriddenConfigImportTest extends KernelTestBase {
15+
16+
/**
17+
* Config Importer object used for testing.
18+
*
19+
* @var \Drupal\Core\Config\ConfigImporter
20+
*/
21+
protected $configImporter;
22+
23+
/**
24+
* {@inheritdoc}
25+
*/
26+
protected static $modules = ['system', 'language'];
27+
28+
/**
29+
* {@inheritdoc}
30+
*/
31+
protected function setUp() {
32+
parent::setUp();
33+
34+
$this->installConfig(['system']);
35+
$this->copyConfig($this->container->get('config.storage'), $this->container->get('config.storage.sync'));
36+
37+
// Set up the ConfigImporter object for testing.
38+
$storage_comparer = new StorageComparer(
39+
$this->container->get('config.storage.sync'),
40+
$this->container->get('config.storage'),
41+
$this->container->get('config.manager')
42+
);
43+
$this->configImporter = new ConfigImporter(
44+
$storage_comparer->createChangelist(),
45+
$this->container->get('event_dispatcher'),
46+
$this->container->get('config.manager'),
47+
$this->container->get('lock'),
48+
$this->container->get('config.typed'),
49+
$this->container->get('module_handler'),
50+
$this->container->get('module_installer'),
51+
$this->container->get('theme_handler'),
52+
$this->container->get('string_translation')
53+
);
54+
}
55+
56+
/**
57+
* Tests importing overridden config alongside config in the default language.
58+
*/
59+
public function testConfigImportUpdates() {
60+
$storage = $this->container->get('config.storage');
61+
$sync = $this->container->get('config.storage.sync');
62+
/** @var \Drupal\language\ConfigurableLanguageManagerInterface $language_manager */
63+
$language_manager = $this->container->get('language_manager');
64+
65+
// Make a change to the site configuration in the default collection.
66+
$data = $storage->read('system.site');
67+
$data['name'] = 'English site name';
68+
$sync->write('system.site', $data);
69+
70+
// Also make a change to the same config object, but using a language
71+
// override.
72+
/* @var \Drupal\Core\Config\StorageInterface $overridden_sync */
73+
$overridden_sync = $sync->createCollection('language.fr');
74+
$overridden_sync->write('system.site', ['name' => 'French site name']);
75+
76+
// Before we start the import, the change to the site name should not be
77+
// present. This action also primes the cache in the config factory so that
78+
// we can test whether the cached data is correctly updated.
79+
$config = $this->config('system.site');
80+
$this->assertNotEquals('English site name', $config->getRawData()['name']);
81+
82+
// Before the import is started the site name should not yet be overridden.
83+
$this->assertFalse($config->hasOverrides());
84+
$override = $language_manager->getLanguageConfigOverride('fr', 'system.site');
85+
$this->assertTrue($override->isNew());
86+
87+
// Start the import of the new configuration.
88+
$this->configImporter->reset()->import();
89+
90+
// Verify the new site name in the default language.
91+
$config = $this->config('system.site')->getRawData();
92+
$this->assertEquals('English site name', $config['name']);
93+
94+
// Verify the overridden site name.
95+
$override = $language_manager->getLanguageConfigOverride('fr', 'system.site');
96+
$this->assertEquals('French site name', $override->get('name'));
97+
}
98+
99+
}

‎tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use Drupal\Component\Utility\Crypt;
66
use Drupal\Component\Render\FormattableMarkup;
7+
use Drupal\Core\Config\Config;
78
use Drupal\Core\Config\ConfigNameException;
89
use Drupal\Core\Config\ConfigValueException;
910
use Drupal\Core\Config\InstallStorage;
@@ -38,14 +39,19 @@ class ConfigCRUDTest extends KernelTestBase {
3839
* Tests CRUD operations.
3940
*/
4041
public function testCRUD() {
42+
$event_dispatcher = $this->container->get('event_dispatcher');
43+
$typed_config_manager = $this->container->get('config.typed');
44+
4145
$storage = $this->container->get('config.storage');
46+
$collection_storage = $storage->createCollection('test_collection');
47+
4248
$config_factory = $this->container->get('config.factory');
4349
$name = 'config_test.crud';
4450

51+
// Create a new configuration object in the default collection.
4552
$config = $this->config($name);
4653
$this->assertIdentical($config->isNew(), TRUE);
4754

48-
// Create a new configuration object.
4955
$config->set('value', 'initial');
5056
$config->save();
5157
$this->assertIdentical($config->isNew(), FALSE);
@@ -54,6 +60,25 @@ public function testCRUD() {
5460
$actual_data = $storage->read($name);
5561
$this->assertIdentical($actual_data, ['value' => 'initial']);
5662

63+
// Verify the config factory contains the saved value.
64+
$actual_data = $config_factory->get($name)->getRawData();
65+
$this->assertIdentical($actual_data, ['value' => 'initial']);
66+
67+
// Create another instance of the config object using a custom collection.
68+
$collection_config = new Config(
69+
$name,
70+
$collection_storage,
71+
$event_dispatcher,
72+
$typed_config_manager
73+
);
74+
$collection_config->set('value', 'overridden');
75+
$collection_config->save();
76+
77+
// Verify that the config factory still returns the right value, from the
78+
// config instance in the default collection.
79+
$actual_data = $config_factory->get($name)->getRawData();
80+
$this->assertIdentical($actual_data, ['value' => 'initial']);
81+
5782
// Update the configuration object instance.
5883
$config->set('value', 'instance-update');
5984
$config->save();
@@ -71,6 +96,14 @@ public function testCRUD() {
7196
// Pollute the config factory static cache.
7297
$config_factory->getEditable($name);
7398

99+
// Delete the config object that uses a custom collection. This should not
100+
// affect the instance returned by the config factory which depends on the
101+
// default collection storage.
102+
$collection_config->delete();
103+
$actual_config = $config_factory->get($name);
104+
$this->assertIdentical($actual_config->isNew(), FALSE);
105+
$this->assertIdentical($actual_config->getRawData(), ['value' => 'instance-update']);
106+
74107
// Delete the configuration object.
75108
$config->delete();
76109

0 commit comments

Comments
 (0)