Problem/Motivation

Over in #2224887: Language configuration overrides should have their own storage we've been exploring providing a separate storage for language configuration overrides. The approach has several advantages as:

  • config schema just work as the config object name is the same
  • the amount of config objects in each storage does not become a huge mess - think of how many files a staging directory could contain on a big multilingual site
  • fixes issues with key length since we don't have to prefix language overrides

However the approach falls down when trying to make it work with config synchronisation because:

  • If we are only importing language overrides at the moment this will not work because the config sync page is only looking for changes in the regular config storage
  • The language config sync code has to make assumptions about how staging in structured and that it is in fact a file storage
  • If another module was providing overrides through a similar method of storing in pseudo config files it is going to have to write much of the same code
  • If the language module is not enabled then it is totally impossible with this approach for the sync process to inform the user what is going to happen if you stage installing language and create language configuration overrides in the same sync

Proposed resolution

Add collections to config storages. A configuration storage can contain multiple sets of configuration objects in partitioned collections. The collection key name identifies the current collection used. The storage interface contains the method getCollectionNames() to list all the available collections. This means that config sync will be able to compare staging to active even if the module that writes to that particular collection is not installed. So for example, if staging contains a collection called language.de, we can discover this and display to the user that this configuration will be synchronized even when the language module is being installed at the same time.

Use createCollection() which returns a new instance of the storage with the collection set. It does this to avoid side effects caused by the storage object being injected into each Config object - changing the collection on the storage of one Config object would change them all.

For databases this will add an extra column and in file systems this will use directories.

The patch here just adds the ability and tests it. Implementation of language to use this should be done in #2224887: Language configuration overrides should have their own storage

Remaining tasks

Review patch

User interface changes

None

API changes

Only additions.

Comments

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
133.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

We need to make CachedStorage work and introduce a proper test for it since it is not being used anymore. And add code to the config sync process to be able to handle collections.

alexpott’s picture

FileSize
22.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,564 pass(es). View

Amateur - forgot to rebase :) Ignore patch in #2

The last submitted patch, 2: 2262861.2.patch, failed testing.

Gábor Hojtsy’s picture

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

alexpott’s picture

Issue summary: View changes

Patch adds:

  • necessary documentation of what a collection is to StorageInterface
  • fixes CachedStorage to work with collections and improves its test coverage
  • removes bogus cache.config service - at the moment this has one entry for schema which seems weird to have a cache table only with one entry

Now to work on ConfigSync and ConfigImporter support.

alexpott’s picture

FileSize
18.98 KB
38.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2262861.6.patch. Unable to apply patch. See the log in the details link for more information. View

Premature save!

alexpott’s picture

Priority: Normal » Critical
Issue tags: +beta blocker

Postponed #2224887: Language configuration overrides should have their own storage on this - so marking this a beta blocker.

alexpott’s picture

Title: Add collection information to config storages » Add concept of collections to config storages
Issue summary: View changes
xjm’s picture

Issue summary: View changes
alexpott’s picture

Gábor Hojtsy’s picture

Status: Needs review » Postponed

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

Gábor Hojtsy’s picture

Status: Postponed » Needs review

7: 2262861.6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2262861.6.patch, failed testing.

Gábor Hojtsy’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
78.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,871 pass(es). View

Patch attached is a reroll after the 3 clean up issues have gone in. Plus it adds collection awareness to:

  • StorageComparer - so that the ConfigSync page and ConfigImporter can display what is going to happen.
  • ConfigSync - so that what the ConfigImporter is going to do with the collection information is displayed to user.
  • ConfigImporter - so that the changes to the collections actually occur

One interesting issue is whether or not collections should support config entities - this opens up a huge can of worms so I've implemented a method of the storageComparer::supportsConfigurationEntities() that basically says that only the default collection does. If you think about language overrides this only contain the overridden configuration keys and therefore can not be treated as an entity. I've tried to solve this in a way that makes sense for how things work at the minute but would allow us to explore other possibilities.

A test is added to test the importing of collection configuration change.

alexpott’s picture

FileSize
9.61 KB
86.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,951 pass(es). View

Added ability to diff config in that is stored in collections.

alexpott’s picture

How to test this patch:

  1. Create some config in a collection. You can use drush core-cli to do this.
    $collection = \Drupal::service('config.storage')->createCollection('test');
    $collection->write('system.performance', \Drupal::config('system.performance')->get());
    
  2. Export the configuration admin/config/development/configuration/full/import to a tarball and extract to the staging directory. Or you can use the Drush patch attached to this issue to use drush cex -y
  3. In the staging directory there will now be a directory called test with a configuration file called system.performance.yml in it.
  4. You can edit this file to make differences between the staging and active test collection
  5. Visit admin/config/development/configuration to test the diff, listing and finally the ability to import change
Berdir’s picture

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

Gábor Hojtsy’s picture

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?

alexpott’s picture

@Gábor Hojtsy - actually one of the aims of the language storage patch was to have the same config object name so that schema would apply and casting on save would work :)

There is a question about how to prevent overriding overrides but this does not occur on the StorageInterface level so it is something that can be explored in #2224887: Language configuration overrides should have their own storage

Gábor Hojtsy’s picture

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.

alexpott’s picture

FileSize
9.29 KB
88.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,289 pass(es). View
  1. Adding documentation
  2. Without removing this we'd need to add a $collection param - but none of the usages pass $op in so it seemed expedient to remove.
  3. Fixed :)
  4. Fixed
  5. Calling delete on a collection or file storage with a directory that has never been used.
  6. Good idea
  7. It does support deeper levels than two - changed ConfigStorageTestBase to prove this. Added documentation.
  8. The null storage doesn't do anything else so I don't think we should make it support collections.
  9. Fixed
  10. Improved to mention what characters make up a valid collection name and nesting / parent issue. I wish we could get around that. This limitation comes from the file system implementation and the wish to not list empty collections.
  11. Fixed
Berdir’s picture

  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?

alexpott’s picture

FileSize
1.37 KB
88.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,329 pass(es). View

Fixed up commentes

alexpott’s picture

FileSize
2.37 KB
89.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,380 pass(es). View

Whilst working on #2224887: Language configuration overrides should have their own storage discovered a bug in DatabaseStorage::exists(). Patch fixes and adds tests.

Gábor Hojtsy’s picture

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

alexpott’s picture

FileSize
129 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,438 pass(es). View
48.58 KB
  1. Nice catch
  2. Fixed
  3. Fixed
  4. Fixed

I think some of the work on the installer in #2224887: Language configuration overrides should have their own storage can be generalised so that we fire an event to list active collections and install config from them. The patch attached explores this - I'll reroll the language storage patch on top - it should have far less non test code. There are lots of changes to the ConfigInstaller service to support this.

alexpott’s picture

Drupal\config\Tests\ConfigInstallTest is the test I've updated to test all the permutations around collections and config install

alexpott’s picture

FileSize
3.42 KB
130.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,418 pass(es), 1 fail(s), and 0 exception(s). View

Missed uninstall.

Status: Needs review » Needs work

The last submitted patch, 30: 2262861.30.patch, failed testing.

sun’s picture

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.

Gábor Hojtsy’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
131.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,464 pass(es). View

To go down the route of decoupling all knowledge how collections are stored by the storage we need to a factory per storage "area" so one for active, staging and snapshot.

The new patch attached removes the collection name restrictions so both 'collection' and 'collection.new' are equally valid - tests added.

At the moment each storage has it's own factory method createCollection() that returns a new instance. And a method getAllCollectionNames() to list all the collections that are in use. The createCollection() method I think obeys the rules @sun outlines in #32. The question is does the abstraction of a factory per storage buy us anything? I think not really - just another level abstraction in an already very abstract system. As @Gábor Hojtsy explains in order for config synchronisation of collections to work we need to be able discover collections before a module that uses it is installed - without decoupling collections from the module that uses that particular collection we have no way to inform the user what will occur during a sync.

To say that this patch "to some very large extent" duplicates #2190723: Add a KeyValueStore\FileStorage to replace e.g. ConfigStorage is misleading - that patch is about architectural purity of the config storage system - it happens to introduce collections to config storage because our key value solution has the concept of collections. That patch does not touch config synchronisation, config installation of config storage comparison with respect to what collections mean for config. I have a number of concerns with respect to building config storage on top of key value storage because we have two requirements - the ability to list by partial key and the ability to list all collections in use that are not currently part of the key value sub system.

The fail in Drupal\locale\Tests\LocaleUpdateTest appears to be random.

Gábor Hojtsy’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Issue summary: View changes

Fixing issue summary to be inline with patch.

  • Commit 4e451f6 on 8.x by Dries:
    Issue #2262861 by alexpott: Add concept of collections to config...
Dries’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Fixed

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.

olli’s picture

xjm’s picture

Jose Reyero’s picture

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)

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

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