Problem/Motivation

Detect config file renames during the import process. According to comment #2, there are two use cases (in the fields and instances conversion) where the import is not going to detect a rename, of which one is quite problematic.

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

Proposed resolution

According to comment #3, we should re-write the config import process to use UUIDs instead of config object names. Or a combination of both (comment #5).

Remaining tasks

Original report by heyrocker

Right now, we have no way to handle renamed config. If you rename a config file, it will appear in the import process as the deletion of one file and the creation of another one. This will probably use UUIDs within the config file to determine that this same config just has a different name, rather than being an all new config.

Followups

#2240813: Move ImportableEntityStorageInterface check in ConfigImporter to a validation event
#2240819: Improve storage changelists to handle rename information better

Files: 
CommentFileSizeAuthor
#78 1740378.78.patch38.8 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,061 pass(es).
[ View ]
#78 75-78-interdiff.txt2.12 KBalexpott
#75 1740378.75.patch38.6 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,021 pass(es).
[ View ]
#75 72-75-interdiff.txt20.78 KBalexpott
#72 interdiff-68-72.txt18.69 KBxjm
#72 config-rename-1740378-72.patch35.29 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,008 pass(es).
[ View ]
#68 1740378.68.patch34.21 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,844 pass(es).
[ View ]
#68 64-68-interdiff.txt10.21 KBalexpott
#64 1740378.64.patch32.66 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,842 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#64 60-64-interdiff.txt19.02 KBalexpott
#64 rename-screenshot.png113.42 KBalexpott
#60 1740378.60.patch18.21 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,801 pass(es).
[ View ]
#60 53-60-interdiff.txt1.78 KBalexpott
#53 48-53-interdiff.txt1.21 KBalexpott
#53 1740378.53.patch18.24 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,748 pass(es).
[ View ]
#48 1740378.48.patch17.36 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,755 pass(es).
[ View ]
#48 46-48-interdiff.txt3.79 KBalexpott
#46 1740378.46.patch15.79 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,646 pass(es).
[ View ]
#41 1740378-config_rename-41.patch16.04 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,810 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#39 1740378-config_rename-39.patch16.01 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/StorageComparer.php.
[ View ]
#37 interdiff-34-36.txt1.47 KBDésiré
#37 1740378-config_rename-37.patch44.74 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,847 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#34 interdiff-32-34.txt2.65 KBDésiré
#34 1740378-config_rename-34.patch44.71 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/StorageComparer.php.
[ View ]
#32 interdiff-29-32.txt1.42 KBDésiré
#32 1740378-config_rename-32.patch16.16 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,891 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#29 interdiff-24-29.txt9.34 KBDésiré
#29 1740378-config_rename-29.patch15.96 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,844 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#24 interdiff-21-24.txt4.43 KBDésiré
#24 1740378-config_rename-24.patch11.76 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,803 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#21 1740378-config_rename-21.patch13.54 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#19 1740378-config_rename-18.patch13.55 KBDésiré
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1740378-config_rename-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 1740378.11.patch14.02 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,259 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Comments

sun’s picture

swentel’s picture

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.

sun’s picture

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.

yched’s picture

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

heyrocker’s picture

Priority:Major» Critical

Upgrading this status because this is obviously a critical missing feature.

I was thinking about this today and I think the best way to manage this is to add the config UUIDs to the manifest files. Using the combination of UUID and machine name to detect renames should be reasonably simple to implement.

thanhtek’s picture

Issue Summary Template has been added.

thanhtek’s picture

Issue summary:View changes

Issue Summary Template

mtift’s picture

Issue summary:View changes

Updated issue summary.

mtift’s picture

Issue tags:+sprint

Tagging

ruloweb’s picture

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.

beejeebus’s picture

bump.

beejeebus’s picture

Issue summary:View changes

Updated issue summary.

alexpott’s picture

Issue tags:+beta blocker

Wow how did we forget about this?

alexpott’s picture

Status:Active» Needs review
StatusFileSize
new14.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,259 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 11: 1740378.11.patch, failed testing.

Berdir’s picture

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?

xjm’s picture

Status:Needs work» Postponed

Postponing per #11.

jessebeach’s picture

Issue summary:View changes
jessebeach’s picture

Title:Implement renames in the import cycle» [PP-1] Implement renames in the import cycle
jessebeach’s picture

Title:[PP-1] Implement renames in the import cycle» Implement renames in the import cycle
Status:Postponed» Active

Unpostponed!

beejeebus’s picture

just had a discussion with alexpott at drupaldevdays - we'd like to split this out in to two issues.

first, when we do delete/create (a config entity with the same name but different UUID is imported), and second, when we do a rename (where a config entity changes name, but the UUID is the same).

leaving this issue as rename, and created #2224873: Handle a config entity with the same name but different UUID being imported for the first case.

Désiré’s picture

Status:Active» Needs review
StatusFileSize
new13.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1740378-config_rename-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll

Status:Needs review» Needs work

The last submitted patch, 19: 1740378-config_rename-18.patch, failed testing.

Désiré’s picture

Status:Needs work» Needs review
StatusFileSize
new13.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

alexpott’s picture

Status:Needs review» Needs work
+++ 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

The last submitted patch, 21: 1740378-config_rename-21.patch, failed testing.

Désiré’s picture

StatusFileSize
new11.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,803 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
new4.43 KB
  • Same name test removed.
  • Undeclared method calls fixed.
  • Doc comment for addChangelistRename().
Désiré’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 24: 1740378-config_rename-24.patch, failed testing.

Désiré’s picture

Assigned:Unassigned» Désiré
Désiré’s picture

Status:Needs work» Needs review
StatusFileSize
new15.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,844 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
new9.34 KB
Désiré’s picture

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

Status:Needs review» Needs work

The last submitted patch, 29: 1740378-config_rename-29.patch, failed testing.

Désiré’s picture

Status:Needs work» Needs review
StatusFileSize
new16.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,891 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.42 KB

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

Status:Needs review» Needs work

The last submitted patch, 32: 1740378-config_rename-32.patch, failed testing.

Désiré’s picture

Issue summary:View changes
StatusFileSize
new44.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/StorageComparer.php.
[ View ]
new2.65 KB

Some minor refactor in addChangelistRename().

Désiré’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 34: 1740378-config_rename-34.patch, failed testing.

Désiré’s picture

Status:Needs work» Needs review
StatusFileSize
new44.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,847 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new1.47 KB

Status:Needs review» Needs work

The last submitted patch, 37: 1740378-config_rename-37.patch, failed testing.

Désiré’s picture

Status:Needs work» Needs review
StatusFileSize
new16.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/StorageComparer.php.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 39: 1740378-config_rename-39.patch, failed testing.

Désiré’s picture

Status:Needs work» Needs review
StatusFileSize
new16.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,810 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 41: 1740378-config_rename-41.patch, failed testing.

xjm’s picture

Title:Implement renames in the import cycle» [PP-1] Implement renames in the import cycle
Assigned:Désiré» Unassigned
Status:Needs work» Postponed

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.

xjm’s picture

Title:[PP-1] Implement renames in the import cycle» Implement renames in the import cycle
Status:Postponed» Needs work

Unpostponed now.

xjm’s picture

Assigned:Unassigned» alexpott
alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new15.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,646 pass(es).
[ View ]

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

xjm’s picture

Status:Needs review» Needs work
alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new3.79 KB
new17.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,755 pass(es).
[ View ]
xjm’s picture

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

alexpott’s picture

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

Gábor Hojtsy’s picture

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.

xjm’s picture

Status:Needs review» Needs work
alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new18.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,748 pass(es).
[ View ]
new1.21 KB

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.

beejeebus’s picture

Status:Needs review» Reviewed & tested by the community

i'm ok with fixing the awkward in a follow up. RTBC if this is green.

beejeebus’s picture

Status:Reviewed & tested by the community» Needs work

doh, i forgot the other concern i had.

<?php
+    $entity_storage = $this->configManager->getEntityManager()->getStorage($entity_type_id);
+   
// Call to the configuration entity's storage to handle the configuration
+    // change.
+    if (!($entity_storage instanceof ImportableEntityStorageInterface)) {
+      throw new
EntityStorageException(String::format('The entity storage "@storage" for the "@entity_type" entity type does not support imports', array('@storage' => get_class($entity_storage), '@entity_type' => $entity_type)));
+    }
?>

this looks out of place. if we can work this out during validate, we should, else we're allowing a known-bad import to proceed past the point of no return.

xjm’s picture

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

alexpott’s picture

Assigned:alexpott» Unassigned
Issue summary:View changes
Status:Needs work» Needs review

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

xjm’s picture

@alexpott, can you link said followups?

alexpott’s picture

alexpott’s picture

StatusFileSize
new1.78 KB
new18.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,801 pass(es).
[ View ]
beejeebus’s picture

Status:Needs review» Reviewed & tested by the community

ok, RTBC with follow-ups in.

xjm’s picture

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

xjm’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new113.42 KB
new19.02 KB
new32.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,842 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

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.

xjm’s picture

Assigned:Unassigned» xjm
+++ 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. :)

xjm’s picture

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

Status:Needs review» Needs work

The last submitted patch, 64: 1740378.64.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new10.21 KB
new34.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,844 pass(es).
[ View ]

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

xjm’s picture

Thanks, that's much better.

xjm’s picture

Status:Needs review» Needs work

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

tim.plunkett’s picture

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

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

xjm’s picture

Assigned:xjm» Unassigned
Status:Needs work» Needs review
StatusFileSize
new35.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,008 pass(es).
[ View ]
new18.69 KB

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

xjm’s picture

  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.

xjm’s picture

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.

alexpott’s picture

StatusFileSize
new20.78 KB
new38.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,021 pass(es).
[ View ]
  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

xjm’s picture

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.

xjm’s picture

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

alexpott’s picture

StatusFileSize
new2.12 KB
new38.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,061 pass(es).
[ View ]

#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

xjm’s picture

Status:Needs review» Reviewed & tested by the community

That all makes sense. Looks good to me.

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

  • Commit 37250bb on 8.x by webchick:
    Issue #1740378 by xjm, Désiré, alexpott | heyrocker: Implement renames...
xjm’s picture

Status:Reviewed & tested by the community» Fixed

Status:Fixed» Closed (fixed)

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