Implement renames in the import cycle

This issue sorta duplicates #1609418: Configuration objects are not unique

This is important for the fields and instances conversion. There are two use cases where the import is not going to detect a rename, of which one is quite problematic.

• Rename bundle, e.g article to article2: in that case the instance is going to be renamed, e.g field.instance.node.article.field_name.yml to field.instance.node.article2.field_name. The import will detect a delete and a new import. It would be nicer in case this was detected as a rename, but isn't problematic as there's no data loss or so.
• Deleting a field and recreating a field with the same name. The import will detect this as a change for both the field instance, while it actually should be a delete and a new import. Ironically, we /do/ want data loss here because deleting a field is going to rename the data en revision table so the data in it can be removed later during cron runs.

Talked this through quickly with alexpott and he suggested to use validation API and error out and say people should go back to dev and re-rename (basically delete and create) the field to a different name. This buys us time, however, we lose a feature that was in Field API.

That's why we implemented UUIDs — see #1. As explained over in that issue already:

The config import process has to be rewritten to be based on UUIDs instead of config object names, so we are able to detect renames as well as deleted-recreated-with-same-name objects.

That's supposed to happen in that issue.

[edit: crosspost]

Recognizing "different uuid on import" as "delete existing + create new" is definitely something that we need to support in the end. I kind of thought it was "fixed" alredy.

I've lost track where exactly this logic could/should reside - "somewhere" in the generic import process or within Field API's specific reaction to it. I'd tend to think the former of course, sounds like basic import logic rather than something specific to Field API. Basically, this is needed for any config entity with a CRUD cycle - and with entity API hooks possibly implemented in 3rd part code, that's *all* config entities, Field API is just the subsystem with the most critical use case in core right now.

Issue Summary Template has been added.

Issue Summary Template

Updated issue summary.

Tagging

Just tested case 1

1. Rename bundle, e.g article to article2: in that case the instance is going to be renamed, e.g field.instance.node.article.field_name.yml to field.instance.node.article2.field_name. The import will detect a delete and a new import. It would be nicer in case this was detected as a rename, but isn't problematic as there's no data loss or so.

And it breaks nodes of that bundle.

Usually when you change the bundle machine_name the node_type_update_nodes function change the node's bundle in the database:

https://api.drupal.org/api/drupal/core%21modules%21node%21lib%21Drupal%2...

so everything is ok, but when you import a config that change a bundle name, this function is not executed, then you breaks all the nodes of that bundle, so yes, there is data lost.

I suggest to change the issue summary.

Wow how did we forget about this?

A starter for ten :)

The patch detects both renames (where an incoming create has the same UUID as a delete) and recreations (where an update has a UUID change and should be a delete and then a create). It adds a test based on these two scenarios for a content type - which works out well because creating a node type at the moment involves the creation of a field, field instance, entity form mode and an entity display mode! Additionally it is currently possible to change the machine name of a content type through the UI.

The recreation import works fine but the renames breaks due to secondary writes and deletes and needs #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall to land. Therefore I will postpone this issue on that once the testbot is back.

That said I think we should consider preventing renames and actually not support them - the complexities of keeping everything in sync and not breaking anything is boggling and this will only actually get worse after the dependencies patch lands since we we need to update the dependencies of other config entity when a rename occurs :(

Yeah, renames are hard. Not sure what and how to support them, but we should probably at least inform the user?

And recreations is something that we should be able to handle, and is IMHO at least as important.

Not sure I understand why we can't use the existing API to rename a config entity?

Postponing per #11.

Unpostponed!

Reroll

Well... ConfigStorageController.php was just renamed in the last 15 minutes...

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportRenameTest.php
@@ -0,0 +1,140 @@
+  public function testSameName() {
+    $type_name = Unicode::strtolower($this->randomName(16));
+    $content_type = entity_create('node_type', array(
+      'type' => $type_name,
+      'name' => 'first node type',
+    ));
+    $content_type->save();
+    /** @var \Drupal\Core\Config\StorageInterface $active */
+    $active = $this->container->get('config.storage');
+    /** @var \Drupal\Core\Config\StorageInterface $staging */
+    $staging = $this->container->get('config.storage.staging');
+
+    $config_name = $content_type->getEntityType()->getConfigPrefix() . '.' . $content_type->id();
+    $this->copyConfig($active, $staging);
+
+    // Change the machine name of the content type. This wil rename 5 configuration
+    // entities: the node type, the body field, the body field instance, the
+    // entity form display and the entity view displays for teaser and default.
+    $content_type->delete();
+    $this->assertFalse($active->exists($config_name), 'Content type\'s old name does not exist active store.');
+    // Recreate with the same type - this will have a different UUID.
+    $content_type = entity_create('node_type', array(
+      'type' => $type_name,
+      'name' => 'second node type',
+    ));
+    $content_type->save();
+
+    $this->configImporter->reset();
+    $this->assertEqual(6, count($this->configImporter->getUnprocessed('create')), 'There are 6 configuration items to create.');
+    $this->assertEqual(6, count($this->configImporter->getUnprocessed('delete')), 'There are 6 configuration items to delete.');
+    $this->assertEqual(0, count($this->configImporter->getUnprocessed('update')), 'There are no configuration items to update.');
+    $this->assertEqual(0, count($this->configImporter->getUnprocessed('rename')), 'There are no configuration items to rename.');
+
+    $this->configImporter->import();
+
+    // Verify that there is nothing more to import.
+    $this->assertFalse($this->configImporter->reset()->hasUnprocessedChanges());
+    $content_type = entity_load('node_type', $type_name);
+    $this->assertEqual('first node type', $content_type->label());
+  }

This test is not needed. It already exists in #2224873: Handle a config entity with the same name but different UUID being imported

• Same name test removed.
• Undeclared method calls fixed.
• Doc comment for addChangelistRename().

Now posptoned on #2124535: Prevent secondary configuration creates and deletes from breaking the ConfigImporter.

While we waiting for #2124535: Prevent secondary configuration creates and deletes from breaking the ConfigImporter here are some improvements for this.

+++ b/core/tests/Drupal/Tests/Core/Config/StorageComparerTest.php
@@ -211,10 +211,10 @@ public function testCreateChangelistUpdate() {
+    $this->sourceStorage->expects($this->atLeastOnce())

Here I'm unsure how can we do this better, since the new addChangelistRename() will call 'readMultiple' more times.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportRenameTest.php
@@ -0,0 +1,106 @@
+    debug($this->configImporter->getUnprocessed('rename'));
+    $this->assertEqual(5, count($this->configImporter->getUnprocessed('rename')), 'There are 5 configuration items to rename.');

Also here, the debug will show that the body field base will not updated only the field instance. I don't know what is the expected here.

This new patch should fix the exception in ConfigSnapshotTest. But I think this solution will need some cleanup.

Some minor refactor in addChangelistRename().

Hm... Something messed up when I created the #34 patch (and #37, just see the file sizes), so here I recreated the right one.

I see, that syntax error doesn't happens with PHP 5.5... (So repeat #37)

Thanks for the work on this. At this point, per discussion with @alexpott, posptoning until #2124535: Prevent secondary configuration creates and deletes from breaking the ConfigImporter is in since there is overlap.

Unpostponed now.

Working patch - no interdiff due to considerable changes. Needs tidy up.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -452,8 +452,6 @@
-      // @todo Implement proper dependency ordering using
-      //   https://drupal.org/node/2080823

We're removing this @todo, but I don't see anything in the interdiff that addresses it?

The @todo is being removed from the patch because it was addressed by #2030073: Config cannot be imported in order for dependencies

Reviewed this patch.

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -689,6 +725,38 @@ protected function importInvokeOwner($op, $name) {
   +  /**
   +   * @param string $name
   +   * @return bool
   +   */
   

   Docs need to be filled in still.

2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -689,6 +725,38 @@ protected function importInvokeOwner($op, $name) {
   +    $names = explode('::', $name);
   
   +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -209,6 +211,71 @@ protected function addChangelistUpdate() {
   +        $renames[] = $id . '::' . $create_uuids[$data['uuid']];
   

   I'm not very well versed in the import code, but this :: concatenated approach seems to be very strange to me...

   I see the changelists are designed to take strings only but this looks like an ugly workaround so we don't need to change any of that...

3. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -209,6 +211,71 @@ protected function addChangelistUpdate() {
   +   * @param string $name
   +   *   The name of the configuration to remove.
   

   So this is not really a name, it may be a name pair concatenated too for the rename op. This may need to be updated elsewhere too.

1. Fixed
2 & 3. Yeah @beejeebus is not that keen either but change this from a string will mean that all the changelist code will need to change - can we explore doing this is a beta target - as this works and is not really an important developer facing API.

Let's also get any needed followups filed before we RTBC, and maybe we should get a review from yched if possible?

#55 should be a followup too since this is a copy and paste from ConfigImporter::importInvokeOwner()

All necessary followups created. I think this is good to go.

@alexpott, can you link said followups?

I did add them to the issue summary but let's make them related too...

Rerolled now that #2236553: Config import validation needs to be able to list multiple issues and the messages should be translatable has landed

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportRenameTest.php
@@ -0,0 +1,107 @@
+    // If we try to do this then we fail badly because of secondary writes and
+    // deletes.
+    $this->configImporter->import();
+  }
+
+}

The test makes sense up to this bit?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportRenameTest.php
@@ -0,0 +1,107 @@
+    // Change the machine name of the content type. This wil rename 5
+    // configuration entities: the node type, the body field instance, the
+    // entity form display and the entity view display.
...
+    $this->assertEqual(4, count($this->configImporter->getUnprocessedConfiguration('rename')), 'There are 4 configuration items to rename.');

Also, the comment says 5 config entities, but the assertion says 4. More specific assertions might be better. Looks like the test could use some work, so setting NW per discussion with @alexpott. It might also be good to document what happens for the user with screenshots or suchlike.

Improved the test and added a new test to cover the validation code.

So @xjm's question about the UI prompted quite a bit of thinking. Renames were not even being listed by the patch in #64. Now they are and the user can diff them. Screenshot of how the list of renames looks attached.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -668,6 +669,21 @@ protected function getNextConfigurationOperation() {
+        // Has to be a configuration entity.
+        if (!$old_entity_type_id) {
+          $this->logError($this->t('Rename operation for simple configuration. Existing configuration !old_name and staged configuration !new_name.', array('old_name' => $old_name, 'new_name' => $new_name)));
+        }

How would this case even happen? Aren't the renames detected based on UUIDs?

There's also a few minor coding standards issues in the patch. Rather than enumerating, I'll clean them up. :)

+++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -209,6 +211,71 @@ protected function addChangelistUpdate() {
+        // Create rename operation.
+        $renames[] = $id . '::' . $create_uuids[$data['uuid']];

This is where the magical double colon concatenation happens; I spent awhile trying to find it. We should document that in this docblock and probably @see from the methods that take the colon-concatenated-name-thing.

Also, colons are a legitimate filename character on my filesystem, weird as that seems. Are we already validating config prefixes and IDs to ensure this character is not used otherwise, so that there is no risk of false positives?

@xjm: so colons are not legal config characters see ConfigBase::validateName()

Added methods to StorageComparer to encapsulate the rename name oddness which might make #2240819: Improve storage changelists to handle rename information better obsolete. And fixed the test.

Thanks, that's much better.

More correct status since I'm still fixing things here albeit distractedly.

+++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -318,2 +318,27 @@
+    $names = explode('::', $name);

This should use explode('::', $name, 2) to be safe

Here we go. Sorry for the delay; the patch went and got twice as big following my question about the UI. :)

I still have several outstanding questions, which I'll post in a moment when I can use dreditor on my own patch. ;)

1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
   @@ -668,6 +669,19 @@ protected function getNextConfigurationOperation() {
   +        // Has to be a configuration entity.
   +        if (!$old_entity_type_id) {
   +          $this->logError($this->t('Rename operation for simple configuration. Existing configuration !old_name and staged configuration !new_name.', array('old_name' => $names['old_name'], 'new_name' => $names['new_name'])));
   +        }
   

   I'm still not sure how we would ever get to this case, since the rename is detected based on UUID; we check elsewhere that the importer is an instance of WhatsitEntityImporterInterface (whatever). It's certainly robust to have this here, but does this mean we have an untested code path?

2. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -209,6 +211,72 @@ protected function addChangelistUpdate() {
   +    // Import the renames in reverse order, so that (e.g.) content types are
   +    // handled before fields.
   +    $this->addChangeList('rename', array_reverse($renames));
   

   This has me going "hmm" in light of #2240709: ConfigImportUITest::testImport fails when the module list changes. It's unclear why the array_reverse() means that content types are handled before fields, and it's weird that we're mentioning content types and fields specifically in this generic class. At least, we need more detail in this comment.

3. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -249,4 +317,31 @@ protected function getAndSortConfigData() {
   +  protected function createRenameName($name1, $name2) {
   
   +++ b/core/lib/Drupal/Core/Config/StorageComparerInterface.php
   @@ -80,4 +80,30 @@ public function hasChanges($ops = array('delete', 'create', 'update'));
   +  public function extractRenameNames($name);
   

   Maybe these methods should be static, since they don't require the instantiated object in any way?

4. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -249,4 +317,31 @@ protected function getAndSortConfigData() {
   +    $names = explode('::', $name, 2);
   

   I did as @tim.plunkett suggested here as a best practice for explode(). We shouldn't ever run into a case where we have more than one set of :: because of the config name validation described above, but this is fine.

5. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
   @@ -214,9 +218,18 @@ public function buildForm(array $form, array &$form_state) {
   +          $names = explode('::', $config_file);
   

   This still needs to be replaced with our new method. I guess that means we'd need to add the config importer to the 39,000 injected dependencies? Edit: Maybe unless we made it static as I suggest above.

6. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportRenameTest.php
   @@ -0,0 +1,125 @@
   +    // We expect that changing the machine name of the content type will
   +    // rename five configuration entities: the node type, the body field
   +    // instance, two entity form displays, and the entity view display.
   

   Where are they coming from, why are there these 5? Does node really create all of this for you automagically, or is this coming from the config test module? More detail or an @see to the config we're testing on would not go amiss.

Also, ConfigImportRenameTest isn't generic; rather, it's a test of renames' impact on content types and fields. Maybe it would be good to make that the explicit purpose for the test (including renaming it, moving it to node.module, whatever) so that it doesn't break unexpectedly when we change what node does with the body field and whatnot.

A part of me also wants to ask for a decoupled test that just uses the test implementation, but OTOH we already have other coverage using the test config entity and this specific case is already more rich than a test scenario we'd derive, so I think the coverage is good enough.

1. Yep this code is not tested - test added.
2. Good point $this->targetNames is sorted correctly already new patch uses that.
3. You never have this rename without a storageComparer so I don't think this is necessary
4. It just codifies our expectations
5. We have a storageComparer as per #3 - so fixed :)
6. Yes the \Drupal\node\Entity\NodeType::postSave and the firing of the bundle rename hook is a config rename avalanche.

Move and renamed the test to node & NodeTypeRenameConfigImportTest

Looking good. Two things.

1. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
   @@ -209,6 +211,76 @@ protected function addChangelistUpdate() {
   +    // Renames should be ordered so that dependencies are renamed last. This
   +    // ensures that if there is logic in the configuration entity class to keep
   +    // names in sync it should still work.
   +    // @see \Drupal\node\Entity\NodeType::postSave()
   +    foreach ($this->targetNames as $name) {
   +      $data = $this->targetData[$name];
   +      if (isset($data['uuid']) && isset($create_uuids[$data['uuid']])) {
   +        // Remove the item from the create list.
   +        $this->removeFromChangelist('create', $create_uuids[$data['uuid']]);
   +        // Remove the item from the delete list.
   +        $this->removeFromChangelist('delete', $name);
   +        // Create the rename name.
   +        $renames[] = $this->createRenameName($name, $create_uuids[$data['uuid']]);
   +      }
   +    }
   +
   +    $this->addChangeList('rename', $renames);
   +  }
   

   I have no idea how the comment at the beginning of this bears any relation to the code following it. To the reader, the code inside the foreach() has nothing to do with ordering. Either the comment is in totally the wrong place, or it's missing some explanation, or both. @alexpott spammed a lot of "look at docblocks for fooMethod()" at me and mentioned StorageComparer::getAndSortConfigData(), so if no one else improves this in the meanwhile, I'll look at that when I have the chance to try to figure out what needs to be clarified here. ;)

2. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportRenameValidationTest.php
   @@ -0,0 +1,168 @@
   +      $this->fail('Expected ConfigImporterException thrown when a renamed configuration entity does not match the existing entity type.');
   ...
   +      $this->pass('Expected ConfigImporterException thrown when a renamed configuration entity does not match the existing entity type.');
   

   Copypasta fail.

One more question, sorry:

+++ b/core/lib/Drupal/Core/Config/StorageComparer.php
@@ -236,20 +236,24 @@
-    $this->addChangeList('rename', array_reverse($renames));
+    $this->addChangeList('rename', $renames);

If the array_reverse() was completely wrong, why did no tests fail? :)

#76:

1. Comment improved
2. Fixed

#77 - Because the order checked for in the test was wrong - because the test is renamed this is hidden in the interdiff

That all makes sense. Looks good to me.

Since I mostly only cleaned up nitpicky stuff, I'm comfortable RTBCing this pending tests passing. :)