Move image style load/update/delete operations into a new ImageStyleStorageController

I start to work on it

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

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

Oh, right. Sorry about that.

#6: image_storage_controller-1821848-6.patch queued for re-testing.

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?

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.

#6: image_storage_controller-1821848-6.patch queued for re-testing.

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

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.

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

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

Patch itself

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

So moved all the logic to storage controller

Also fixed flushing media for importDelete().

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

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

Let's get this in

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

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

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

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

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

Rerolled due to configentity rename change

Looks great to me.

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?