Convert image styles $style array into ImageStyle (extends ConfigEntity)

tagging

Related #1788542: Use EntityFormController and EntityListController for image styles
Could be done after this one

This seems major (at least) to me? Also #1781372: Add an API for listing (configuration) entities is in so we can use that for the image style configuration page now.

Updated issue summary.

Can we make it after feature freeze?

Demoting in favor of #1802750: [Meta] Convert configurable data to ConfigEntity system.

I'm going to take a stab at this. If there's no patch until Tuesday, feel free to take this or un-assign.

Posting a WIP patch in order to meet my own deadline. :-)

Works:
* List controller
Doesn't work:
* Form controller

Unassigning. I'll try to bring this home, but I'm not sure if I'll get to it.

Looking through this, I'm curious why you changed the path to all the admin urls? For instance, 'edit' is now 'manage', '[style]/delete' is now 'manage/[style]/delete', etc.

OK after discussing with xjm and timplunkett in IRC, I learned that it is more semantically proper and standard to go [thing]/[task] rather than the other way around (which is how it was in D7.) An open question to me though is that 'manage' seems unnecessary, we could just go '[style]/edit' etc.

I'm grabbing this, I know its marked as novice but it will provide me a good opportunity to get more well versed in config entities.

Updated issue summary.

Meta issue for these: #1802750: [Meta] Convert configurable data to ConfigEntity system

This issue also should kill one theme function - replace with listController, related issue #1805928: Convert theme_image_style_list

Added link to meta issue

Well we need the 'manage' in there, because otherwise we have the following menu routers:
admin/config/media/image-style/%image_style
admin/config/media/image-style/add

So you inherently can't name an image style 'add'. In general, I think we should try to avoid both wildcards and static paths on the same level of a menu tree. We do the same thing for node types and currently for the config test entity.

The reason I changed this in the first place is because the list controller appends '/edit' and '/delete' to the entity URIs, to create the operation links. The menu routers before were
admin/config/media/image-style/delete/%image_style
admin/config/media/image-style/edit/%image_style
so that obviously didn't work.

Edit: Where I said "config entity" before, I actually meant "config test entity".

@tstoeckler see #1588422-130: Convert contact categories to configuration system

Right, that shows that they have the same problem over there. They need to change the menu routers as well.

Added twig related issues

Here we go. As discussed elsewhere, this is just the conversion to ConfigEntity and does not include the ListController or FormController changes. There are only two other moderately significant changes here

- I removed image_style_effects() because it was totally pointless.
- I removed hook_image_style_alter(). We are going to get rid of it anyways, and it reduced the code in that area. A few months ago I did some research and to the best of my knowledge nobody is using that hook in contrib-land. That said, if someone wants to push that change off to a followup then I'm fine with that too.

Not addressed at all is that we should just remove all the image_effect_*() functions entirely. That is total followup work.

This passes all tests locally.

This all looks awesome.

+++ b/core/modules/image/image.admin.incundefined
@@ -195,40 +195,38 @@ function image_style_form_submit($form, &$form_state) {
   image_style_save($style);

@@ -270,9 +268,10 @@ function image_style_add_form_submit($form, &$form_state) {
+  image_style_save($style);

+++ b/core/modules/image/image.moduleundefined
@@ -506,8 +507,9 @@ function image_config_import_create($name, $new_config, $old_config) {
+  image_style_save($style);

@@ -519,7 +521,7 @@ function image_config_import_change($name, $new_config, $old_config) {
   return image_style_save($style);

These should all be $style->save()
(and more in the tests)

+++ b/core/modules/image/image.moduleundefined
@@ -625,22 +597,8 @@ function image_style_load($name) {
 function image_style_save($style) {
-  $config = config('image.style.' . $style['name']);
-  $is_new = $config->isNew();
-
-  $config
-    ->set('name', $style['name'])
-    ->set('label', $style['label']);
-  if (isset($style['effects'])) {
-    $config->set('effects', $style['effects']);
-  }
-  else {
-    $config->set('effects', array());
-  }
-  $config->save();
+  $style->save();
 
-  // Let other modules update as necessary on save.
-  $style['is_new'] = $is_new;
   module_invoke_all('image_style_save', $style);
 
   // Clear all caches and flush.

This should all be removed, and use hook_image_style_presave() that is triggered by ConfigStorageController::save()

+++ b/core/modules/image/image.moduleundefined
@@ -663,34 +621,18 @@ function image_style_save($style) {
 function image_style_delete($style, $replacement_style_name = '') {
   image_style_flush($style);
 
-  $config = config('image.style.' . $style['name']);
-  $config->delete();
+  $style->delete();
 
   // Let other modules update as necessary on save.
-  $style['old_name'] = $style['name'];
-  $style['name'] = $replacement_style_name;
+  if ($replacement_style_name) {
+    $style->set('name', $replacement_style_name);
+  }
   module_invoke_all('image_style_delete', $style);
 
   return TRUE;

Ideally the same for here, but I'm not sure about the replacement style stuff

+++ b/core/modules/image/lib/Drupal/image/ImageStyle.phpundefined
@@ -0,0 +1,45 @@
+  public $name;
+  ¶
+  /**
+   * The image style label.
+   *
+   * @var string
+   */
+  public $label;
+  ¶
+  /**

Trailing whitespace.

+++ b/core/modules/image/lib/Drupal/image/ImageStyle.phpundefined
@@ -0,0 +1,45 @@
+   * Implements Drupal\Core\Entity\EntityInterface::id().
+   */
+  public function id() {
+    return $this->name;

The docblock should be "Overrides Drupal\Core\Entity\Entity::id()."

But in reality, Drupal\Core\Entity\Entity::id() should be less stupid and use the entity keys...

Not completely sure what happened there

Crap I forgot about the upgrade path tests. If someone wants to pick this up at this point, feel free. I may not get back to it within a couple days.

Also, when the change notice for this is written, we should make sure and note the following: Upgrade path will migrate all styles present in the image_styles table. However, it will not update and styles supplied via hook_image_effect_info(). It will be responsibility of the contributed module to supply this style information in a YAML file in the module's config directory.

reverted changes in upgrade path and fixed small whitespace issues

Added new hooks and a bit cleaned up doc blocks
Removed image_style() image_style_save() so fixed all of #18 except image_style_delete()

Suppose we need to override delete() method of entity and introduce storage controller with this override

EDIT todo:
1) how to deal with alter hook_image_styles_alter()
2) change path for administration could be done in follow-up for form-controller admin/config/media/image-styles/edit/

+++ b/core/modules/image/image.moduleundefined
@@ -537,160 +554,71 @@ function image_config_import_delete($name, $new_config, $old_config) {
+function image_image_style_update($style) {
+  // Flush cached media for the style.
   image_style_flush($style);
-
-  return $style;

Does this get called on creates as well as saves?

I think we should leave the delete stuff for now while we work out the replacement style stuff. We have to open a followup to figure out how to deal with that in deployments, so we can address it there.

I also think we are fine removing hook_image_styles_alter() entirely. I've done a reasonably in-depth search of contrib and I haven't found any instances of it being used, and there are none in core. We have been establishing a standard that there are no alters for config as well. Dump it.

Actually shouldn't image.api.php be essentially emptied out? All usages of the hook_image_crud() hooks should move to responding to the appropriate entity hooks so are we repeating them here? They aren't really 'image api' hooks anymore. It seems like the only ones that should stay are

hook_image_effect_info()
hook_image_effect_info_alter() (both of these pending reworking of the image effect api)
hook_image_style_flush()

So other than the doc questions and the one question above I think this is ready to go. I've been testing it quite a bit in accordance with the import UI patch and things are working well.

I just talked to jhodgdon about the documentation issue, and she agrees that the hook_ENTITY_NAME_CRUD() hooks should not be documented anywhere outside of entity.api.php. Therefore here is a new patch that removes them. I also verified that an entity create will call hook_entity_name_udpate() so that is no longer an issue. I won't RTBC this patch even though its technically just a docs reroll, but I consider it so.

The only ugly part of this patch is the list(, , $id) = explode('.', $name); stuff, but that will be addressed in #1760358: Provide a way to extract the ID from a config object name.

Everything else looks great!

Updated issue summary.

+1 to RTBC, We need this get commited to proceed with
#1788542: Use EntityFormController and EntityListController for image styles
#1809376: Use EntityListController for image styles

+++ b/core/modules/image/image.moduleundefined
@@ -537,160 +554,71 @@ function image_style_delete($style, $replacement_style_name = '') {
+  $style->delete();
 
   // Let other modules update as necessary on save.
-  $style['old_name'] = $style['name'];
-  $style['name'] = $replacement_style_name;
+  if ($replacement_style_name) {
+    $style->set('name', $replacement_style_name);
+  }

Filed as follow-up #1818702: Get rid of image_style_delete() to find a proper way to implement this

#26: 1782244-image-styles-config-entity-26.patch queued for re-testing.

Great to see this one ready. Looks like we'll need a change notice since this removes the _alter() hook and other things. Committed/pushed to 8.x.

We should additionally document the conversion on http://drupal.org/node/1818734. I've added this issue there but not updated it.

I've updated all related issues with links to this issue, suppose we could change title and priority

http://drupal.org/node/1818734
http://drupal.org/node/1734556
http://drupal.org/node/1799642

http://drupal.org/node/1820974

Suppose all places are fixed, also we have a special change notice. heyrocker++

I came across #1821534: Can't delete image effects from an image style when testing #1697256: Create a UI for importing new configuration today. Most likely from this issue, I guess. Can someone take a look?

Do we still have a use case for the unique ieid ? AFAIK those were only introduced because of limitations in our former *xml* storage (couldn't handle arrays properly).
They are also very hacky and brittle: a string being a hash of effect name plus its settings - no way we can reliably generate that for any arbitrary collection of settings. Besides, it doesnt ensure uniqueness anyway you could perfectly well have the same effect with the same settings twice in the same style, if the effect is not idempotent ("darken by N%")

@sun this issue is about convertion, all other staff should be done in follow-ups as it was written in comments above.

Configurables have explicit support for renames, which is even covered by tests.

ConfigStorageController invokes the commonly known entity CRUD

hook_ENTITY_TYPE_insert() => image_image_style_save() && user_image_style_save() is not it?
hook_ENTITY_TYPE_delete() => image_image_style_delete() is not it?

This patch introduced image_style_uri(), but there is a image_style_url() already

They are very different things

$style->effects[$effect['ieid']] = $effect;

Addressed in #1821534: Can't delete image effects from an image style

PS: I update summary with all this follow-ups

Added follow-ups

Updated issue summary.

+++ b/core/modules/image/image.moduleundefined
@@ -72,6 +72,35 @@ function image_help($path, $arg) {
+ * Implements hook_entity_info().
+ */
+function image_entity_info() {
+  return array(
+    'image_style' => array(
+      'label' => t('Image style'),
+      'entity class' => 'Drupal\image\ImageStyle',
+      'controller class' => 'Drupal\Core\Config\Entity\ConfigStorageController',
+      'uri callback' => 'image_style_entity_uri',
+      'config prefix' => 'image.style',
+      'entity keys' => array(
+        'id' => 'name',
+        'label' => 'label',
+        'uuid' => 'uuid',
+      ),
+    ),
+  );

@@ -1163,31 +1129,3 @@ function _image_effect_definitions_sort($a, $b) {
-/**
- * Implements hook_entity_info().
- */
-function image_entity_info() {
-  return array(
-    'image_style' => array(
-      'label' => t('Image style'),
-      'entity class' => 'Drupal\image\ImageStyle',
-      'controller class' => 'Drupal\Core\Config\Entity\ConfigStorageController',
-      'uri callback' => 'image_style_uri',
-      'config prefix' => 'image.style',
-      'entity keys' => array(
-        'id' => 'name',
-        'label' => 'label',
-        'uuid' => 'uuid',
-      ),
-    ),
-  );

That certainly makes it hard to follow this patch. Can you at least try to make it reviewable?

So we had no test coverage for that hook whatsoever? Yikes.

sun's clear "test recipe" in #41 should make it straightforward to add test coverage for this.

#41: drupal8.image-config.41.patch queued for re-testing.

Tested as per #42
Works good, looks good

If we're fixing a regression, where are the tests?

Removing novice tags

It occurs to me that ImageAdminStylesTest::testStyleReplacement should cover the case outlined in #41. What is missing in that test?

Successfully performed the test sequence outlined in #41 on current 8.x (without this patch). That means either we need another test sequence or the problem was fixed in #1821534: Can't delete image effects from an image style (covered by tests). It also means that objections in #45, #50 are probably invalid.

Successfully performed the test sequence outlined in #41 on current 8.x with commit 0d073ce #1821534: Can't delete image effects from an image style reverted. That means that I'm sort of lost now. @sun or @xjm could you please point out what exactly needs to be tested for?

@znerol sequence from #41 needs to be implemented as test* to proof that #48 fixes the regression

@andypost: I understand. But in order to verify that the patch actually fixes the regression, we need a test that fails before applying the patch (i.e. current 8.x) and succeeds after applying the patch. However performing the steps outlined in #41 by hand on a fresh 8.x install (and even after rolling back 0d073ce) does not expose any problems. That means that those steps are not suitable to cover the regression.

Before implementing the test I have to know which sequence of steps will lead to the problem the patch tries to fix.

#48: drupal8.image-config.48.patch queued for re-testing.

patch needs re-roll

#1292470: Convert user pictures to Image Field made some of it obsolete.

Still needs tests.

#60: image-styles-1782244-60.patch queued for re-testing.

Confirm #53 Styles are replaced successfully without patch and we have a test for that replacement

Anyway patch looks a good clean-up

No tests?

testStyleReplacement() has always existed to test style replacement, which was never actually broken despite claims to the contrary (verified in #53 and #63.) Setting back to RTBC.

I just spent like 20 minutes trying to parse what the actual critical problem is here in order to determine what regression we're fixing, and it seems like there is none.

- There are steps to reproduce a bug in #41. However, those steps are refuted by later comments (#53 and #56) which are able to do those tasks successfully.
- The patch itself doesn't really contain much in the way of functional changes aside from removing image_style_delete() in favour of $style->delete() and changing hook_image_style_save() to hook_image_style_update(). One would think that if that were fixing some kind of critical bug, the existing tests in ImageAdminStylesTest.php would've been raising a stink about it, because they seem pretty thorough.

So as far as I can tell, this is just a straight-forward follow-up to improve some things, by better leveraging more of the Config Entity API and making the scope of the hook more direct. OK, great. But let's done down the drama-meter, OK? :)

Needs a re-roll.

project_issue--

Here is a reroll to HEAD. One slightly notable change: hook_config_import_delete() was removed by #1806178: Remove code duplication for hook_config_import_*() implementations of config entities, and the @todo related to it in image.api.php is also resolved from that issue, so the two related hunks are just removed.

Other than that its a straight reroll. Should go right back to RTBC if the bot passes.

Committed and pushed to 8.x. Thanks!

Major follow-up #1821848-17: Move image style load/update/delete operations into a new ImageStyleStorageController

ImageStyleStorageController::importDelete() uses removed function image_style_delete()