Goal

  • Allow concurrent writing to multiple (configurable) storages.

Related issues

Notes

  • Critical, because this blocks the import/export mechanism.
Files: 
CommentFileSizeAuthor
#86 config.canonical-filestorage-exists-phpdoc.patch610 bytessun
PASSED: [[SimpleTest]]: [MySQL] 41,100 pass(es). View
#82 1702080_82.patch60.99 KBchx
PASSED: [[SimpleTest]]: [MySQL] 41,094 pass(es). View
#82 interdiff.txt1.86 KBchx
#81 1702080_80.patch60.72 KBchx
PASSED: [[SimpleTest]]: [MySQL] 41,094 pass(es). View
#81 interdiff.txt1.15 KBchx
#79 1702080_79.patch60.71 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#79 interdiff.txt1.17 KBchx
#74 config.canonical.74.patch59.72 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,092 pass(es). View
#74 interdiff.txt686 bytessun
#72 config.canonical.72.patch59.72 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,090 pass(es). View
#71 config.canonical.71.patch59.66 KBsun
PASSED: [[SimpleTest]]: [MySQL] 41,088 pass(es). View
#71 interdiff.txt8.32 KBsun
#68 1702080_68.patch58.99 KBchx
PASSED: [[SimpleTest]]: [MySQL] 41,078 pass(es). View
#60 interdiff.txt23.69 KBchx
#58 config.canonical.58.patch57.78 KBsun
PASSED: [[SimpleTest]]: [MySQL] 40,714 pass(es). View
#58 interdiff.txt2.3 KBsun
#57 config.canonical.57.patch57.75 KBsun
PASSED: [[SimpleTest]]: [MySQL] 40,714 pass(es). View
#57 interdiff.txt44.9 KBsun
#54 interdiff.txt44 KBsun
#53 interdiff.txt43.63 KBsun
#52 1702080_52.patch55.77 KBchx
PASSED: [[SimpleTest]]: [MySQL] 40,685 pass(es). View
#52 interdiff.txt1.83 KBchx
#51 1702080_51.patch55.64 KBchx
PASSED: [[SimpleTest]]: [MySQL] 40,690 pass(es). View
#51 interdiff.txt23.84 KBchx
#45 1702080_43.patch52.71 KBchx
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es). View
#43 1741804_43.patch23.73 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1741804_43_0.patch. Unable to apply patch. See the log in the details link for more information. View
#25 config.canonical.25.patch51.26 KBsun
PASSED: [[SimpleTest]]: [MySQL] 40,401 pass(es). View
#25 interdiff.txt691 bytessun
#22 config.canonical.22.patch50.58 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,116 pass(es), 242 fail(s), and 4 exception(s). View

Comments

sun’s picture

Title: Introduce MultiStorage as a replacement for removed StorageDispatcher » Introduce MultiStorage as a replacement for removed StorageDispatcher, and enable concurrent writing
sun’s picture

tim.plunkett’s picture

Status: Active » Postponed

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

sun’s picture

Category: task » feature
Priority: Critical » Major
sun’s picture

Status: Postponed » Closed (duplicate)
Dries’s picture

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

catch’s picture

Status: Closed (duplicate) » Active

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.

sun’s picture

Title: Introduce MultiStorage as a replacement for removed StorageDispatcher, and enable concurrent writing » Introduce canonical FileStorage + (regular) cache, and two-directory approach for staging configuration

Yep, I just wanted to do the same. Adjusting the issue title accordingly.

I've prepared the entire implementation in:

#1626584-105: Combine configuration system changes to verify they are compatible (and following; #112 contains working patch; linking to where I started, since I ran into lots of problems, particularly in the installer)

I will extract the relevant storage/import changes for this issue in a bit.

That said, I've already extracted a list of blockers:

#1730774: Untangle Cache\DatabaseBackend from procedural database.inc functions to make it available in early bootstrap
#1730770: Drupal installer cannot be fully tested in a separate sub/multi-site
#1730760: theme_task_list() does not use installer-compatible get_t()
#1730754: UpgradePathTestBase::performUpgrade() does not always load all new modules after upgrade

heyrocker’s picture

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.

sun’s picture

Oh, hm. I was under the impression that we sorta reached consensus on that already... (?)

heyrocker’s picture

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.

chx’s picture

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.

catch’s picture

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.

beejeebus’s picture

Title: Introduce canonical FileStorage + (regular) cache, and two-directory approach for staging configuration » Introduce canonical FileStorage + (regular) cache

updating title, as the two-directory import issue is over here: #1741804: Implement a config import/staging directory.

i didn't really grok #12, but i don't think its what we should go with.

i think we should get rid of any notion of an 'active' store entirely.

we have a config directory with files. in front of that, we have a cache. that's it.

heyrocker’s picture

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.

pounard’s picture

I agree with beejeebus here!

beejeebus’s picture

re #15 - great, i'll stop fussing about use of the word 'active' if that's what it means :-)

sorry if i misunderstood you chx!

chx’s picture

Agreed with #15.

webchick’s picture

sun’s picture

Status: Active » Postponed
webchick’s picture

Status: Postponed » Active

That was committed, moving back to active.

sun’s picture

Status: Active » Needs review
FileSize
50.58 KB
FAILED: [[SimpleTest]]: [MySQL] 40,116 pass(es), 242 fail(s), and 4 exception(s). View

Overall, I cannot repeat often enough how much more awesome it is to see your actual configuration in files. This change is literally the most awesome since sliced bread, so I'm really happy that everyone I've spoken to so far seems to want it to happen! :)

So here we go. The size of this patch needs explanation, so let me try really hard to do that:

The actual change from DatabaseStorage to FileStorage for the config's active store is actually pretty simple. The configuration system's architecture is fully prepared for pluggable storages already. The actual change boils down to ~10 lines of changing the config storage service definition in drupal_container(). That's a true YAY! (just for the record)

Likewise, tacking a (regular) cache backend onto the file storage is equally simple. The fact that we're basing the canonical storage on files does not matter at all though. What matters is that there is 1) a canonical storage, and 2) its contents are cached.

I strongly believe that contributed projects will attempt to advance on the ground-work we're laying with the configuration system for D8. Our goal should be to make it easy for people to experiment with alternative solutions, so we can learn and enhance the system for D9.

This is why I chose to go with an implementation that might look more abstracted than it needs to be, but essentially allows for mixing and mashing the built-in storage controllers more easily in novel ways. Essentially, there are three components:

  1. CachedFileStorage: The "final", actively used controller. As the name implies, it is a FileStorage that has a cache layer in front of it.

  2. FileStorage: The actual and regular FileStorage controller being used by the CachedFileStorage.

  3. CacheStorage: A config storage controller being used by CachedFileStorage, which bridges/forwards all calls to a specified backend of the Cache component.

The CachedFileStorage wraps the FileStorage and CacheStorage, and simply forwards all method calls to the appropriate controller(s). Each controller is only instantiated once, so in overall terms, all of this most probably sounds more complicated to you in this explanation than it actually is. ;)

Technically, the CacheStorage could even be used as a primary config storage controller by itself, but of course, you will agree that doesn't make much sense. :) The primary goal and underlying idea is to make it easy for alternative implementations to integrate with the Cache subsystem and also clarify which config operations are not supported (right now) by Cache backends.

An essential bottom line I want to draw here is that we do not want to force anyone into presuming and using files as the canonical storage. It might very well be that We're All Wrong™ and that some way smarter guys than us will show us how to implement alternative config storage implementations during D8.

Regardless of that, #1741804: Implement a config import/staging directory introduced two directories for maintaining the "active" configuration and the "staging" configuration. In order to remove any inherent knowledge about the actual storage engine/controller being used for each, it is an essential part of this patch to have both storages as services on the DI container.

The discussion on how to name the two built-in storage "bins" has been moved back into #1703168: [Meta] Ensure that configuration system functionality matches expected workflows for users and devs though. For now, I went with the most pragmatic service and storage names:

  1. config.storage.active: The active configuration storage. (CachedFileStorage with this patch; DatabaseStorage previously)
  2. config.storage.staging: The staging configuration storage. (unnamed previously, but commonly referred to as the "import/export directory")

I cannot stress enough on the topic to keep these things as pluggable as possible. For example, I'm actively playing with the idea of writing a GitStorage controller, which would actively work with a git repository (and possibly also files in a working directory) under the hood - potentially even introducing true and native revision support for configuration entities, which already on its own sounds way too exciting to not give it a shot...

Ultimately, I merely want to say that I spent quite some time with thinking through ways of how this technology could be leveraged by contrib + custom implementations in order to make more sense for all of us in the long run, beyond D8.

Anyway. I could probably shortcut and end this summary with a simple: "Installer. Fuck that shit." ;) Trust me, I wanted to create the smallest possible patch for this change. I really bumped from one major WTF into the next with it.

But for full disclosure, and also to explain the massive amount of changes that had to be applied all over the system, here's the short rundown:

  1. The installer cannot work with config in files, because there are no config file directories yet.
  2. The current installer only works, because we've implanted some special try/catch blocks into the database and cache subsystems that essentially account for the case when they're being used in a situation when the storage engine and/or storage bin (table) does not exist yet. Since we're currently using the DatabaseStorage controller in HEAD, we did not ever notice any of these special conditions yet.
    (see #1730774: Untangle Cache\DatabaseBackend from procedural database.inc functions to make it available in early bootstrap)
  3. When there are no config directories yet, we essentially need something similar to the config's NullBackend. But. Alas...
  4. The installer's requirements for configuration are actually very different: Instead of operating on "custom" configuration, the installer's configuration is either "hard-coded" default config, or overridden by the installation profile. The bottom line, however, is that configuration is actually read from diverse locations, which do not map to "a single storage" in our existing understanding.
    (Exact details for this should be figured out in a separate issue; this patch only makes sure to not introduce any regressions.)
  5. As soon as the config file directories exist, the installer has to use them. Otherwise, the installed config wouldn't get installed. This essentially means to rebuild the service container of the installer at the right time; i.e., directly after creating and verifying that the configuration directories exist.
  6. All of this also inherently ties into the testing framework, of course -- any test that is run (regardless of unit or web test) needs to have a fresh pair of config directories at hand. Which means that the service container has to be reset and rebuilt very early, when the new environment for a test is set up.
    (A related patch landed for WebTests recently, which primarily cared for resetting/rebooting the kernel for web tests, but that is not sufficient for all tests and the config services.)

Even more interestingly, switching to files revealed a lot of problems that were not exposed before. I did not fully understand why a range of test failures are only triggered and revealed with this change to a canonical file storage yet. But at the same time, I'm also happy that we're catching quite a range of pretty essential bugs with this change. (Tests have been adjusted + enhanced accordingly, which obviously add some additional bytes to this patch.)

Speaking of tests, I performed countless of installations, both in single-site and multi-site (subdirectory) environments, on this patch/branch in the recent past and can thus confirm that all of the (partially special) cases are working; e.g., with and without settings.php, with and without $config_directories being defined, existing and non-existing directories (whereas defined or not), etc.pp. (OMG) (Early versions of this branch/patch made the simpletest installer overwrite my actual live configuration of the parent site running the tests... nasty surprise -- just to round up the full disclosure ;))

FWIW, the code has been extracted from #1626584: Combine configuration system changes to verify they are compatible and lives in the CMI sandbox' config-canonical-1702080-sun branch.

Long story short:

This patch is feature-complete. It passes tests. It has been extensively tested manually. I personally consider it RTBC.

Status: Needs review » Needs work
Issue tags: -API change, -Configuration system

The last submitted patch, config.canonical.22.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Configuration system

#22: config.canonical.22.patch queued for re-testing.

sun’s picture

FileSize
691 bytes
51.26 KB
PASSED: [[SimpleTest]]: [MySQL] 40,401 pass(es). View

Oh. I'm terribly sorry. Blatant oversight on my half during extraction of changes from config-next branch.

Fixed stale DatabaseStorage in module_uninstall().

yched’s picture

Status: Needs review » Needs work

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.

yched’s picture

switching to NW was unintended

alexpott’s picture

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']);
chx’s picture

Status: Needs work » Needs review

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

chx’s picture

Status: Needs review » Needs work

Opsie sorry x-post

sun’s picture

Status: Needs work » Needs review

@yched:

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() ?

That's something we're currently discussing in #1764474: Make Cache interface and backends use the DIC and related issues. This code is using and following the current way of how the options and dependent services are specified for the config storage controllers. We should discuss that and adjust it to whatever new pattern we settle on in a separate follow-up issue.

@alexpott:

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

The error suppression was copied 1:1 from DatabaseStorage. I agree that the phpDoc isn't really correct, but I'd suggest to re-evaluate the entire error handling in storage controllers (and expectations) in a separate issue.

// @todo Should the config object be cached if it does not exist?

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?

Definitely not an error, since the current expectation for storage controllers is to just return FALSE on a "404". This is e.g. used and the case when creating a new config object via config('does.not.exist.yet')->set('foo', 'bar')->save();

The actual question here is whether the non-existing config object should be cached, so in case no new config object is being created and the same config name is requested again, then the cached FALSE would be returned. The only real difference is the lack of a filestat on a subsequent retrieval of the same, non-existing config object.

I consider that to be a rare edge-case though, so I think that we shouldn't cache the FALSE (as the code does already; i.e., so we just remove the @todo).

the intention here is to remove encode and decode from storageinterface. @todo should be removed as there is nothing todo here.

I'd prefer to keep them, since I'm actively tracking the @todos in the config system code. Either way, doesn't really matter, since we're going to remove them anyway in the end. ;) I hope we can ignore this detail.

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

Good point on debugging. I'll improve the exception messages.

If we're passing a new value [to isCurrentRevision()] shouldn't this function return it?

The adjusted EntityInterface documents that the function returns the previous value when a new one is passed. I will say that this part of the patch is meant to be a stop-gap fix only - we most likely want to split the function into separate a getter and setter. However, I'd like to leave the proper implementation for entity system maintainers to figure out in a separate follow-up issue.

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

No. Module updates must not call into hook_schema() implementations, because hook_schema() always represents the current schema only. If there is another schema change to the cache table schema later on, then this update would install the new schema already, which leads to a schema mismatch in the (other new) module update that attempts to update the schema.

katbailey’s picture

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.

alexpott’s picture

Status: Needs review » Needs work

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']);

sun’s picture

Status: Needs work » Needs review

I did a straight copy of the schema definition of the 'cache' table into the module update function. If you have a good visual diff tool, then that allows you to compare whether there are any differences between that copy and the one in system_schema(). I could probably have added a code comment there to explain the copied schema.

Anyway... Consider me happy if reviews are going to stay at this level ;)

beejeebus’s picture

overall, i like this patch! sun++

the only thing that doesn't sit right to me is the CacheStorage implementation and chaining calls. i'd rather we just have a configurable (including NULL) cache object (configurable via the DIC etc) in file storage.

i'm not sure what extra flexibility we get from the extra complexity in the patch - contrib can go ahead and override all of this anyway if we make sure it is setup via the DIC correctly.

having said that, i don't wish to block this patch on that at all. i'd prefer it to be simpler, but i'd be happy to see it land with the current design. (i see i have the opposite reaction to yched in #26 ;-) )

on do we cache non-existent files, i agree with sun that is purely a perf issue. so, lets not do it to start, remove the todo, and revisit later when we have actual data showing us that is a problem.

(historical point - we ended up caching misses in the registry after we found that saved us a lot of cycles.)

some nits:

+  // config_get_config_directory() throws an exception when there is a prepared
+  // settings.php that defines $config_directories already and the directories
+  // do not exist yet.

this comment isn't right, config_get_directory() throws an exception when the $type passed to it doesn't exist in $config_directories.

+    $db_data = $storage->read($name);

$db_data is no longer a good name for that variable.

chx’s picture

Status: Needs review » Needs work

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.

sun’s picture

The second part about injecting the CacheFactory (alas, still procedural incarnation in cache()) makes perfect sense to me. (That said, I need a proper CacheFactory to inject here, because a dependency on procedural code will cause major breakage down the line.)

However, the first part on "PhpStorageInterface" was a bit too cryptic, since there is no notion of that in this patch. Can you clarify how that relates to this patch?

chx’s picture

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

chx’s picture

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

sun’s picture

Alright. That sounds pretty much in line with what @beejeebus and me discussed earlier in IRC already. Give or take some injection details. Perfectly doable.

That said, pretty much everyone so far agreed that the question of how the actual storages are registered and the cache is injected is a very very low level internal detail - which can be happily revamped and adjusted under the hood at any time.

As said, I'm not married to the currently proposed implementation; the only part that is of importance in my mind is to retain a un-cached FileStorage on its own; for testability as well as pluggability. In terms of architectural pluggability, I'd also like to basically have a CachedStorage storage controller that allows me to swap out the underlying FileStorage with a GitStorage or whatever else, since the fundamental requirements for decorating the active store with a cache are always going to be the same (which isn't covered by the implementation in this patch). I don't think we're able to apply a real decorator pattern here (and I think that would be too much), but the idea appears to be very similar.

Since all of that is extremely internal, this begs the question of how much architectural reworking of the storage classes/services we actually want to do as part of this issue, or whether we want to leave that to a dedicated follow-up that can exclusively focus on that topic?

Just want to bring up that process question, as I gained the impression that everyone would like to see the essential change of this patch happening rather sooner than later, since we can still refactor the controllers + services later on.

I'm happy either way. Will incorporate feedback soonish (but won't mind an early commit ;)).

chx’s picture

Status: Needs work » Postponed

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.

sun’s picture

Status: Postponed » Needs review

Sorry, but that's BS. 80% of this patch are the required changes to make the rest of the system behave with a storage that doesn't tie into the Database subsystem. An in-depth explanation was provided in #22.

The essential change here is the usage of files as canonical storage. The cache being tacked onto that is secondary. We could even skip that part (temporarily), but that would likely cause a major slowdown in terms of testbot performance.

chx’s picture

FileSize
23.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1741804_43_0.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 1741804_43.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
52.71 KB
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es). View

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

chx’s picture

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

  Current patch Suggested naming
Uncached active dir config.storage.file config.storage.active
Cached active dir config.storage.active config.storage
Uncached staging dir config.storage.staging config.storage.staging

Status: Needs review » Needs work
Issue tags: -API change, -Configuration system

The last submitted patch, 1702080_43.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Configuration system

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

chx’s picture

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

alexpott’s picture

Status: Needs review » Needs work

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.

chx’s picture

Assigned: sun » chx
Status: Needs work » Needs review
FileSize
23.84 KB
55.64 KB
PASSED: [[SimpleTest]]: [MySQL] 40,690 pass(es). View

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.

chx’s picture

FileSize
1.83 KB
55.77 KB
PASSED: [[SimpleTest]]: [MySQL] 40,685 pass(es). View

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

sun’s picture

Assigned: chx » sun
Status: Needs review » Needs work
FileSize
43.63 KB

Erm, can we please respect the assigned property?

sun’s picture

FileSize
44 KB

Trying to make sense of what happened here.

chx’s picture

Category: feature » task
Status: Needs work » Needs review

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.

chx’s picture

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.

sun’s picture

Category: task » feature
FileSize
44.9 KB
57.75 KB
PASSED: [[SimpleTest]]: [MySQL] 40,714 pass(es). View

The latest patches contained a range flaws, which have been corrected in the config-canonical branch and attached patch:

[- Replaced CachedFileStorage with CachedStorage, and replaced storage options arrays with directly injected services.]
- Reverted invalid removal of DatabaseStorage.
- Fixed CachedFileStorage and CacheStorage were inappropriately merged into CachedStorage, destroying the essential caching logic.
- Fixed drupal_container() rebuild logic, calling code, and insufficient documentation for new $rebuild parameter.
- Fixed bogus __construct() phpDoc in all storage controllers.
- Updated DatabaseStorage for storage controller constructor changes.
- Fixed grouping of registered services in drupal_container().
- Fixed CacheFactory cannot depend on configuration, since config.storage depends on CacheFactory.
- Fixed bogus and missing code comments.
- Fixed tests should use $this->configDirectories provided by TestBase::prepareEnvironment().
- Renamed config.cachedstorage.cache service into cache.config.

Interdiff is against #25.

Also, reverting category back to feature, since this should not count against thresholds and block other changes from landing.

sun’s picture

FileSize
2.3 KB
57.78 KB
PASSED: [[SimpleTest]]: [MySQL] 40,714 pass(es). View

Final minor tweaks:

Fixed CachedStorage caches non-existent configuration data.
Fixed stale documentation in install_begin_request().

sun’s picture

Adding DI tag, so people focusing on that topic are aware of the additional changes in this patch.

chx’s picture

FileSize
23.69 KB

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.

chx’s picture

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review

The extra check exists, because I was able to prime a cached config object with bogus during my extensive testing of this patch, and it's generally not guaranteed that a cache backend returns a valid configuration data array.

The DatabaseStorage is to be kept, for the reasons stated in #22, and also because it is the only other storage controller implementation that we can reasonably provide in core and which can be tested.

chx’s picture

Status: Needs review » Reviewed & tested by the community

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.

webchick’s picture

Assigned: sun » heyrocker

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

chx’s picture

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.

catch’s picture

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()?

sun’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for the review @catch! :)

This patch no longer applies due to #1713564: Make Config\FileStorage instantiate Yaml\Dumper and Yaml\Parser only once. I'll re-roll/merge it later for that, and to incorporate the review issues. Ideally, I'd like to address any review issues of @heyrocker (if any) at the same time.

chx’s picture

Status: Needs work » Needs review
FileSize
58.99 KB
PASSED: [[SimpleTest]]: [MySQL] 41,078 pass(es). View

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.

heyrocker’s picture

Status: Needs review » Needs work

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.

sun’s picture

Assigned: heyrocker » sun
sun’s picture

Status: Needs work » Needs review
FileSize
8.32 KB
59.66 KB
PASSED: [[SimpleTest]]: [MySQL] 41,088 pass(es). View

Thanks for the reviews! :)

@catch:

To clarify once more on DatabaseStorage:

- We want the config storage to be swappable/pluggable.
- Removing DatabaseStorage would only leave FileStorage.
- We need it to validate that storage controllers can actually be swapped out.
- We need it to validate that we do not implement features into a (FileStorage) controller that cannot be supported by others.
- We need it to evaluate and validate that errors are handled in a proper way. (which is not the case yet and an entire issue of its own)

The DatabaseStorage code is already there and done. It is essential for validating and testing the architecture. After this very issue here, we're pretty much done with architectural changes to the config system (except of aforementioned error handling).

The reasoning for removing it is... what exactly? ;) I strongly object to removing it (neither now nor later), because of the reasons outlined above. It's the proof of concept for me — so if anyone comes and says that we said it was possible but it doesn't appear to be the case, then I can point that one to DatabaseStorage and say "look, I've no idea what you want, this alternative implementation here works to 100% and I have proof."

@heyrocker:

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

This nicely ties into what I mentioned above -- FileStorage will actually throw an exception already, because the Yaml\Parser throws one on invalid data. But the situation with what throws an exception (and which) and what does not is anything but consistent in storage controllers currently; we generally deferred that to "later." Whereas later is "now", after this patch landed. This is why I started to add @throws directives to the methods (which are not complete though, and alas, generally missing in this new CachedStorage controller).

On #1751592: Add install profile support to config system:

I gained a pretty solid understanding of the installer and install profile system through some other recent issues in the meantime and thus have pretty concrete plans for the InstallStorage and architectural support for config in install profiles. :) Planning to work on that after this issue here.

How do the isCurrentRevision() changes to the EntityInterface relate to this patch?

The isCurrentRevision() changes were originally part of #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) but got lost in later re-rolls of that patch. They are part of this patch, since manual verification of the generated configuration files showed a isCurrentRevision key in every configuration file. (ConfigStorageController currently introspects a ConfigEntity class to extract all public class properties, so as to put the values of only those properties into the config object that is being saved.)

In this patch:

Moved cache_get_backends() into CacheFactory::getBackends().
Fixed bogus config object cache is not invalidated if config object no longer exists in storage.
Clarified install_begin_request() docs on config_get_config_directory().
Fixed missing Connection::escapeTable() on table name in DatabaseStorage.

I think we're effectively back to RTBC here.

sun’s picture

FileSize
59.72 KB
PASSED: [[SimpleTest]]: [MySQL] 41,090 pass(es). View

d'oh, sorry, interdiff in #71 shows more than actually in the .patch, since I didn't commit the last changes. Fixed the patch, interdiff is #71.

chx’s picture

sun’s picture

FileSize
686 bytes
59.72 KB
PASSED: [[SimpleTest]]: [MySQL] 41,092 pass(es). View

Reviewed this once more myself.

Fixed stale @see in install_begin_request().

alexpott’s picture

+1 for rtbc! Looks fantastic

beejeebus’s picture

i'm ok with this RTBC.

i really don't like the DatabaseStorage stuff landing with this patch, but i see there's a follow up, so we can rip out the unnecessary code later.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

catch’s picture

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

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.17 KB
60.71 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1702080_79.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.15 KB
60.72 KB
PASSED: [[SimpleTest]]: [MySQL] 41,094 pass(es). View

I meant this.

chx’s picture

FileSize
1.86 KB
60.99 KB
PASSED: [[SimpleTest]]: [MySQL] 41,094 pass(es). View

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

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Let's get some review on this.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

sun’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
610 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,100 pass(es). View

Merging latest HEAD into config-next in #1626584: Combine configuration system changes to verify they are compatible revealed that one phpDoc fix got lost in re-rolls here.

Quick fix, so going straight to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Automatically closed -- issue fixed for 2 weeks with no activity.