Add a config storage copy utility trait and fix ConfigManager::createSnapshot

This patch is super clear and really easy to read, I have found some really small things but not setting back to needs work for any of them. I think on all of them you could just answer "no" and I'd still be happy with this patch :)

I also already agreed at drupalcamp Ghent that delete first, copy everything to target was a good sequence.

Great work @bircher!

1. +++ b/core/lib/Drupal/Core/Config/StorageCopier.php
   @@ -0,0 +1,57 @@
   +   * and copies also all collections. This is not importing configuration,
   +   * DO NOT use the active storage as the target.
   

   Should we try to throw an exception when someone tries to do this? Or can we imagine someone trying to do this?

2. +++ b/core/lib/Drupal/Core/Config/StorageCopier.php
   @@ -0,0 +1,57 @@
   +    if ($target->getCollectionName() != StorageInterface::DEFAULT_COLLECTION) {
   

   Can we use strict comparison here (and in similar points in this method?)

3. +++ b/core/lib/Drupal/Core/Config/StorageCopier.php
   @@ -0,0 +1,57 @@
   +    // Copy everything.
   ...
   +    // Copy collections as well.
   

   This isn't very clear. we first copy everthing, and then we copy some other things. The first thing is collection-less config, right?

   Can we document that more clearly?

4. +++ b/core/tests/Drupal/Tests/Core/Config/StorageCopierTest.php
   @@ -0,0 +1,76 @@
   +  public function testCopyConfig() {
   

   This function tests both the copy and the fact that the copy command deletes before copying, should we split that up? Not sure, I like that this is only one method, but since this is a unit test there's hardly any cost to making it more clear?

I agree, looks good. It definitely can be a statically used class as it's not meant to be injected anywhere and there are no other ways storage values should be copied over.

A couple of minor things on my part as well:

1. +++ b/core/lib/Drupal/Core/Config/StorageCopier.php
   @@ -0,0 +1,57 @@
   +    foreach ($source->getAllCollectionNames() as $collection) {
   

   This made me wonder about what getAllCollectionNames() returns or should return. I see all the storage implementations returning the collection names without the default collection. Maybe we should document this on the StorageInterface::getAllCollectionNames() no? That implementations should not return inclusive of the default name.

2. +++ b/core/tests/Drupal/Tests/Core/Config/StorageCopierTest.php
   @@ -0,0 +1,76 @@
   +   * Generate random data in a config Storage.
   

   No need to capitalise "Storage" I guess.

3. +++ b/core/tests/Drupal/Tests/Core/Config/StorageCopierTest.php
   @@ -0,0 +1,76 @@
   +  protected function generateRandomData(StorageInterface &$storage) {
   

   No need to pass this by reference no? It's an object so it's automatically by ref.

4. +++ b/core/lib/Drupal/Core/Config/StorageCopier.php
   @@ -0,0 +1,57 @@
   +    // Copy everything.
   

   As borrison_ pointed out, I'd update this description to: "Copy the default collection" or something like this.

Awesome, I'd rtbc this, but it's actually postponed, so not doing that yet. In any case this looks great.

+++ b/core/lib/Drupal/Core/Config/StorageCopier.php
@@ -0,0 +1,57 @@
+class StorageCopier {
...
+  public static function copyConfig(StorageInterface $source, StorageInterface $target) {

Let's make this a trait with a protected static instead of a public static function. This makes it a bit less of generic utility that can be (ab)used to copy something to active storage.

I wonder if this can be simplified rather than handling StorageInterface::DEFAULT_COLLECTION so differently. Here is a patch that passes the test, but this needs checking by somebody who understands the complexity of Collections and DEFAULT_COLLECTION more than I.

Some thoughts:

1. +++ b/core/lib/Drupal/Core/Config/StorageCopyTrait.php
   @@ -0,0 +1,42 @@
   +   * This method makes sure that the target storage is emptied before copying
   +   * and copies also all collections. This is not importing configuration,
   +   * DO NOT use the active storage as the target.
   

   can we prevent this from happening programmatically instead of just a warning in the docbloc?

2. +++ b/core/lib/Drupal/Core/Config/StorageCopyTrait.php
   @@ -0,0 +1,42 @@
   +  protected static function copyConfig(StorageInterface $source, StorageInterface &$target) {
   +    // Delete all, the destination storage needs to be empty.
   +    foreach (array_merge([StorageInterface::DEFAULT_COLLECTION], $target->getAllCollectionNames()) as $collection) {
   +      $target_collection = $target->createCollection($collection);
   +      $target_collection->deleteAll();
   +    }
   

   we could separate the empty logic here to its own method and throw an exception if the targeted storage isn't empty. that means the method is only responsible for one thing and not putting the target in its correct state first.

I think I agree with @johndevman we should not do copy and delete in the same function. I'm not sure that copy should check for emptiness by default.

I don't agree with removing the "empty/delete" logic. That is part of the copy process. We are trying to make the dest config the same as the source config, and that means adding new items, changing existing items, and deleting old items. This is implemented by emptying the dest, then adding the source. The fact it is "deleting" is an implementation detail and not something that should be split into a separate callable function. If the caller forgot to call the `emptyConfigStorage` then whey would have a mess, or they would get an exception for "storage not empty" and in general that is making it needlessly more complex for the caller.

I think `copyConfig` is still a valid name for this. It is not just `addConfig`. We aren't making a CRUD interface here.

I can see the use-case of a newly created Storage object only wanting to call `copyStorage` without needing to clear it first. So I'm happy with this and the new names.

+++ b/core/lib/Drupal/Core/Config/StorageCopyTrait.php
@@ -0,0 +1,69 @@
+    foreach (array_merge([StorageInterface::DEFAULT_COLLECTION], $storage->getAllCollectionNames()) as $collection) {
+      $storage->createCollection($collection)->deleteAll();

Wonder if DEFAULT_COLLECTION needs to come last in the array_merge here. Thinking of how this would work on a FileStorage where the parent directory needs to be deleted last when empty.

OK, with that answered I'm ready to RTBC this now.

Oh noes seems like I had an xpost... I thought I'd commented on this hours ago. I'm not sure the compromise is a good deal. We've now got three methods to cope with and we've got to account for the difference between syncStorage and the ConfigSync class.

Let's choose one way (copy and clear) or another (sync - but with a better name). I'm not particularly in favour one on over the other. The 2 method approach offers future flexibility whilst the 1 method approach offers ease of use and consistency.

I could go either way. I understand both sides. I actually like the name "syncStorage" because that's exactly what it does and doesn't get confused with whether Copy deletes stuff or just adds new stuff. I think @bircher was trying to support more flexibility and other use cases. It also makes the syncStorage a bit easier to understand by splitting the sub-functions into their own. Would we combine those back into the single sync method, or just mark them as private?

At this point I'd say to go with whichever has the best chance of getting committed. We only have a couple days left for 8.7 and it would be super helpful to get this into core so other parts of the initiative can start using it.

To help move this along, here is a version back to a single "syncStorage" method. This is the smallest refactor from current core and includes the bug fix from #14. Included an interdiff from #13 since this is going back to the single-method approach.

As mentioned above, I think "syncStorage" is a better indication of what this does. If somebody has a better name, please make a specific suggestion.

We could also rename the Trait itself from StorageCopyTrait to StorageSyncTrait but that would have made the interdiff harder to review.

+++ b/core/lib/Drupal/Core/Config/StorageCopyTrait.php
@@ -0,0 +1,41 @@
+   * Synchronize the configuration from one storage to another.

This should be updated to reflect the name of the method.

Mean while I'm more +1 to have two methods, I understand the concerns adressed by bircher and mpotter and I buy it. I also think the naming makes sense, in my opinion 'replace' is the closest thing to describe what we actually do and I think the verbosity of actually stating that it's the contents of the storage that is replaced makes it clear to the user what's going on. This behaviour is similar to when you move a folder on the file system to another one where it already exists, you "Replace" it.

+1 on replaceStorageContents - nice work on the name.

+++ b/core/tests/Drupal/Tests/Core/Config/StorageCopyTraitTest.php
@@ -16,7 +16,7 @@ class StorageCopyTraitTest extends UnitTestCase {
   public function testSyncStorage() {

The test method name too :)

This patch makes a lot of sense, nice work. Some observations below

1. +++ b/core/lib/Drupal/Core/Config/StorageCopyTrait.php
   @@ -0,0 +1,41 @@
   +   * Configuration is only copied and not imported, so should not use
   +   * with active storage as the target.
   

   should not use => should not be used ?

2. +++ b/core/lib/Drupal/Core/Config/StorageCopyTrait.php
   @@ -0,0 +1,41 @@
   +  protected static function replaceStorageContents(StorageInterface $source, StorageInterface &$target) {
   

   do we need the pass by reference operator given this is an object and therefore is passed by reference by default?

3. +++ b/core/tests/Drupal/Tests/Core/Config/StorageCopyTraitTest.php
   @@ -0,0 +1,95 @@
   +    if (!empty($collections)) {
   ...
   +    if (!empty($collections)) {
   

   what if collection is empty? should we $this->fail() cause we're in an unexpected state?

4. +++ b/core/tests/Drupal/Tests/Core/Config/StorageCopyTraitTest.php
   @@ -0,0 +1,95 @@
   +    for ($i = 0; $i < rand(0, 5); $i++) {
   

   ah, so this is why we have the code above - in some tests we test for collection names, but in others we don't. I think this is brittle, if we care about the behaviour in both cases, we should have an explicit case for with and without a collection, and we can easily do that with a dataProvider

How does one make the testbot not fail patches that are to test only, I thought test-only would do the trick..

I think that only works when the test-only patch is the first uploaded patch.

I really like the new direction + name here, replaceStorageContents is very clear.

Interdiff looks great.

We have better test coverage and proved why we need to pass by reference.

Back to RTBC.

dispatcher/testbot issue. back to rtbc

Committed and pushed 21c3504993 to 8.8.x and fe487bd5e2 to 8.7.x. Thanks!

publish change record