Config::rename() was added after the fact in #1739900: Add a rename operation to config storage controllers and I don't think we were aware of it when writing ConfigStorageController. We need to use it.

Files: 
CommentFileSizeAuthor
#15 8-15-interdiff.txt651 bytesalexpott
#15 1865206.config-rename-cleanup.15.patch10.82 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,291 pass(es).
[ View ]
#8 7-8-interdiff.txt1.9 KBalexpott
#8 1865206.config-rename-cleanup.8.patch10.83 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,301 pass(es).
[ View ]
#7 4-7-interdiff.txt3.46 KBalexpott
#7 1865206.config-rename-cleanup.7.patch12.4 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,273 pass(es).
[ View ]
#4 3-4-interdiff.txt1.12 KBalexpott
#4 1865206.config-rename-cleanup.4.patch8.88 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 53,329 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#3 1865206.config-rename-cleanup.3.patch8.4 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 53,376 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Comments

sun’s picture

Title:Clean up save/postsave/rename» Clean up ConfigFactory + ConfigStorageController::save/postsave/rename()
Priority:Normal» Major
Issue tags:+API clean-up, +Configuration system, +Configurables

This should definitely happen before release, so bumping to major.

swentel’s picture

Coming from #1950326: Entity displays are not renamed/deleted when a bundle is renamed/deleted. - I had to manually call the configFactory::rename() method so the internal cache was cleaned up.
Also, when renaming a config entity, the manifest files are not updated.

alexpott’s picture

Status:Active» Needs review
StatusFileSize
new8.4 KB
FAILED: [[SimpleTest]]: [MySQL] 53,376 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

I agree with what @reyero says in http://drupal.org/node/1187726#comment-6813858 point 3

3. Bad API encapsulation, as the config objects get to "rename themselves" (or delete themselves)

The patch attached moves the rename operation completely to the factory and cleans up it's use in Drupal\Core\Config\Entity\ConfigStorageController. Tests are added for the rename operation in ConfigCRUDTest and ConfigEntityTest

alexpott’s picture

StatusFileSize
new8.88 KB
FAILED: [[SimpleTest]]: [MySQL] 53,329 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new1.12 KB

@chx and @xjm pointed out that

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -403,13 +403,25 @@ public function save(EntityInterface $entity) {
+      // Entity id's can contain a dot so we can not use Config::clear() to

Should be // Entity IDs...

As this was a copy&paste job I've fixed it in the original location too...

@chx also queried the empty protected function postSave(EntityInterface $entity, $update) {}... this just the same as preSave(), preDelete() and postDelete()... plus is basest class directly implementing the EntityStorageControllerInterface for Config Entities.

chx’s picture

Status:Needs review» Reviewed & tested by the community

I ran out of things to complain about.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1865206.config-rename-cleanup.4.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new12.4 KB
PASSED: [[SimpleTest]]: [MySQL] 53,273 pass(es).
[ View ]
new3.46 KB

Image styles played up :(... that'll be our nice losely (sic) coupled drupal! @chx :D

Basically it was relying on a full delete being called on the old image style name which would then call image_image_style_delete()... to fix this I've moved all this functionality to an implementation of postDelete() on the ImageStyleStorageController and implemented rename detection in the postSave(). This has the nice side effect of removing code from image.module - and actually image_image_style_update() should move to the ImageStyleStorageController as well - but that is for another patch :)

alexpott’s picture

StatusFileSize
new10.83 KB
PASSED: [[SimpleTest]]: [MySQL] 53,301 pass(es).
[ View ]
new1.9 KB

A comment from @chx on irc made me realise that we can get away with much less change on ImageStyleStorageController and leave the refactor of the image_* entity hooks to #1821848: Move image style load/update/delete operations into a new ImageStyleStorageController.

Status:Needs review» Needs work
Issue tags:-API clean-up, -Configuration system, -Configurables

The last submitted patch, 1865206.config-rename-cleanup.8.patch, failed testing.

alexpott’s picture

Status:Needs work» Needs review
Issue tags:+API clean-up, +Configuration system, +Configurables

#8: 1865206.config-rename-cleanup.8.patch queued for re-testing.

alexpott’s picture

The fail of #8 was Drupal\system\Tests\File\StreamWrapperTest... locally passed... and completely unrelated...

chx’s picture

Status:Needs review» Reviewed & tested by the community

That does look good.

xjm’s picture

#8: 1865206.config-rename-cleanup.8.patch queued for re-testing.

xjm’s picture

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -87,7 +87,23 @@ function testCRUD() {
+    // Test renaming when config.factory does not have the object in it's static
+    // cache.

AAAAHHHHHHH

http://theoatmeal.com/comics/apostrophe

alexpott’s picture

StatusFileSize
new10.82 KB
PASSED: [[SimpleTest]]: [MySQL] 53,291 pass(es).
[ View ]
new651 bytes

Boo to me... love the cartoon :)

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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