Introduce canonical FileStorage + (regular) cache

Doesn't this imply that StorageDispatcher was removed? If so, postponed.

Committed #1671080: Remove StorageDispatcher to simplify configuration system on which this patch was blocked on.

I'm un-duplicating this, that other issue is a massive, massive meta-issue that only barely touches on this problem. If we want to read and/or write to more than one storage layer, and we already do that with both $conf and db/files albeit hardcoded, then switching that to a chained storage system similar to #1651206: Added a cache chain implementation is a relatively small change that would remove a lot of the assumptions from the system and mean any decisions made elsewhere can be implemented on top of it.

I think we need to separate the discussion of FileStorage + Cache, which may or may not happen and has no consensus, from the discussion of two-directory implementation and writing to multiple storages, which do, and on which the UI is completely stuck which is pretty much the highest priority thing we have at the moment.

It hasn't even really been discussed at all except between us. At the very least I would like to get the opinion of some of the people who architected the original solution (DavidStrauss, crell and chx) but it certainly merits an issue of its own and not being buried in the middle of an unrelated one.

I think both me and msonabbaum agree on this. Let me write down my understanding.

We have a canonical storage containing YAML files, this can go into git.

There is an active storage, also containing YAML files. You are commuting files between the two via explicit import and export operations. On import we create derivative tables and likely add derivative information. On export, we strip the derivative information out. For example, when importing fields you might want to add information based on the installed entity types and strip that on export.

This brings simplicity to the configuration system code as it only needs to ever read and write YAML.

Using the normal caching system brings speed. Perhaps we could use the CacheArray trick to avoid N gets for N config calls for even more speed.

fwiw I'm keen on this as well. On the CacheArray stuff I've wanted that since the original config patch landed but then I want it for most things.. We should have #1637478: Add a PHP array cache backend soon, once that's in we could add another cache backend which does the same trick as CacheArray but just implementing the cache interface since we don't need the fake array trick. That'd be aimed at config but we wouldn't need to bake any of it in.

That's what we're talking about. Import directory, files, cache. The reference to active store is my feeling that this preserves what I think is an essential feature of the active store - that it is known good. You can try to import a broken file, and it won't become live.

I agree with beejeebus here!

Agreed with #15.

#1741804: Implement a config import/staging directory has been committed. carry on! :)

That was committed, moving back to active.

Wow. Awesome work.

+++ b/core/includes/bootstrap.incundefined
@@ -2426,25 +2430,38 @@ function drupal_container(Container $reset = NULL) {
+    $container->setParameter('config.storage.active.options', array(
+      'Drupal\Core\Config\FileStorage' => array(
+        'directory' => config_get_config_directory(CONFIG_ACTIVE_DIRECTORY),
+      ),
+      'Drupal\Core\Config\CacheStorage' => array(
+        'backend' => 'Drupal\Core\Cache\DatabaseBackend',
+        'bin' => 'config',

This bit and the corresponding CachedFileStorage::__contruct() look a bit odd. I'm not too familiar with DIC practices yet, but I would have expected the two stores to be registered as their own config.storage.active.file & config.storage.active.cache entries in the DIC, and added to config.storage.active arguments as references The objects would then be instanciated by the DIC and injected into CachedFileStorage::__contruct() ?

Also, big +1 on an architecture that allows "mixing and mashing the built-in (or custom/contrib) storage controllers more easily in novel ways". I'm not sure at which point this pushes for 'storage controllers as Plugins'.

None of the above is worth delaying the patch, though. The 1st point, if applicable, can totally be adjusted in a followup, and the second point can be a separate philosophical discussion (what should be plugins, what should not). I think "should cache backends be plugins ?" derailed the Plugins API patch for a while.

switching to NW was unintended

Great patch... really looking forward to putting memcache / redis / riak / pick your favourite nosql tool in front of the config system.

A few nits...

+++ b/core/lib/Drupal/Core/Config/CacheStorage.phpundefined
@@ -0,0 +1,143 @@
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::decode().
+   *
+   * @throws ErrorException
+   *   unserialize() triggers E_NOTICE if the string cannot be unserialized.
+   */
+  public static function decode($raw) {
+    $data = @unserialize($raw);
+    return is_array($data) ? $data : FALSE;
+  }

Won't the @unserialise prevent the error being thrown.

+++ b/core/lib/Drupal/Core/Config/CachedFileStorage.phpundefined
@@ -0,0 +1,134 @@
+      // @todo Should the config object be cached if it does not exist?
+      if ($data !== FALSE) {
+        $this->storages['cache']->write($name, $data);

I think you've got it correct here. If the config doesn't exist then we can't cache it. The further question here is - is this actually an error?

+++ b/core/lib/Drupal/Core/Config/CachedFileStorage.phpundefined
@@ -0,0 +1,134 @@
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::encode().
+   *
+   * @todo Remove encode() from StorageInterface.
+   */
+  public static function encode($data) {
+    return $this->storages['file']->encode($data);
+  }

After chatting with @sun in IRC the intention here is to remove encode and decode from storageinterface. @todo should be removed as there is nothing todo here.

+++ b/core/lib/Drupal/Core/Config/CachedFileStorage.phpundefined
@@ -0,0 +1,134 @@
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::decode().
+   *
+   * @todo Remove decode() from StorageInterface.
+   */
+  public static function decode($raw) {
+    return $this->storages['file']->decode($raw);
+  }

As above

+++ b/core/lib/Drupal/Core/Config/InstallStorage.phpundefined
@@ -0,0 +1,105 @@
+  /**
+   * Overrides Drupal\Core\Config\FileStorage::write().
+   *
+   * @throws Drupal\Core\Config\StorageException
+   */
+  public function write($name, array $data) {
+    throw new StorageException('Write operations are not allowed.');
+  }
+
+  /**
+   * Overrides Drupal\Core\Config\FileStorage::delete().
+   *
+   * @throws Drupal\Core\Config\StorageException
+   */
+  public function delete($name) {
+    throw new StorageException('Write operations are not allowed.');
+  }
+
+  /**
+   * Overrides Drupal\Core\Config\FileStorage::rename().
+   *
+   * @throws Drupal\Core\Config\StorageException
+   */
+  public function rename($name, $new_name) {
+    throw new StorageException('Write operations are not allowed.');
+  }
+
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::listAll().
+   *
+   * @throws Drupal\Core\Config\StorageException
+   */
+  public function listAll($prefix = '') {
+    throw new StorageException('List operation is not supported.');
+  }

I think there is some inconsistency in language here... not supported vs not allowed and for debugging it might be nicer to use the actual operation over "Write operation".

+++ b/core/modules/entity/lib/Drupal/entity/StorableBase.phpundefined
@@ -280,7 +280,11 @@ public function getRevisionId() {
   /**
    * Implements Drupal\entity\StorableInterface::isCurrentRevision().
    */
-  public function isCurrentRevision() {
-    return $this->isCurrentRevision;
+  public function isCurrentRevision($new_value = NULL) {
+    $return = $this->isCurrentRevision;
+    if (isset($new_value)) {
+      $this->isCurrentRevision = (bool) $new_value;
+    }
+    return $return;
   }

This code looks odd. If we're passing a new value shouldn't this function return it?

+++ b/core/modules/system/system.installundefined
@@ -1567,33 +1547,63 @@ function system_update_8002() {
 /**
- * Adds {config} table for new Configuration system.
+ * Creates {cache_config} cache table for the new configuration system.
  */
 function system_update_8003() {
-  // @todo Temporary.
-  if (db_table_exists('config')) {
-    db_drop_table('config');
-  }
-  db_create_table('config', array(
-    'description' => 'Default active store for the configuration system.',
+  $schema['cache'] = array(
+    'description' => 'Generic cache table for caching things not separated out into their own tables. Contributed modules may also use this to store cached items.',
     'fields' => array(
-      'name' => array(
-        'description' => 'The identifier for the configuration entry, such as module.example (the name of the file, minus the file extension).',
+      'cid' => array(
+        'description' => 'Primary Key: Unique cache ID.',
         'type' => 'varchar',
         'length' => 255,
         'not null' => TRUE,
         'default' => '',
       ),
       'data' => array(
-        'description' => 'The raw data for this configuration entry.',
+        'description' => 'A collection of data to cache.',
         'type' => 'blob',
+        'not null' => FALSE,
+        'size' => 'big',
+      ),
+      'expire' => array(
+        'description' => 'A Unix timestamp indicating when the cache entry should expire, or 0 for never.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+      ),
+      'created' => array(
+        'description' => 'A Unix timestamp indicating when the cache entry was created.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
+      ),
+      'serialized' => array(
+        'description' => 'A flag to indicate whether content is serialized (1) or not (0).',
+        'type' => 'int',
+        'size' => 'small',
         'not null' => TRUE,
+        'default' => 0,
+      ),
+      'tags' => array(
+        'description' => 'Space-separated list of cache tags for this entry.',
+        'type' => 'text',
         'size' => 'big',
-        'translatable' => TRUE,
+        'not null' => FALSE,
+      ),
+      'checksum' => array(
+        'description' => 'The tag invalidation sum when this entry was saved.',
+        'type' => 'int',
+        'not null' => TRUE,
+        'default' => 0,
       ),
     ),
-    'primary key' => array('name'),
-  ));
+    'indexes' => array(
+      'expire' => array('expire'),
+    ),
+    'primary key' => array('cid'),
+  );
+  db_create_table('cache_config', $schema['cache']);
 }

Shouldn't this be replaced with so that we get a table that is exactly the same whether we upgrade or install (and it's much less code).

  $schema['cache_config'] = drupal_get_schema_unprocessed('system', 'cache');
  $schema['cache_config']['description'] = 'Cache table for configuration data.';
  db_create_table('cache_config', $schema['cache_config']);

I really like this. Thanks much. I will post a detailed review post-jetlag :)

Opsie sorry x-post

Re #27 and sun's response in #31 regarding using the DIC properly, the issue for that in relation to config storage is here: #1762388: Use Dependency Injection for FileStorage.

Good point on using hook_schema() in updates... never thought of it like that before :)

@sun: I'm totally with you on not caching the nonexistent config objects - in the same way that caching 404's is a bad idea.

So on system_update_8003() is the following correct?

+++ b/core/modules/system/system.installundefined
@@ -1567,33 +1547,63 @@ function system_update_8002() {
+  $schema['cache'] = array(

Should be 'cache_config'

+++ b/core/modules/system/system.installundefined
@@ -1567,33 +1547,63 @@ function system_update_8002() {
+    'description' =>  'Generic cache table for caching things not separated out into their own tables. Contributed modules may also use this to store cached items.',

should be 'Cache table for configuration data.'

+++ b/core/modules/system/system.installundefined
@@ -1567,33 +1547,63 @@ function system_update_8002() {
+  db_create_table('cache_config', $schema['cache']);

should be db_create_table('cache_config', $schema['cache_config']);

The fundamental problem with the patch is it violates the single responsibility principle. There should be a CacheStorage implementing PhpStorageInterface which gets another PhpStorageInterface injected and all CacheStorage does is cache / calls the injected real storage on cache fails (or in case of exists, just calls).

Also, the cache bin already determines the backend so there should be no need to know about the cache class. For example, when #1764474: Make Cache interface and backends use the DIC gets in, this patch simply breaks. If it'd be using the proper factory -- cache($bin) -- then it would stay working.

Ie just merge CachedFileStorage and CacheStorage while unhardwiring FileStorage. core/vendor/doctrine/common/lib/Doctrine/Common/Annotations/CachedReader.php while alien to us shows this delegation very well. Excerpts:

    public function __construct(Reader $reader, Cache $cache, $debug = false)
    {
        $this->delegate = $reader;
        $this->cache = $cache;


    public function getClassAnnotations(\ReflectionClass $class)
    {
        $cacheKey = $class->getName();

        if (isset($this->loadedAnnotations[$cacheKey])) {
            return $this->loadedAnnotations[$cacheKey];
        }

        if (false === ($annots = $this->fetchFromCache($cacheKey, $class))) {
            $annots = $this->delegate->getClassAnnotations($class);

That's even better as it even gets the cache object injected but that might need to be a todo on #1764474: Make Cache interface and backends use the DIC

Sorry I meant Drupal\Core\Config\StorageInterface. (I got the other one stuck in my head)

I can't see this being a minor implementation detail. The followup would be a 80%+ rewrite of this patch, how is that a followup? Let's get #1762388: Use Dependency Injection for FileStorage fixed first and then doing the proper injection is easy.

It doesn't worth debating. I have simply enrolled that issue into this one because it was a small change if we are getting rid of DatabaseStorage anyways and implemented the proper dependency injection based CachedStorage. Look at how pretty that class is.

Note that I needed to add another $this->container = drupal_container() into WebTestBase since the kernel changes the object (and not the object contents). Ran the Language upgrade test, Import configuration , checked install.

Oh dear. That's the wrong patch. Too many _43.patch files.

One thing I am unsure about is the naming of the config services:

#45: 1702080_43.patch queued for re-testing.

LanguageUpgradePathTest passes locally, both CLI and browser. Edit: and now it did on the bot too.

Thanks everyone for all the great work... I'm not submitting patches here so that once the patch is ready I can RTBC :)

So patch in #45 still has my minor nits.

• Exceptions in rename and delete in InstallStorage class should be more specific
• Comments from #33 on the creation of cache_config - especially the table description as at the moment this will be different if you've been through an upgrade or a fresh install.

And @beejeebus's 2 nit's from #35 need addressing too.

Additionally...

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -0,0 +1,121 @@
+class CachedStorage implements StorageInterface {
+
+  protected $options;
+
+  /**
+   * The instantiated Cache backend.
+   *
+   * @var Drupal\Core\Cache\CacheBackendInterface
+   */
+  protected $storage;
+
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::__construct().
+   */
+  public function __construct(StorageInterface $delegate, CacheBackendInterface $cache) {
+    $this->delegate = $delegate;
+    $this->cache = $cache;
+  }

Need to define and doxygen $delegate (from @chx in IRC)

+++ b/core/lib/Drupal/Core/Config/FileStorage.phpundefined
@@ -26,11 +26,11 @@ class FileStorage implements StorageInterface {
   /**
    * Implements Drupal\Core\Config\StorageInterface::__construct().
    */
-  public function __construct(array $options = array()) {
-    if (!isset($options['directory'])) {
-      $options['directory'] = config_get_config_directory();
+  public function __construct($directory = '') {
+    if (empty($directory)) {
+      $directory = config_get_config_directory();
     }
-    $this->options = $options;
+    $this->directory = $directory;

I'm not 100% convinced that we should be changing the FileStorage constructor's arguments. The use of array $options keeps it consistent across all config storage backends. OTOH: If we change this here then we need to replace $options in the new InstallStorage class. And when this patch lands we probably should create a followup to totally remove DatabaseStorage.

Addressed, I think, all. The constructor can't be an array due to how the DIC works. And, it doesn't need to be one.

We discussed naming of services with alexpott and arrived to the attached.

Obviously we can but as I have worked on it I thought better to reassign to myself. But, doesn't matter. Also, not sure why you set it to CNW, resetting that.

Because this was raised on IRC and surely will be in the following surely scathing comment:

I really don't know why we started to change the storage options and service injection in this issue, since that is 100% off-topic, but well, I'll correct it now

The reason for fixing the dependency injection is that the only way to write this properly is by writing the delegating cache class I written (whether that class works is another question but the tests didnt take longer than usual so either our YAML parsing is blisteringly fast or the caching actually works or the tests are so short the caching has no effect on them in which case we might want to consider a noncaching version for tests?). To inject a service from the DIC, however, we needed to get rid of the array of options. The Symfony DIC we have does not support $container->setParameter('something', array('foo' => new Reference('bar') because Reference merely holds a string and while setArgument does look for Reference classes and do the retrieval from the container it is a method of (ie. runs $this->get) setParameter does not do recursive resolving.

Making it more reviewable by attaching an interdiff to #52. There's no point in going back to #25. I have no idea why the removal of DatabaseStorage was "invalid" but that's a minor point and can be a followup.

The only problem I am seeing now is

      // The cache backend supports primitive data types, but only an array
      // represents valid config object data.
      if (is_array($cache->data)) {
        return $cache->data;
      }

care to explain why this is necessary? The only cache set we have is

    $data = $this->storage->read($name);
    if ($data !== FALSE) {
      $this->cache->set($name, $data, CacheBackendInterface::CACHE_PERMANENT, array('config' => array($name)));

and read() returns FALSE or an array so we store only an array. I didn't see a point in this, that's why I removed it and I still fail to see a point.

And, again, why are we keeping DatabaseStorage around when the table for it is dropped...?

The actual change from DatabaseStorage to FileStorage for the config's active store is actually pretty simple

config.storage.active: The active configuration storage. (CachedFileStorage with this patch; DatabaseStorage previously)

Since we're currently using the DatabaseStorage controller in HEAD, we did not ever notice any of these special conditions yet.

These are the mentions of DatabaseStorage in #22. It doesn't explain to me why do we want to keep it. There's hardly any point in keeping it around. If you still think it's useful for testing purposes (I don't see this) then please move it testing.

Of course one can manually write things into cache that break things. But then again you can also hand edit config files or the database to reach the same and we usually do not protect against these. I am surprised a bit but not enough to hold it up over any of these. Core committers can decide whether this a serious enough problem. Despite I written a significant portion of the current patch I know alexpott considered mine version ready and I consider sun's fixes OK so let's do this.

Let's get heyrocker's sign off on this.

One more note: - Fixed CacheFactory cannot depend on configuration, since config.storage depends on CacheFactory. This is not true, CacheFactory called cache_get_backends which called variable_get which does not rely on the config() system only on global $conf. Inling cache_get_backends() and variable_get() into CacheFactory does not help anything aside from a minuscule performance -- but then again it's not such a big issue to hold this one up with it.

Here's a review. Not changing status for now. Overall looks great, most of this is nitpicks.

+++ b/core/includes/bootstrap.incundefined
@@ -2404,47 +2404,62 @@ function drupal_get_bootstrap_phase() {
   // We do not use drupal_static() here because we do not have a mechanism by
   // which to reinitialize the stored objects, so a drupal_static_reset() call

Is that actually true any more now you can pass in a completely new container?

+++ b/core/includes/cache.incundefined
@@ -32,9 +34,7 @@ function cache($bin = 'cache') {
-    $cache_backends = cache_get_backends();
-    $class = isset($cache_backends[$bin]) ? $cache_backends[$bin] : $cache_backends['cache'];
-    $cache_objects[$bin] = new $class($bin);
+    $cache_objects[$bin] = CacheFactory::get($bin);

ooh fancy.

+++ b/core/includes/install.core.incundefined
@@ -272,6 +274,42 @@ function install_begin_request(&$install_state) {
+  // - The first call into drupal_container() will try to set up the regular
+  //   runtime configuration storage, using the CachedStorage. The storage
+  //   controller requires to pass in the active config directory name, but that

What's the problem with passing in a directory name that doesn't exist yet?

+++ b/core/lib/Drupal/Core/Cache/CacheFactory.phpundefined
@@ -0,0 +1,48 @@
+  static function get($bin) {
+    // @todo Improve how cache backend classes are defined. Cannot be
+    //   configuration, since e.g. the CachedStorage config storage controller
+    //   requires the definition in its constructor already.
+    global $conf;
+    $cache_backends = isset($conf['cache_classes']) ? $conf['cache_classes'] : array();

This is duplicating cache_get_backends() now. Could we not share this code between them somewhere?

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -0,0 +1,123 @@
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::exists().
+   */
+  public function exists($name) {
+    // A single storage lookup is faster than a cache lookup and possibly
+    // subsequent storage lookup.
+    return $this->storage->exists($name);

O RLY? Why's it faster?

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -0,0 +1,123 @@
+      // The cache backend supports primitive data types, but only an array
+      // represents valid config object data.
+      if (is_array($cache->data)) {
+        return $cache->data;
+      }

So if the cache data is corrupted for some reason, we're hoping the later set is going to clean it up. This feels fine while typing it so is probably OK.

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -0,0 +1,123 @@
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::write().
+   */
+  public function write($name, array $data) {
+    $this->cache->delete($name);
+    return $this->storage->write($name, $data);

Why delete and not set?

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.phpundefined
@@ -16,31 +17,49 @@
 class DatabaseStorage implements StorageInterface {
 
   /**
-   * Database connection options for this storage controller.
+   * The database connection.
    *
-   * - connection: The connection key to use.
-   * - target: The target on the connection to use.

I also don't know why this is being kept. Can we open a follow-up issue to discuss removing it before closing this one?

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.phpundefined
@@ -16,31 +17,49 @@
-  protected function getConnection() {
-    return Database::getConnection($this->options['target'], $this->options['connection']);
+  public function exists($name) {
+    return (bool) $this->connection->queryRange('SELECT 1 FROM {' . $this->table . '} WHERE name = :name', 0, 1, array(
+      ':name' => $name,
+    ), $this->options)->fetchField();

No db_escape_table()?

Sigh. I am sick of fighting. Really. Noone has any idea why to keep the DatabaseStorage and problems crop up with it so it is removed.

> Is that actually true any more now you can pass in a completely new container?

The comment is about not using drupal_static -- if we were to use drupal_static here then after drupal_static_reset we would have just the emergency container and not the bundles based one from the Kernel. Let's improve this documentation in #1772380: Improve drupal_container comments.

> ooh fancy.
> This is duplicating cache_get_backends() now. Could we not share this code between them somewhere?

Well, then let's move backends code into the factory. I did. It's not used anywhere outside of cache.inc which keeps shrinking...

> What's the problem with passing in a directory name that doesn't exist yet?

Improved this comment.

> So if the cache data is corrupted for some reason, we're hoping the later set is going to clean it up. T

Provided the set happens. I added a delete if it doesn't.

> Why delete and not set?

No idea. I used set. Restored.

I also found some fatal errors added into CachedStorage: the static encode and decode functions can not use $this... Let's add tests in #1772386: Add dedicated tests for Config\StorageInterface::encode() and ::decode(). However, the issue linked by sun changed them to non-static (hurray!) so the fix was easy.

I'm woefully uneducated on the DIC implementation in core as it stands, so I will rely on the expertise of the others who have reviewed this patch that this is a good idea.

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -0,0 +1,131 @@
+    // Read from the storage on a cache miss and cache the data, if any.
+    $data = $this->storage->read($name);
+    if ($data !== FALSE) {
+      $this->cache->set($name, $data, CacheBackendInterface::CACHE_PERMANENT, array('config' => array($name)));
+    }
+    elseif ($delete) {
+      // This data was already found bogus so let's get rid of it.
+      $this->cache->delete($name);
+    }

If there's bad data in the file, won't it just replicate back into the cache here? Seems like if we're going to make this check for bogus data here, we should also do it in FileStorage::read(). Might be worth an exception or at least a watchdog as well. Looking back I see chx thought this code was weird too. I'm trying to think through the possible things that could go wrong here. For instance, an invalid YAML file will never get here, it would get caught during import, and if you stick something in live without going through import then that's your problem IMO. The question is are there things that could get through the import process but are still worth erroring out over? I'm happy to throw this to followup.

+++ b/core/lib/Drupal/Core/Config/InstallStorage.phpundefined
@@ -0,0 +1,95 @@
+   * Determines the owner and path to the default configuration file of a
+   * requested config object name located in the installation profile, a module,
+   * or a theme (in this order).

This is a clever way to deal with the installer problem that I hadn't thought about. Also note that we actually aren't really supporting profiles right now, see #1751592: Add install profile support to config system which could really use someone who knows about how profiles work fleshing out the details

+++ b/core/lib/Drupal/Core/Config/InstallStorage.phpundefined
@@ -0,0 +1,95 @@
+   * @todo Improve this when figuring out how we want to handle configuration in
+   *   installation profiles. E.g., a config object actually has to be searched

Oh you noted this already :)

@@ -280,8 +280,12 @@ class Entity implements EntityInterface {
   /**
    * Implements Drupal\entity\EntityInterface::isCurrentRevision().
    */
-  public function isCurrentRevision() {
-    return $this->isCurrentRevision;
+  public function isCurrentRevision($new_value = NULL) {
+    $return = $this->isCurrentRevision;
+    if (isset($new_value)) {
+      $this->isCurrentRevision = (bool) $new_value;
+    }
+    return $return;
   }

How do the isCurrentRevision() changes to the EntityInterface relate to this patch? They seem completely unrelated and I'm not sure what they're doing here.

Ultimately I'm +1 to the architectural changes here and I agree with sun that this will open up some really cool potential. It's funny, about five years ago I had a very long phone call with sdboyer about Deploy and one of his grand visions was that storage and transport for Deploy would get abstracted and pluggable, so that he could use git as the storage and delivery mechanism for all his configuration changes. Now we're almost there :)

So basically, pending rerolls on the reviews above, lets ship this thing.

#1772682: Remove the variable_get inline in CacheFactory::getBackends #1772686: Move DatabaseStorage into testing

Edit: for completeness the other two spinoffs from #68 #1772380: Improve drupal_container comments #1772386: Add dedicated tests for Config\StorageInterface::encode() and ::decode()

+1 for rtbc! Looks fantastic

I've posted on the DatabaseStorage issue.

Everything looks good to me now, but I spotted a race condition. Could we get a quick re-roll?

+++ b/core/lib/Drupal/Core/Config/CachedStorage.phpundefined
@@ -0,0 +1,131 @@
+  public function write($name, array $data) {
+    // Not all data that is written is necessarily read; just invalidate the
+    // cache.
+    $this->cache->delete($name);
+    return $this->storage->write($name, $data);
+  }
+
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::delete().
+   */
+  public function delete($name) {
+    $this->cache->delete($name);
+    return $this->storage->delete($name);
+  }
+
+  /**
+   * Implements Drupal\Core\Config\StorageInterface::rename().
+   */
+  public function rename($name, $new_name) {
+    $this->cache->delete($name);
+    $this->cache->delete($new_name);
+    return $this->storage->rename($name, $new_name);

With all of these, the cache delete needs to happen after the storage/write delete. Otherwise there's a potential race condition where another process gets a cache miss and sets it while this one is writing it.

I'm not convinced about the 'not always read' comment for not writing - that means a higher chance of stampedes just to avoid a write, but stampedes are less of an issue here than the variable cache so meh.

Hmm while we're moving them around, we could just move those back to cache sets as well where it's appropriate.

Here is set back with comments and the delete race fixed.

I meant this.

Further discussion leads to this version: write, delete and rename all follows a stampede-avoiding pattern.

Let's get some review on this.

I'm rtbc'ing this based on the fact the #82 addresses Catch's comments from #77 and #78.

Plus Heyrocker (#69), Sun (#70), Beejeebus (#76) have all rtbc'd the patch previously.

In IRC @chx, @catch and I considered doing a cache->set() in Cachedstorage::rename() but due to the fact that the rename operation is only being used (atm) when a field is deleted and the possibilities setting a stale cache we decided to do the easy thing and invalidate both caches.

Committed/pushed to 8.x. I don't think we need a separate change notice apart from "we added CMI" at the moment.

Committed and pushed that follow-up to 8.x. Thanks!