Issue #3406929 by alexpott, quietone, bircher, borisson_, Wim Leers: Configuration being imported by the ConfigImporter sometimes has stale original data

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();
   }
 
 }

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();
+  }
+
 }

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.
    *

core/modules/config/tests/config_test/config_test.install

@@ -0,0 +1,19 @@
+<?php
+
+/**
+ * @file
+ * Install, update and uninstall functions for the config_test module.
+ */
+
+/**
+ * Implements hook_install().
+ */
+function config_test_install($is_syncing): void {
+  $secondary_write = \Drupal::state()->get('config_test_install.foo_value');
+  if ($secondary_write !== NULL) {
+    \Drupal::configFactory()
+      ->getEditable('config_test.system')
+      ->set('foo', $secondary_write)
+      ->save();
+  }
+}

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.
    *

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;
   }
 
   /**