Issue #2177739 by Berdir, alexpott, Gábor Hojtsy: Fix inefficient config factory caching.

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

@@ -68,22 +68,14 @@ public function exists($name) {
    */
   public function read($name) {
     if ($cache = $this->cache->get($name)) {
-      // The cache backend supports primitive data types, but only an array
-      // represents valid config object data.
-      if (is_array($cache->data)) {
-        return $cache->data;
-      }
+      // The cache contains either the cached configuration data or FALSE
+      // if the configuration file does not exist.
+      return $cache->data;
     }
-    // Read from the storage on a cache miss and cache the data, if any.
+    // Read from the storage on a cache miss and cache the data. Also cache
+    // information about missing configuration objects.
     $data = $this->storage->read($name);
-    if ($data !== FALSE) {
-      $this->cache->set($name, $data, Cache::PERMANENT);
-    }
-    // If the cache contained bogus data and there is no data in the storage,
-    // wipe the cache entry.
-    elseif ($cache) {
-      $this->cache->delete($name);
-    }
+    $this->cache->set($name, $data);
     return $data;
   }
 
@@ -99,9 +91,10 @@ public function readMultiple(array $names) {
 
     if (!empty($names)) {
       $list = $this->storage->readMultiple($names);
-      // Cache configuration objects that were loaded from the storage.
-      foreach ($list as $name => $data) {
-        $this->cache->set($name, $data, Cache::PERMANENT);
+      // Cache configuration objects that were loaded from the storage, cache
+      // missing configuration objects as an explicit FALSE.
+      foreach ($names as $name) {
+        $this->cache->set($name, isset($list[$name]) ? $list[$name] : FALSE);
       }
     }
 
@@ -110,7 +103,9 @@ public function readMultiple(array $names) {
       $list[$name] = $cache->data;
     }
 
-    return $list;
+    // Ensure that only existing configuration objects are returned, filter out
+    // cached information about missing objects.
+    return array_filter($list);
   }
 
   /**
@@ -120,7 +115,7 @@ public function write($name, array $data) {
     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, Cache::PERMANENT);
+      $this->cache->set($name, $data);
       Cache::deleteTags(array($this::FIND_BY_PREFIX_CACHE_TAG => TRUE));
       $this->findByPrefixCache = array();
       return TRUE;

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

@@ -67,7 +67,7 @@ class Config {
    *
    * @var array
    */
-  protected $data;
+  protected $data = array();
 
   /**
    * The original data of the configuration object.
@@ -107,13 +107,6 @@ class Config {
    */
   protected $storage;
 
-  /**
-   * Whether the configuration object has already been loaded.
-   *
-   * @var bool
-   */
-  protected $isLoaded = FALSE;
-
   /**
    * The config schema wrapper object for this configuration object.
    *
@@ -161,7 +154,6 @@ public function __construct($name, StorageInterface $storage, EventDispatcherInt
    *   The configuration object.
    */
   public function initWithData(array $data) {
-    $this->isLoaded = TRUE;
     $this->settingsOverrides = array();
     $this->languageOverrides = array();
     $this->moduleOverrides = array();
@@ -236,9 +228,6 @@ public static function validateName($name) {
    *   TRUE if this configuration object does not exist in storage.
    */
   public function isNew() {
-    if (!$this->isLoaded) {
-      $this->load();
-    }
     return $this->isNew;
   }
 
@@ -263,9 +252,6 @@ public function isNew() {
    *   The data that was requested.
    */
   public function get($key = '') {
-    if (!$this->isLoaded) {
-      $this->load();
-    }
     if (!isset($this->overriddenData)) {
       $this->setOverriddenData();
     }
@@ -295,8 +281,6 @@ public function get($key = '') {
    */
   public function setData(array $data) {
     $this->replaceData($data);
-    // A load would destroy the data just set (for example on import).
-    $this->isLoaded = TRUE;
     return $this;
   }
 
@@ -417,10 +401,6 @@ protected function resetOverriddenData() {
    *   The configuration object.
    */
   public function set($key, $value) {
-    if (!$this->isLoaded) {
-      $this->load();
-    }
-
     // The dot/period is a reserved character; it may appear between keys, but
     // not within keys.
     $parts = explode('.', $key);
@@ -444,9 +424,6 @@ public function set($key, $value) {
    *   The configuration object.
    */
   public function clear($key) {
-    if (!$this->isLoaded) {
-      $this->load();
-    }
     $parts = explode('.', $key);
     if (count($parts) == 1) {
       unset($this->data[$key]);
@@ -458,28 +435,6 @@ public function clear($key) {
     return $this;
   }
 
-  /**
-   * Loads configuration data into this object.
-   *
-   * @return \Drupal\Core\Config\Config
-   *   The configuration object.
-   */
-  public function load() {
-    $this->isLoaded = FALSE;
-    $data = $this->storage->read($this->name);
-    if ($data === FALSE) {
-      $this->isNew = TRUE;
-      $this->replaceData(array());
-    }
-    else {
-      $this->isNew = FALSE;
-      $this->replaceData($data);
-    }
-    $this->originalData = $this->data;
-    $this->isLoaded = TRUE;
-    return $this;
-  }
-
   /**
    * Saves the configuration object.
    *
@@ -500,9 +455,6 @@ public function save() {
       }
     }
 
-    if (!$this->isLoaded) {
-      $this->load();
-    }
     $this->storage->write($this->name, $this->data);
     $this->isNew = FALSE;
     $this->notify('save');
@@ -557,9 +509,6 @@ protected function notify($config_event_name) {
    *   The configuration object.
    */
   public function merge(array $data_to_merge) {
-    if (!$this->isLoaded) {
-      $this->load();
-    }
     // Preserve integer keys so that configuration keys are not changed.
     $this->replaceData(NestedArray::mergeDeepArray(array($this->data, $data_to_merge), TRUE));
     return $this;
@@ -661,9 +610,6 @@ public function getLanguage() {
    *   The raw data.
    */
   public function getRawData() {
-    if (!$this->isLoaded) {
-      $this->load();
-    }
     return $this->data;
   }
 
@@ -685,13 +631,9 @@ public function getRawData() {
    *   The data that was requested.
    */
   public function getOriginal($key = '', $apply_overrides = TRUE) {
-    if (!$this->isLoaded) {
-      $this->load();
-    }
-
+    $original_data = $this->originalData;
     if ($apply_overrides) {
       // Apply overrides.
-      $original_data = $this->originalData;
       if (isset($this->languageOverrides) && is_array($this->languageOverrides)) {
         $original_data = NestedArray::mergeDeepArray(array($original_data, $this->languageOverrides), TRUE);
       }

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

@@ -264,7 +264,9 @@ public function rename($old_name, $new_name) {
 
     $new_cache_key = $this->getCacheKey($new_name);
     $this->cache[$new_cache_key] = new Config($new_name, $this->storage, $this->eventDispatcher, $this->typedConfigManager, $this->language);
-    $this->cache[$new_cache_key]->load();
+    if ($data = $this->storage->read($new_name)) {
+      $this->cache[$new_cache_key]->initWithData($data);
+    }
     return $this->cache[$new_cache_key];
   }
 

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

@@ -296,7 +296,9 @@ protected function importInvokeOwner() {
         // Config::validateName($name);
         if ($entity_type = config_get_entity_type_by_name($name)) {
           $old_config = new Config($name, $this->storageComparer->getTargetStorage(), $this->eventDispatcher, $this->typedConfigManager);
-          $old_config->load();
+          if ($old_data = $this->storageComparer->getTargetStorage()->read($name)) {
+            $old_config->initWithData($old_data);
+          }
 
           $data = $this->storageComparer->getSourceStorage()->read($name);
           $new_config = new Config($name, $this->storageComparer->getTargetStorage(), $this->eventDispatcher, $this->typedConfigManager);

core/lib/Drupal/Core/DrupalKernel.php

@@ -417,7 +417,7 @@ protected function initializeContainer() {
 
       // If 'container.modules' is wrong, the container must be rebuilt.
       if (!isset($this->moduleList)) {
-        $this->moduleList = $this->container->get('config.factory')->get('system.module')->load()->get('enabled');
+        $this->moduleList = $this->container->get('config.factory')->get('system.module')->get('enabled');
       }
       if (array_keys($this->moduleList) !== array_keys($container_modules)) {
         $persist = $this->getServicesToPersist();

core/modules/rest/lib/Drupal/rest/Routing/ResourceRoutes.php

@@ -62,7 +62,7 @@ public static function create(ContainerInterface $container) {
    */
   public function routes() {
     $routes = array();
-    $enabled_resources = $this->config->get('rest.settings')->load()->get('resources');
+    $enabled_resources = $this->config->get('rest.settings')->get('resources');
 
     // Iterate over all enabled resource plugins.
     foreach ($enabled_resources as $id => $enabled_methods) {

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

@@ -66,11 +66,11 @@ public function testGetMultipleOnPrimedCache() {
       'baz.back',
     );
     $configCacheValues = array(
-      'foo.bar' => (object) array(
-        'data' => array('foo' => 'bar'),
+      'foo.bar' => array(
+        'foo' => 'bar',
       ),
-      'baz.back' => (object) array(
-        'data' => array('foo' => 'bar'),
+      'baz.back' => array(
+        'foo' => 'bar',
       ),
     );
     $storage = $this->getMock('Drupal\Core\Config\StorageInterface');
@@ -84,35 +84,87 @@ public function testGetMultipleOnPrimedCache() {
   }
 
   /**
-   * Test fall through to file storage on a cache miss.
+   * Test fall through to file storage in CachedStorage::readMulitple().
    */
   public function testGetMultipleOnPartiallyPrimedCache() {
     $configNames = array(
       'foo.bar',
       'baz.back',
-      $this->randomName() . '. ' . $this->randomName(),
+      'config.exists_not_cached',
+      'config.does_not_exist_cached',
+      'config.does_not_exist',
     );
     $configCacheValues = array(
-      'foo.bar' => (object) array(
-        'data' => array('foo' => 'bar'),
+      'foo.bar' => array(
+        'foo' => 'bar',
       ),
-      'baz.back' => (object) array(
-        'data' => array('foo' => 'bar'),
+      'baz.back' => array(
+        'foo' => 'bar',
       ),
     );
     $cache = new MemoryBackend(__FUNCTION__);
     foreach ($configCacheValues as $key => $value) {
       $cache->set($key, $value);
     }
+    $cache->set('config.does_not_exist_cached', FALSE);
 
-    $response = array($configNames[2] => array($this->randomName()));
+    $config_exists_not_cached_data = array('foo' => 'bar');
+    $response = array(
+      $configNames[2] => $config_exists_not_cached_data,
+      $configNames[4] => FALSE,
+    );
     $storage = $this->getMock('Drupal\Core\Config\StorageInterface');
     $storage->expects($this->once())
       ->method('readMultiple')
-      ->with(array(2 => $configNames[2]))
+      ->with(array(2 => $configNames[2], 4 => $configNames[4]))
       ->will($this->returnValue($response));
 
     $cachedStorage = new CachedStorage($storage, $cache);
-    $this->assertEquals($configCacheValues + $response, $cachedStorage->readMultiple($configNames));
+    $expected_data = $configCacheValues + array($configNames[2] => $config_exists_not_cached_data);
+    $this->assertEquals($expected_data, $cachedStorage->readMultiple($configNames));
+
+    // Ensure that the a missing file is cached.
+    $entry = $cache->get('config.does_not_exist');
+    $this->assertFalse($entry->data);
+
+    // Ensure that the a file containing data is cached.
+    $entry = $cache->get('config.exists_not_cached');
+    $this->assertEquals($config_exists_not_cached_data, $entry->data);
+  }
+
+  /**
+   * Test fall through to file storage on a cache miss in CachedStorage::read().
+   */
+  public function testReadNonExistentFileCacheMiss() {
+    $name = 'config.does_not_exist';
+    $cache = new MemoryBackend(__FUNCTION__);
+    $storage = $this->getMock('Drupal\Core\Config\StorageInterface');
+    $storage->expects($this->once())
+            ->method('read')
+            ->with($name)
+            ->will($this->returnValue(FALSE));
+    $cachedStorage = new CachedStorage($storage, $cache);
+
+    $this->assertFalse($cachedStorage->read($name));
+
+    // Ensure that the a missing file is cached.
+    $entry = $cache->get('config.does_not_exist');
+    $this->assertFalse($entry->data);
+  }
+
+  /**
+   * Test file storage on a cache hit in CachedStorage::read().
+   */
+  public function testReadNonExistentFileCached() {
+    $name = 'config.does_not_exist';
+    $cache = new MemoryBackend(__FUNCTION__);
+    $cache->set($name, FALSE);
+
+    $storage = $this->getMock('Drupal\Core\Config\StorageInterface');
+    $storage->expects($this->never())
+            ->method('read');
+    $cachedStorage = new CachedStorage($storage, $cache);
+    $this->assertFalse($cachedStorage->read($name));
   }
+
 }