Problem

  • Image module implements its own image style hooks in order to perform essential massaging of ImageStyle entity properties.

Solution

  • Add a new ImageStyleStorageController extends ConfigStorageController to perform those operations.
Files: 
CommentFileSizeAuthor
#33 1821848-image-33.patch10.08 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,839 pass(es).
[ View ]
#24 22-24-interdiff.txt3.79 KBalexpott
#24 1821848-image-24.patch9.78 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1821848-image-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 1821848-image-22.patch7.86 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,308 pass(es).
[ View ]
#19 1821848-image-18.patch1.01 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,642 pass(es).
[ View ]
#17 1821848-image-17.patch718 bytesandypost
PASSED: [[SimpleTest]]: [MySQL] 49,670 pass(es).
[ View ]
#6 image_storage_controller-1821848-6.patch6.43 KBrbayliss
PASSED: [[SimpleTest]]: [MySQL] 49,363 pass(es).
[ View ]
#4 image_storage_controller-1821848-4.patch4.81 KBrbayliss
FAILED: [[SimpleTest]]: [MySQL] 38,582 pass(es), 65 fail(s), and 8 exception(s).
[ View ]

Comments

Assigned:Unassigned» japicoder

I start to work on it

Assigned:japicoder» Unassigned

Finally I'm not able to start this task. I can't figure how the ConfigStorageController is going to work and after some frustrating hours, I release to anonymous so maybe later I can do more with it or another person with more knowledge than me.

@japicoder you could take example from
#1497380: Convert shortcut sets to ConfigEntity #1552396: Convert vocabularies into configuration
or
/core/modules/views/lib/Drupal/views/ViewStorageController.php

Status:Active» Needs review
StatusFileSize
new4.81 KB
FAILED: [[SimpleTest]]: [MySQL] 38,582 pass(es), 65 fail(s), and 8 exception(s).
[ View ]

Here's a start.

+++ b/core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.phpundefined
@@ -18,7 +18,7 @@
+ *   controller_class = "Drupal\image\ImageStyleStorageController",

It seems this file is missing? Try git add .; git diff --staged

StatusFileSize
new6.43 KB
PASSED: [[SimpleTest]]: [MySQL] 49,363 pass(es).
[ View ]

Oh, right. Sorry about that.

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

The last submitted patch, image_storage_controller-1821848-6.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Configuration system, +Configurables

I remember having this conversation with timplunkett when I was doing the conversion, and we both agreed it really didn't matter whether we implement hook_ENTITY_TYPE() or extend the storage controller. I seem to remember that at the time Views was doing the former, although looking now they aren't. I also had this conversation with fubhy around #1479454: Convert user roles to configurables. I personally prefer the hook implementations because the code is in the same file as the main implementation, which makes it a lot easier to find. Is there a significant advantage either way to moving this code to the storage controller?

Hooks are easier to find...? If you look at this patch, then you see a storage controller for image styles, which contains the entire CRUD logic for image styles. Discovery of that is barely beatable. ;)

That's only the secondary advantage though. The primary advantage is that image styles are (or should not) actually be special and require tons of custom storage code — as long as image styles need that, we can be sure that almost all other config entities will need that, too.

Therefore, the other goal here is to get a better overview of what logic is in the ImageStyleStorageController, and ultimately, which parts of that could be simplified/generalized for all config entities.

One part there is definitely going to be the rename/replace logic (which wasn't moved to the storage controller in this patch yet for some reason, but it should), which is a concept that is currently known to be special for image styles, but actually, it is quite foreseeable that many other config entities will have a need for that, too.

They are easier to find if you're familiar with the architecture of entity storage. If you're not then they are incredibly difficult to find.

Anyways, I appear to be in the minority on this one, and I honestly don't have a horribly strong feeling about it. The patch is straightforward. Just gonna give the bot another run at it and if its green I'm fine.

@rbayliss: Was there any particular reason for not including the replace code in the controller?

I think we want to put that into a dedicated replace() method even (only on this particular controller for now), and call that method from the delete form submission handler, which gets the replacement ID.

Lastly, there are various coding style issues with the current patch, which we need to clean up.

@rbayliss: Was there any particular reason for not including the replace code in the controller?

Not that I can think of. I remember being confused about how the replace code fits in with the config import/export API. Other than that I guess I just didn't think to do it.

Status:Needs review» Needs work

There is now an ImageStyleStorageController, so this needs a reroll.
Though it also might need to be redone after #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) as well.

Status:Needs work» Needs review

it seems everything was fixed already, so we can close this one!

StatusFileSize
new718 bytes
PASSED: [[SimpleTest]]: [MySQL] 49,670 pass(es).
[ View ]

The only hunk that lost... Suppose this issue major because calling not-existent function

Fixed comment.

Remaining hooks uses functional style of operation
image_image_style_load() - image_effect_definition_load($effect['name'])
image_image_style_update() - depends on field module (could be fixed here)
image_image_style_delete() - uses image_image_style_update()

StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 49,642 pass(es).
[ View ]

Patch itself

I don't see the remaining parts of the patch in #6 in HEAD, so I think the entire original patch still needs to be done.

E.g., image_image_style_load() still exists, but effects should be prepared by the storage controller already, before the image style is passed to hook implementations.

Same applies to the other hooks that Image module implements for itself.

@sun agree but #19 is required, probably we have no tests for this

StatusFileSize
new7.86 KB
PASSED: [[SimpleTest]]: [MySQL] 49,308 pass(es).
[ View ]

So moved all the logic to storage controller

Also fixed flushing media for importDelete().

#22: 1821848-image-22.patch queued for re-testing.

Category:task» bug
StatusFileSize
new9.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1821848-image-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.79 KB

Now that we have a postDelete() method the whole importDelete method is superflous. I've added a test to ensure that a configImport() does the right thing for image style deletes.

Also this issue is a bug because the call to the non-existant function image_style_delete() that's currently in ImageStyleStorageController::importDelete() would cause a fatal error.

Also the @todo in the following code...

+++ b/core/modules/image/lib/Drupal/image/ImageStyleStorageController.phpundefined
@@ -16,19 +18,106 @@
   public function importDelete($name, Config $new_config, Config $old_config) {
     list(, , $id) = explode('.', $name);
     $entities = $this->load(array($id));
     $entity = $entities[$id];
-    // @todo image_style_delete() supports the notion of a "replacement style"
+    // Flush cached media for the deleted style.
+    image_style_flush($entity);
+    // @todo image_style entity supports the notion of a "replacement style"
     //   to be used by other modules instead of the deleted style. Essential!
     //   But that is impossible currently, since the config system only knows
     //   about deleted and added changes. Introduce an 'old_ID' key within
     //   config objects as a standard?
-    return image_style_delete($entity);
+    $this->replaceImageStyle($entity);
+    return TRUE;
   }

is completely spurious because when you do a delete that has a replacement style you will delete the config file... therefore you can not know the replacement style when you import to another site because the config no longer exists.

As this was a re-roll the interdiff is not a true interdiff but based on my git history...

Status:Needs review» Reviewed & tested by the community

Let's get this in

#24: 1821848-image-24.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Configuration system, +Configurables

The last submitted patch, 1821848-image-24.patch, failed testing.

The fail was in our old friend Drupal\translation_entity\Tests\EntityTestTranslationUITest... re-testing

Status:Needs work» Needs review

#24: 1821848-image-24.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Setting back to RTBC as the fail was due to a random unrelated fail... :(

#24: 1821848-image-24.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Configuration system, +Configurables

The last submitted patch, 1821848-image-24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.08 KB
PASSED: [[SimpleTest]]: [MySQL] 53,839 pass(es).
[ View ]

Rerolled due to configentity rename change

Status:Needs review» Reviewed & tested by the community

Looks great to me.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

+++ b/core/modules/image/lib/Drupal/image/ImageStyleStorageController.phpundefined
@@ -37,8 +40,64 @@ public function importDelete($name, Config $new_config, Config $old_config) {
+      image_style_flush($style);

Seems we need a follow-up to get rid of this

@andypost how come?

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