Issue #2326203 by effulgentsia, alexpott: Fixed Config's cached storage should only use one bin.

core/core.services.yml

@@ -51,6 +51,13 @@ services:
     factory_method: get
     factory_service: cache_factory
     arguments: [bootstrap]
+  cache.config:
+    class: Drupal\Core\Cache\CacheBackendInterface
+    tags:
+      - { name: cache.bin }
+    factory_method: get
+    factory_service: cache_factory
+    arguments: [config]
   cache.default:
     class: Drupal\Core\Cache\CacheBackendInterface
     tags:

core/lib/Drupal/Core/Config/CachedStorage.php

@@ -8,12 +8,12 @@
 namespace Drupal\Core\Config;
 
 use Drupal\Core\Cache\Cache;
-use Drupal\Core\Cache\CacheFactoryInterface;
+use Drupal\Core\Cache\CacheBackendInterface;
 
 /**
  * Defines the cached storage.
  *
- * The class gets another storage and the cache factory injected. It reads from
+ * The class gets another storage and a cache backend injected. It reads from
  * the cache and delegates the read to the storage on a cache miss. It also
  * handles cache invalidation.
  */
@@ -26,13 +26,6 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
    */
   protected $storage;
 
-  /**
-   * The cache factory.
-   *
-   * @var \Drupal\Core\Cache\CacheFactoryInterface
-   */
-  protected $cacheFactory;
-
   /**
    * The instantiated Cache backend.
    *
@@ -52,20 +45,12 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
    *
    * @param \Drupal\Core\Config\StorageInterface $storage
    *   A configuration storage to be cached.
-   * @param \Drupal\Core\Cache\CacheFactoryInterface $cache_factory
+   * @param \Drupal\Core\Cache\CacheBackendInterface $cache
-   *   A cache factory used for getting cache backends.
+   *   A cache backend used to store configuration.
    */
-  public function __construct(StorageInterface $storage, CacheFactoryInterface $cache_factory) {
+  public function __construct(StorageInterface $storage, CacheBackendInterface $cache) {
     $this->storage = $storage;
-    $this->cacheFactory = $cache_factory;
+    $this->cache = $cache;
-    $collection = $this->getCollectionName();
-    if ($collection == StorageInterface::DEFAULT_COLLECTION) {
-      $bin = 'config';
-    }
-    else {
-      $bin = 'config_' . str_replace('.', '_', $collection);
-    }
-    $this->cache = $this->cacheFactory->get($bin);
   }
 
   /**
@@ -82,7 +67,8 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
    * Implements Drupal\Core\Config\StorageInterface::read().
    */
   public function read($name) {
-    if ($cache = $this->cache->get($name)) {
+    $cache_key = $this->getCacheKey($name);
+    if ($cache = $this->cache->get($cache_key)) {
       // The cache contains either the cached configuration data or FALSE
       // if the configuration file does not exist.
       return $cache->data;
@@ -90,7 +76,7 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
     // Read from the storage on a cache miss and cache the data. Also cache
     // information about missing configuration objects.
     $data = $this->storage->read($name);
-    $this->cache->set($name, $data);
+    $this->cache->set($cache_key, $data);
     return $data;
   }
 
@@ -98,32 +84,41 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
    * {@inheritdoc}
    */
   public function readMultiple(array $names) {
-    $list = array();
+    $data_to_return = array();
-    // The names array is passed by reference and will only contain the names of
-    // config object not found after the method call.
-    // @see \Drupal\Core\Cache\CacheBackendInterface::getMultiple()
-    $cached_list = $this->cache->getMultiple($names);
 
-    if (!empty($names)) {
+    $cache_keys_map = $this->getCacheKeys($names);
-      $list = $this->storage->readMultiple($names);
+    $cache_keys = array_values($cache_keys_map);
+    $cached_list = $this->cache->getMultiple($cache_keys);
+
+    if (!empty($cache_keys)) {
+      // $cache_keys_map contains the full $name => $cache_key map, while
+      // $cache_keys contains just the $cache_key values that weren't found in
+      // the cache.
+      // @see \Drupal\Core\Cache\CacheBackendInterface::getMultiple()
+      $names_to_get = array_keys(array_intersect($cache_keys_map, $cache_keys));
+      $list = $this->storage->readMultiple($names_to_get);
       // Cache configuration objects that were loaded from the storage, cache
       // missing configuration objects as an explicit FALSE.
       $items = array();
-      foreach ($names as $name) {
+      foreach ($names_to_get as $name) {
-        $items[$name] = array('data' => isset($list[$name]) ? $list[$name] : FALSE);
+        $data = isset($list[$name]) ? $list[$name] : FALSE;
+        $data_to_return[$name] = $data;
+        $items[$cache_keys_map[$name]] = array('data' => $data);
       }
 
       $this->cache->setMultiple($items);
     }
 
     // Add the configuration objects from the cache to the list.
-    foreach ($cached_list as $name => $cache) {
+    $cache_keys_inverse_map = array_flip($cache_keys_map);
-      $list[$name] = $cache->data;
+    foreach ($cached_list as $cache_key => $cache) {
+      $name = $cache_keys_inverse_map[$cache_key];
+      $data_to_return[$name] = $cache->data;
     }
 
     // Ensure that only existing configuration objects are returned, filter out
     // cached information about missing objects.
-    return array_filter($list);
+    return array_filter($data_to_return);
   }
 
   /**
@@ -133,7 +128,7 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
     if ($this->storage->write($name, $data)) {
       // While not all written data is read back, setting the cache instead of
       // just deleting it avoids cache rebuild stampedes.
-      $this->cache->set($name, $data);
+      $this->cache->set($this->getCacheKey($name), $data);
       Cache::deleteTags(array($this::FIND_BY_PREFIX_CACHE_TAG => TRUE));
       $this->findByPrefixCache = array();
       return TRUE;
@@ -148,7 +143,7 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
     // If the cache was the first to be deleted, another process might start
     // rebuilding the cache before the storage is gone.
     if ($this->storage->delete($name)) {
-      $this->cache->delete($name);
+      $this->cache->delete($this->getCacheKey($name));
       Cache::deleteTags(array($this::FIND_BY_PREFIX_CACHE_TAG => TRUE));
       $this->findByPrefixCache = array();
       return TRUE;
@@ -163,8 +158,8 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
     // If the cache was the first to be deleted, another process might start
     // rebuilding the cache before the storage is renamed.
     if ($this->storage->rename($name, $new_name)) {
-      $this->cache->delete($name);
+      $this->cache->delete($this->getCacheKey($name));
-      $this->cache->delete($new_name);
+      $this->cache->delete($this->getCacheKey($new_name));
       Cache::deleteTags(array($this::FIND_BY_PREFIX_CACHE_TAG => TRUE));
       $this->findByPrefixCache = array();
       return TRUE;
@@ -214,23 +209,24 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
    *   An array containing matching configuration object names.
    */
   protected function findByPrefix($prefix) {
-    if (!isset($this->findByPrefixCache[$prefix])) {
+    $cache_key = $this->getCacheKey($prefix);
+    if (!isset($this->findByPrefixCache[$cache_key])) {
       // The : character is not allowed in config file names, so this can not
       // conflict.
-      if ($cache = $this->cache->get('find:' . $prefix)) {
+      if ($cache = $this->cache->get('find:' . $cache_key)) {
-        $this->findByPrefixCache[$prefix] = $cache->data;
+        $this->findByPrefixCache[$cache_key] = $cache->data;
       }
       else {
-        $this->findByPrefixCache[$prefix] = $this->storage->listAll($prefix);
+        $this->findByPrefixCache[$cache_key] = $this->storage->listAll($prefix);
         $this->cache->set(
-          'find:' . $prefix,
+          'find:' . $cache_key,
-          $this->findByPrefixCache[$prefix],
+          $this->findByPrefixCache[$cache_key],
           Cache::PERMANENT,
           array($this::FIND_BY_PREFIX_CACHE_TAG => TRUE)
         );
       }
     }
-    return $this->findByPrefixCache[$prefix];
+    return $this->findByPrefixCache[$cache_key];
   }
 
   /**
@@ -239,9 +235,9 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
   public function deleteAll($prefix = '') {
     // If the cache was the first to be deleted, another process might start
     // rebuilding the cache before the storage is renamed.
-    $cids = $this->storage->listAll($prefix);
+    $names = $this->storage->listAll($prefix);
     if ($this->storage->deleteAll($prefix)) {
-      $this->cache->deleteMultiple($cids);
+      $this->cache->deleteMultiple($this->getCacheKeys($names));
       return TRUE;
     }
     return FALSE;
@@ -260,7 +256,7 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
   public function createCollection($collection) {
     return new static(
       $this->storage->createCollection($collection),
-      $this->cacheFactory
+      $this->cache
     );
   }
 
@@ -278,4 +274,49 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
     return $this->storage->getCollectionName();
   }
 
+  /**
+   * Returns a cache key for a configuration name using the collection.
+   *
+   * @param string $name
+   *   The configuration name.
+   *
+   * @return string
+   *   The cache key for the configuration name.
+   */
+  protected function getCacheKey($name) {
+    return $this->getCollectionPrefix() . $name;
+  }
+
+  /**
+   * Returns a cache key map for an array of configuration names.
+   *
+   * @param array $names
+   *   The configuration names.
+   *
+   * @return array
+   *   An array of cache keys keyed by configuration names.
+   */
+  protected function getCacheKeys(array $names) {
+    $prefix = $this->getCollectionPrefix();
+    $cache_keys = array_map(function($name) use ($prefix) {
+      return $prefix . $name;
+    }, $names);
+
+    return array_combine($names, $cache_keys);
+  }
+
+  /**
+   * Returns a cache ID prefix to use for the collection.
+   *
+   * @return string
+   *   The cache ID prefix.
+   */
+  protected function getCollectionPrefix() {
+    $collection = $this->storage->getCollectionName();
+    if ($collection == StorageInterface::DEFAULT_COLLECTION) {
+      return '';
+    }
+    return $collection . ':';
+  }
+
 }

core/modules/config/src/Tests/Storage/CachedStorageTest.php

@@ -37,7 +37,7 @@ class CachedStorageTest extends ConfigStorageTestBase {
   protected function setUp() {
     parent::setUp();
     $this->filestorage = new FileStorage($this->configDirectories[CONFIG_ACTIVE_DIRECTORY]);
-    $this->storage = new CachedStorage($this->filestorage, \Drupal::service('cache_factory'));
+    $this->storage = new CachedStorage($this->filestorage, \Drupal::service('cache.config'));
     $this->cache = \Drupal::service('cache_factory')->get('config');
     // ::listAll() verifications require other configuration data to exist.
     $this->storage->write('system.performance', array());

core/tests/Drupal/Tests/Core/Config/CachedStorageTest.php

@@ -19,10 +19,6 @@ class CachedStorageTest extends UnitTestCase {
    */
   protected $cacheFactory;
 
-  protected function setUp() {
-    $this->cacheFactory = $this->getMock('Drupal\Core\Cache\CacheFactoryInterface');
-  }
-
   /**
    * Test listAll static cache.
    */
@@ -37,11 +33,8 @@ class CachedStorageTest extends UnitTestCase {
       ->will($this->returnValue($response));
 
     $cache = new NullBackend(__FUNCTION__);
-    $this->cacheFactory->expects($this->once())
+
-      ->method('get')
+    $cachedStorage = new CachedStorage($storage, $cache);
-      ->with('config')
-      ->will($this->returnValue($cache));
-    $cachedStorage = new CachedStorage($storage, $this->cacheFactory);
     $this->assertEquals($response, $cachedStorage->listAll($prefix));
     $this->assertEquals($response, $cachedStorage->listAll($prefix));
   }
@@ -57,11 +50,8 @@ class CachedStorageTest extends UnitTestCase {
     $response = array("$prefix." . $this->randomMachineName(), "$prefix." . $this->randomMachineName());
     $cache = new MemoryBackend(__FUNCTION__);
     $cache->set('find:' . $prefix, $response);
-    $this->cacheFactory->expects($this->once())
+
-      ->method('get')
+    $cachedStorage = new CachedStorage($storage, $cache);
-      ->with('config')
-      ->will($this->returnValue($cache));
-    $cachedStorage = new CachedStorage($storage, $this->cacheFactory);
     $this->assertEquals($response, $cachedStorage->listAll($prefix));
   }
 
@@ -87,11 +77,8 @@ class CachedStorageTest extends UnitTestCase {
     foreach ($configCacheValues as $key => $value) {
       $cache->set($key, $value);
     }
-    $this->cacheFactory->expects($this->once())
+
-      ->method('get')
+    $cachedStorage = new CachedStorage($storage, $cache);
-      ->with('config')
-      ->will($this->returnValue($cache));
-    $cachedStorage = new CachedStorage($storage, $this->cacheFactory);
     $this->assertEquals($configCacheValues, $cachedStorage->readMultiple($configNames));
   }
 
@@ -128,14 +115,10 @@ class CachedStorageTest extends UnitTestCase {
     $storage = $this->getMock('Drupal\Core\Config\StorageInterface');
     $storage->expects($this->once())
       ->method('readMultiple')
-      ->with(array(2 => $configNames[2], 4 => $configNames[4]))
+      ->with(array($configNames[2], $configNames[4]))
       ->will($this->returnValue($response));
 
-    $this->cacheFactory->expects($this->once())
+    $cachedStorage = new CachedStorage($storage, $cache);
-      ->method('get')
-      ->with('config')
-      ->will($this->returnValue($cache));
-    $cachedStorage = new CachedStorage($storage, $this->cacheFactory);
     $expected_data = $configCacheValues + array($configNames[2] => $config_exists_not_cached_data);
     $this->assertEquals($expected_data, $cachedStorage->readMultiple($configNames));
 
@@ -159,11 +142,8 @@ class CachedStorageTest extends UnitTestCase {
             ->method('read')
             ->with($name)
             ->will($this->returnValue(FALSE));
-    $this->cacheFactory->expects($this->once())
+
-      ->method('get')
+    $cachedStorage = new CachedStorage($storage, $cache);
-      ->with('config')
-      ->will($this->returnValue($cache));
-    $cachedStorage = new CachedStorage($storage, $this->cacheFactory);
 
     $this->assertFalse($cachedStorage->read($name));
 
@@ -183,11 +163,8 @@ class CachedStorageTest extends UnitTestCase {
     $storage = $this->getMock('Drupal\Core\Config\StorageInterface');
     $storage->expects($this->never())
             ->method('read');
-    $this->cacheFactory->expects($this->once())
+
-      ->method('get')
+    $cachedStorage = new CachedStorage($storage, $cache);
-      ->with('config')
-      ->will($this->returnValue($cache));
-    $cachedStorage = new CachedStorage($storage, $this->cacheFactory);
     $this->assertFalse($cachedStorage->read($name));
   }