Add concept of collections to config storages

Collections need to be explained both in the summary and in the patch :)

Postponing this on #2263287: Test the CachedStorage class using ConfigStorageTestBase since it does part of the same as here.

7: 2262861.6.patch queued for re-testing.

With #2263287: Test the CachedStorage class using ConfigStorageTestBase committed, this surely does not apply anymore.

Here we go, first review.

Didn't look closely at the tests and the config import/compare changes, but from what I've seen it's just "loop over collections" + "pass collection around" all over the place :)

1. +++ b/core/lib/Drupal/Core/Config/CachedStorage.php
   @@ -45,12 +52,20 @@ class CachedStorage implements StorageInterface, StorageCacheInterface {
   +      $bin = 'config_' . str_replace('.', '_', $collection);
   

   Is it documented somewhere what a valid collection name is? Can't we require [a-z_] only as valid characters?

   Edit: Ah, now I understand, you use . for namespacing/sub-collections, makes sense then. Maybe add a short inline comment?

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -257,16 +261,13 @@ protected function getEmptyExtensionsProcessedList() {
       *
   -   * @param array $ops
   -   *   The operations to check for changes. Defaults to all operations, i.e.
   -   *   array('delete', 'create', 'update', 'rename').
   -   *
       * @return bool
   

   We don't support partial changes, so there's no point in limiting the actions, so we can safely remove it.. not really related to this I think, but good cleanup ;)

3. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -57,10 +67,14 @@ public static function getFileExtension() {
   +    // Only create .htaccess file in root driectory.
   

   wondering if drierectory has something to do with Dries... :)

4. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -57,10 +67,14 @@ public static function getFileExtension() {
        if (!$success) {
   -      throw new StorageException("Failed to create config directory {$this->directory}");
   +      throw new StorageException("Failed to create config directory {$dir}");
   

   Minor: Wouldn't need {} anymore

5. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -142,12 +156,22 @@ public function write($name, array $data) {
        if (!$this->exists($name)) {
   -      if (!file_exists($this->directory)) {
   -        throw new StorageException($this->directory . '/ not found.');
   +      $dir = $this->getCollectionDirectory();
   +      if (!file_exists($dir)) {
   +        throw new StorageException($dir . '/ not found.');
          }
          return FALSE;
        }
   

   Existing code, but how could this happen if we ensure that the directory exists?

6. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -215,4 +240,70 @@ public function deleteAll($prefix = '') {
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * @param string $directory
   +   *   (optional) The directory to check for sub directories. This allows this
   +   *   function to be used recursively to discover all the collections in the
   +   *   storage. Defaults to an empty string which starts checking from the
   +   *   storage's root directory.
   +   */
   

   mixing @inheritdoc with additinal documentation isn't officially supported (and not supported by api.drupal.org). Maybe move the recursion to a protected helper method like discoverCollectionDirectories/Names() ?

7. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -215,4 +240,70 @@ public function deleteAll($prefix = '') {
   +        $collection = $fileinfo->getFilename();
   +        $sub_collections = $this->getAllCollectionNames($directory . '/' . $collection);
   +        if (empty($sub_collections)) {
   +          $collections[] = $collection;
   +        }
   +        else {
   +          foreach ($sub_collections as $sub_collection) {
   +            $collections[] = $collection . '.' . $sub_collection;
   +          }
   +        }
   

   This code isn't trivial to understand (the part about if it has sub-directories, we only use them, otherwise us the directory itself as collection).

   I think it's also not going to work for more than two levels, because if you have:

   a/b/c

   then, when it calls it it to iterate on b, it will only use b and c for the collection name, which would result in b.c, but it should really be a.b.c?

   I think you would need to pass only the relative directory path through, and then use that to build the collection name instead of just the current directory name.

   Once fixed, some inline documentation on what it is doing and why would help.

   I still didn't get to the documentation of the collection string, but this also means that it is not valid to mix collections and collection namespaces, if you use language.$langcode, you are not allowed to put something in a collection "language". Might be worth documenting if it's not already.

8. +++ b/core/lib/Drupal/Core/Config/NullStorage.php
   @@ -92,4 +92,26 @@ public function listAll($prefix = '') {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCollectionName() {
   +    return '';
   +  }
   

   Interesting, if support of collections is mandatory, should there be a way to have a collection on the null storage as well? Probably doesn't matter, just wondering what would be correct here...

9. +++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.php
   @@ -18,7 +18,7 @@
       */
   -  public function getSourceStorage();
   +  public function getSourceStorage($collection = StorageInterface::DEFAULT_COLLECTION);
    
   
   @@ -26,7 +26,7 @@ public function getSourceStorage();
       */
   -  public function getTargetStorage();
   +  public function getTargetStorage($collection = StorageInterface::DEFAULT_COLLECTION);
    
   

   I think we're missing documentation for $collection here?

10. +++ b/core/lib/Drupal/Core/Config/StorageInterface.php
    @@ -156,4 +161,46 @@ public function listAll($prefix = '');
    +   * @param string $collection
    +   *   The collection name. If not set then this is normal configuration
    +   *   storage. However if set the storage needs to use it to differentiate the
    +   *   collections. collections can be nested for example 'language.de'. If this
    +   *   is a file system then we could create a language folder with a subfolder
    +   *   named de. If this is a database table then there could be a collection
    +   *   column which will store the value 'language.de'.
    

    Here we are :)

    Yep, as mentioned above, should possibly mention that when you nest, then you can't use the parent anymore, looks good other than that.

11. +++ b/core/lib/Drupal/Core/Config/StorageInterface.php
    @@ -156,4 +161,46 @@ public function listAll($prefix = '');
    +   * @return \Drupal\Core\Config\StorageInterface
    +   *   An instance of the storage class. It will be cloned from the original
    +   *   class to avoid side effects caused by the fact that Config objects have
    +   *   their storage injected.
    

    The wording is a bit confusing.

    For someone using the method, it's not that relevant that it is cloned I think, it's more an instruction for implementations that they have to clone it, so maybe change it to "Implementations must clone themself.." or something?

    Also, it says instance of storage class, but maybe it would be more correct to say that it's an instance/version/something of that storage *backend* as it is going to be the same backend (class + configuration)?

I wanted to review this but Berdir's points are great :) So reviewed the issue summary instead. One of the points of #2224887: Language configuration overrides should have their own storage was that we need a new storage for language overrides, so we can skip assuming the things we assume about config, such as the forced casting on save. Once its the same storage again but a different collection, how will that assumption hold up then? Will these behaviours be collection specific?

Right, we don't use the storage to load something directly, we load it from upper level APIs which expose the data with the appropriate interfaces. It should be the task of those levels to expose the right collections with their appropriate behaviour, not the storage.

1. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -215,4 +240,97 @@ public function deleteAll($prefix = '') {
   +  protected function getAllCollectionNamesHelper($directory) {
   ...
   +        else {
   +          //
   +          $collections[] = $collection;
   +        }
   

   Helpful comment ;)

2. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -215,4 +240,97 @@ public function deleteAll($prefix = '') {
   +          // Build up the collection name be concatenating the subdirectory
   +          // names with the current directory name.
   +          foreach ($sub_collections as $sub_collection) {
   +            $collections[] = $collection . '.' . $sub_collection;
   +          }
   

   be = by?

So this patch shows the extent of changes needed to handle multiple parallel sources/storages of config, which was what we shied away when we implemented language overrides as files using config key prefixes so the API changes would not have been so extensive on all levels of config. I did not try implementing separate storages but I think the effect would be similar, one would need to be able to get different storages from a storage container or somesuch and iterate over them like this patch iterates over collections in one storage. So given that we want to store multiple things with the same config name, we need to differentiate on a different level, in essence the storage/collection level. The issue summary has several good reasons for this to be the case. The patch seems to be taking collections to all the places it needs and #2224887: Language configuration overrides should have their own storage is proof it works :) So I think some minor cleanups are in order and we can get this in :) Only found these by reading the whole patch top to bottom:

1. +++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
   @@ -38,6 +38,13 @@ class DatabaseStorage implements StorageInterface {
   +  protected $collection = '';
   
   @@ -46,11 +53,14 @@ class DatabaseStorage implements StorageInterface {
   +  public function __construct(Connection $connection, $table, array $options = array(), $collection = '') {
   
   @@ -263,10 +284,39 @@ public function deleteAll($prefix = '') {
   +    return $this->connection->query('SELECT DISTINCT collection FROM {' . $this->connection->escapeTable($this->table) . '} WHERE collection <> \'\' ORDER by collection')->fetchCol();
   
   +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -142,12 +156,22 @@ public function write($name, array $data) {
   +    if ($success && !empty($this->collection)) {
   
   @@ -215,4 +240,99 @@ public function deleteAll($prefix = '') {
   +    if (empty($this->collection)) {
   
   +++ b/core/lib/Drupal/Core/Config/NullStorage.php
   @@ -92,4 +92,26 @@ public function listAll($prefix = '') {
   +    return '';
   
   +++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.php
   @@ -106,4 +116,26 @@ public function moveRenameToUpdate($rename);
   +   * @param bool $include_default
   +   *   (optional) Include the default unnamed collection. Defaults to TRUE.
   
   +++ b/core/lib/Drupal/Core/Config/StorageInterface.php
   @@ -16,6 +16,11 @@
   +   * The default collection name.
   +   */
   +  const DEFAULT_COLLECTION = '';
   

   So the default collection is defined as an empty string and this constant is used extensively but there are still several places where the empty string or the emptyness is explicitly used as equivalent to the default collection as far as I can tell. Eg. docs says "default unnamed collection". Marked some, possible not all of them.

   This bakes in assumptions that the constant was supposed to avoid as far as I see.

2. +++ b/core/lib/Drupal/Core/Config/FileStorage.php
   @@ -215,4 +240,99 @@ public function deleteAll($prefix = '') {
   +  public function getAllCollectionNames($directory = '') {
   +    $collections = $this->getAllCollectionNamesHelper($this->directory);
   +    sort($collections);
   +    return $collections;
   +  }
   

   $directory is never used? Also this is from the general interface, which would not define $directory as an argument either?

   Looks like the helper is separated for recursion support and this $directory arg may be a prior version of the recursion implementation.

3. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -317,12 +382,22 @@ public function validateSiteUuid() {
   +    // Collections only support simple configuration therefore do not use
   +    // configuration dependencies.
   +    if ($this->supportsConfigurationEntities($collection)) {
   

   Not true for collections in general? Would be true for collections outside of the default collection now?

4. +++ b/core/lib/Drupal/Core/Config/StorageInterface.php
   @@ -156,4 +161,52 @@ public function listAll($prefix = '');
   +   *   An new instance of the storage backend with the collection set.
   

   An new

Hrm. To some very large extent, this effort appears to duplicate #2190723: Add a KeyValueStore\FileStorage to replace e.g. ConfigStorage :-(

Limited to major concerns:

(IETF/RFC notation just for clarity, not meant to yell)

1. Collection names MUST NOT have a concept of nested collections.

   The collection name is an arbitrary identifier that is passed into storages. We MAY make special use of the '/' character to denote some level of "namespacing" and the FileStorage MAY use that character literally for subdirectories. But otherwise, the $collection is just a string without meaning, which only controls the hashmap that a specific storage instance operates on.

2. A collection-specific storage instance MUST NOT be aware of any other collections.

   If such an introspection is required for any reason (that I do not understand), then that facility MAY live on the engine-specific factory, but not on the storage itself, because that instance is specific to a specific collection already.

This means to move the collection logic outside of the storage classes. Having that knowledge baked-in is a clear breach of concerns.

The caller has to know the collection name upfront. A storage itself is pure CRUD and I/O — you instantiate it with the right parameters, and the storage just simply operates on those. Mixing any additional logic into that is a recipe for a flawed architecture and thus, unrecoverable errors.

@sun: how would a caller know about the collections in an arbitrary collection that is needs to work with? Eg. config being staged to be synced? The storage would be the only point to ask about the collections within, no?

So this patch moves us from using parts of keys to collections with the same keys in config. Our original approach of config key prefixes was basic and this is a huge improvement in itself, while putting lots of the burden all across config. I think the biggest benefit of our original approach with the key prefixes was that the config API in general was not affected. It did not work with dependent config synching, exploded config keys, ran into key limitations, and so on. I think this collection system is a huge improvement over that and impacts the config API enough that we should not complicate this further if possible.

Since this is a hard-blocker for moving forward with #2224887: Language configuration overrides should have their own storage, proven to work well and got great reviews from berdir and myself, I think this looks good to go. Assigning to catch for core committer review :)

Gabor walked me through this patch. This is a solid step in the right direction and allows us to make progress. We can make it more "awesomer" (per sun's ideas) in follow-ups. Committed to 8.x. Thanks everyone.

Follow-up #2270917: Limit collection name due to MyISAM restrictions and test ConfigCollectionInfo class.

Late to this -interesting- thread, just wondering whether it would make sense to store config schema as a collection?
(Then flattening the folder structure in 'config' a little bit and simplifying file storage classes)

@reyero: I believe the point of the runtime configuration storage is for things that may change in environments and need deployment that may involve merging changes, etc. not just a flat-out push like the schema files that cannot have different changes per environment. Which is why they are deployed with code.