From b2b03c80610790fd7cfbeb3aa579e74130886b42 Mon Sep 17 00:00:00 2001 From: catch Date: Wed, 10 Jan 2024 11:34:42 +0000 Subject: [PATCH] Issue #3406929 by alexpott, quietone, bircher, borisson_, Wim Leers: Configuration being imported by the ConfigImporter sometimes has stale original data --- .../lib/Drupal/Core/Config/ConfigImporter.php | 5 ++ .../Drupal/Core/Config/StorageComparer.php | 72 +++++++++++++++---- .../Core/Config/StorageComparerInterface.php | 11 +++ .../tests/config_test/config_test.install | 19 +++++ .../Core/Config/ConfigImporterTest.php | 39 +++++++++- .../Drupal/KernelTests/KernelTestBase.php | 4 ++ 6 files changed, 136 insertions(+), 14 deletions(-) create mode 100644 core/modules/config/tests/config_test/config_test.install diff --git a/core/lib/Drupal/Core/Config/ConfigImporter.php b/core/lib/Drupal/Core/Config/ConfigImporter.php index 7c161a5f247..0fd7e8a1460 100644 --- a/core/lib/Drupal/Core/Config/ConfigImporter.php +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php @@ -556,6 +556,9 @@ class ConfigImporter { * Exception thrown if the $sync_step can not be called. */ public function doSyncStep($sync_step, &$context) { + if ($this->validated) { + $this->storageComparer->writeMode(); + } if (is_string($sync_step) && method_exists($this, $sync_step)) { \Drupal::service('config.installer')->setSyncing(TRUE); $this->$sync_step($context); @@ -1132,6 +1135,8 @@ class ConfigImporter { // the new container. $this->__sleep(); $this->__wakeup(); + $this->storageComparer->__sleep(); + $this->storageComparer->__wakeup(); } } diff --git a/core/lib/Drupal/Core/Config/StorageComparer.php b/core/lib/Drupal/Core/Config/StorageComparer.php index 51f7bec937d..76521629c25 100644 --- a/core/lib/Drupal/Core/Config/StorageComparer.php +++ b/core/lib/Drupal/Core/Config/StorageComparer.php @@ -3,6 +3,7 @@ namespace Drupal\Core\Config; use Drupal\Core\Cache\MemoryBackend; +use Drupal\Core\Cache\NullBackend; use Drupal\Core\Config\Entity\ConfigDependencyManager; use Drupal\Core\DependencyInjection\DependencySerializationTrait; @@ -10,7 +11,10 @@ use Drupal\Core\DependencyInjection\DependencySerializationTrait; * Defines a config storage comparer. */ class StorageComparer implements StorageComparerInterface { - use DependencySerializationTrait; + use DependencySerializationTrait { + __sleep as defaultSleep; + __wakeup as defaultWakeup; + } /** * The source storage used to discover configuration changes. @@ -77,10 +81,21 @@ class StorageComparer implements StorageComparerInterface { /** * A memory cache backend to statically cache target configuration data. * - * @var \Drupal\Core\Cache\MemoryBackend + * @var \Drupal\Core\Cache\CacheBackendInterface */ protected $targetCacheStorage; + /** + * Indicates whether the target storage should be wrapped in a cache. + * + * In write mode the StorageComparer no longer wraps the target storage in a + * static cache. When writing to active configuration, the target storage must + * reflect any secondary writes to configuration that occur. + * + * @var bool + */ + protected bool $writeMode = FALSE; + /** * Constructs the Configuration storage comparer. * @@ -97,18 +112,16 @@ class StorageComparer implements StorageComparerInterface { $target_storage = $target_storage->createCollection(StorageInterface::DEFAULT_COLLECTION); } - // Wrap the storages in a static cache so that multiple reads of the same - // raw configuration object are not costly. + // Wrap the source storage in a static cache so that multiple reads of the + // same raw configuration object are not costly. $this->sourceCacheStorage = new MemoryBackend(); $this->sourceStorage = new CachedStorage( $source_storage, $this->sourceCacheStorage ); + $this->targetCacheStorage = new MemoryBackend(); - $this->targetStorage = new CachedStorage( - $target_storage, - $this->targetCacheStorage - ); + $this->targetStorage = $target_storage; $this->changelist[StorageInterface::DEFAULT_COLLECTION] = $this->getEmptyChangelist(); } @@ -132,16 +145,35 @@ class StorageComparer implements StorageComparerInterface { */ public function getTargetStorage($collection = StorageInterface::DEFAULT_COLLECTION) { if (!isset($this->targetStorages[$collection])) { - if ($collection == StorageInterface::DEFAULT_COLLECTION) { - $this->targetStorages[$collection] = $this->targetStorage; + $target = $this->targetStorage; + if ($collection !== StorageInterface::DEFAULT_COLLECTION) { + $target = $target->createCollection($collection); } - else { - $this->targetStorages[$collection] = $this->targetStorage->createCollection($collection); + // If we are not in write mode wrap the storage in a static cache so that + // multiple reads of the same configuration object are cheap. + if (!$this->writeMode) { + $target = new CachedStorage( + $target, + $this->targetCacheStorage + ); } + $this->targetStorages[$collection] = $target; } return $this->targetStorages[$collection]; } + /** + * {@inheritdoc} + */ + public function writeMode(): static { + if (!$this->writeMode) { + $this->writeMode = TRUE; + $this->targetCacheStorage = new NullBackend('storage_comparer'); + $this->targetStorages = []; + } + return $this; + } + /** * {@inheritdoc} */ @@ -458,4 +490,20 @@ class StorageComparer implements StorageComparerInterface { return $collections; } + /** + * {@inheritdoc} + */ + public function __sleep(): array { + return array_diff($this->defaultSleep(), ['targetStorages']); + } + + /** + * {@inheritdoc} + */ + public function __wakeup(): void { + $this->defaultWakeup(); + $this->targetStorages = []; + $this->targetCacheStorage->deleteAll(); + } + } diff --git a/core/lib/Drupal/Core/Config/StorageComparerInterface.php b/core/lib/Drupal/Core/Config/StorageComparerInterface.php index ee61c51a4d4..7961b4ec0fb 100644 --- a/core/lib/Drupal/Core/Config/StorageComparerInterface.php +++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.php @@ -31,6 +31,17 @@ interface StorageComparerInterface { */ public function getTargetStorage($collection = StorageInterface::DEFAULT_COLLECTION); + /** + * Changes the StorageComparer to write mode. + * + * Tells the StorageComparer that the target storage is going to be used for + * writing. This information allows implementations to optimize caching + * strategies for reading from or writing to the target storage. + * + * @return $this + */ + public function writeMode(): static; + /** * Gets an empty changelist. * diff --git a/core/modules/config/tests/config_test/config_test.install b/core/modules/config/tests/config_test/config_test.install new file mode 100644 index 00000000000..80835a62c31 --- /dev/null +++ b/core/modules/config/tests/config_test/config_test.install @@ -0,0 +1,19 @@ +get('config_test_install.foo_value'); + if ($secondary_write !== NULL) { + \Drupal::configFactory() + ->getEditable('config_test.system') + ->set('foo', $secondary_write) + ->save(); + } +} diff --git a/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php index 9f3e9fd9a29..004ce2f138a 100644 --- a/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterTest.php @@ -843,7 +843,7 @@ class ConfigImporterTest extends KernelTestBase { $extensions['module']['system_test'] = 0; $extensions['module'] = module_config_sort($extensions['module']); $sync->write('core.extension', $extensions); - $this->configImporter->reset()->import(); + $this->configImporter()->import(); // Syncing values stored in state by hook_module_preinstall should be TRUE // when module is installed via config import. @@ -868,7 +868,7 @@ class ConfigImporterTest extends KernelTestBase { // by uninstall hooks should be TRUE. unset($extensions['module']['module_test']); $sync->write('core.extension', $extensions); - $this->configImporter->reset()->import(); + $this->configImporter()->import(); $this->assertTrue(\Drupal::state()->get('system_test_preuninstall_module_config_installer_syncing'), '\Drupal::isConfigSyncing() in system_test_module_preuninstall() returns TRUE'); $this->assertTrue(\Drupal::state()->get('system_test_preuninstall_module_syncing_param'), 'system_test_module_preuninstall() $is_syncing value is TRUE'); $this->assertTrue(\Drupal::state()->get('system_test_modules_uninstalled_config_installer_syncing'), '\Drupal::isConfigSyncing() in system_test_modules_uninstalled returns TRUE'); @@ -1013,6 +1013,41 @@ class ConfigImporterTest extends KernelTestBase { $this->assertSame($collections, $event_collections); } + /** + * Tests the target storage caching during configuration import. + */ + public function testStorageComparerTargetStorage(): void { + $this->installConfig(['config_events_test']); + $this->copyConfig($this->container->get('config.storage'), $this->container->get('config.storage.sync')); + $this->assertTrue($this->container->get('module_installer')->uninstall(['config_test'])); + \Drupal::state()->set('config_events_test.all_events', []); + \Drupal::state()->set('config_test_install.foo_value', 'transient'); + + // Prime the active config cache. If the ConfigImporter and StorageComparer + // do not manage the target storage correctly this cache can pollute the + // data. + \Drupal::configFactory()->get('config_test.system'); + + // Import the configuration. This results in a save event with the value + // changing from foo to bar. + $this->configImporter()->import(); + $all_events = \Drupal::state()->get('config_events_test.all_events'); + + // Test that the values change as expected during the configuration import. + $this->assertCount(3, $all_events[ConfigEvents::SAVE]['config_test.system']); + // First, the values are set by the module installer using the configuration + // import's source storage. + $this->assertSame([], $all_events[ConfigEvents::SAVE]['config_test.system'][0]['original_config_data']); + $this->assertSame('bar', $all_events[ConfigEvents::SAVE]['config_test.system'][0]['current_config_data']['foo']); + // Next, the config_test_install() function changes the value. + $this->assertSame('bar', $all_events[ConfigEvents::SAVE]['config_test.system'][1]['original_config_data']['foo']); + $this->assertSame('transient', $all_events[ConfigEvents::SAVE]['config_test.system'][1]['current_config_data']['foo']); + // Last, the config importer processes all the configuration in the source + // storage and ensures the values are as expected. + $this->assertSame('transient', $all_events[ConfigEvents::SAVE]['config_test.system'][2]['original_config_data']['foo']); + $this->assertSame('bar', $all_events[ConfigEvents::SAVE]['config_test.system'][2]['current_config_data']['foo']); + } + /** * Helper method to test custom config installer steps. * diff --git a/core/tests/Drupal/KernelTests/KernelTestBase.php b/core/tests/Drupal/KernelTests/KernelTestBase.php index 8b381452839..d89d738e38d 100644 --- a/core/tests/Drupal/KernelTests/KernelTestBase.php +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php @@ -613,6 +613,10 @@ abstract class KernelTestBase extends TestCase implements ServiceProviderInterfa $route_provider_definition = new Definition(RouteProvider::class); $route_provider_definition->setPublic(TRUE); $container->setDefinition($id, $route_provider_definition); + + // Remove the stored configuration importer so if used again it will be + // built with up-to-date services. + $this->configImporter = NULL; } /**