When an image style is deleted you're given the opportunity to choose a new style to replace the one that is being deleted, modules are then given the opportunity to react to this change. Image module should be responding on behalf of image fields and updating each field instances display settings as needed.

Steps to repeat the problem.

1. Add a custom image style
2. Use the newly created image style as the formatter for an image field. For example the field_image field on the article node type.
3. Delete the newly created image style, when presented with the option to choose a replacement style choose 'thumbnail' from the list.

Desired effect: Custom image style is deleted and the field_image formatter that was using it is now updated to use the 'thumbnail' formatter.

Actual effect: Custom image style is deleted, field_image formatter is reset to default format.

The attached patch attempts to fix this problem however I'm running into a couple of issues.

First. image_style_delete() invokes hook_image_style_delete after the custom style has already been deleted so the new image_image_style_delete() function has no way to know which fields were using the old style as they are already reset to default. So, I've moved the hook invocation earlier in the function. Which is working.

Second. While the attached implementation of image_image_style_delete() does the trick, for some reason the custom style isn't deleted the first time you click delete (formatters are updated as expected). You have to click it a second time and go through the process again before it is deleted. This seems to be caused by the call to field_update_instance() -> _field_write_instance(), however I can't figure out why. Any help would be appreciated.

Finally, this patch depends on #601966: Tests for image field as it extends the tests in that patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naxoc’s picture

I can't see a patch - did you forget to upload it? ;)

eojthebrave’s picture

Apparently I did. Here it is.

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
4.04 KB

Tests are passing locally now, lets see what the bot has to say about it.

cosmicdreams’s picture

I'm subscribing so I can test this patch tomorrow.

cosmicdreams’s picture

After I deleted the custom image style in my first test, my content type (Article) displayed a image style set at

<no preview>
cosmicdreams’s picture

Status: Needs review » Needs work

flushed cache, applied database update, ran cron, tried again.

By following the testing procedure described by the OP, I was able to still see this bug happen. I'll switch this back to needs works as it doesn't seem to be fixed yet.

andypost’s picture

subscribe

andypost’s picture

Patch does not work or me!

Delete happens only if I visit delete twice, after first attempt only changed display style, after second delete happens and as #5

Also when preset is renamed all settings are reverts to "Image" and preview set to ""

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
11.18 KB

Here's another go at this, found another issues while trying to figure this out the solution for which is wrapped in to this patch since it is necessary in order to make it work.

image_style_load() should allow you to load an image style by it's unique numeric ID but doesn't. The function locates but never returns the appropriate $style array.

This version of the patch also adds support for renaming a style via hook_image_style_save().

chx expressed concern in IRC about this line:

if (preg_match('/__([a-z0-9_]+)/', $display['type'], $matches)) {

Which is used to determine which style an image field is using. I'm not entirely sure what he didn't like about it. My argument is that it is the same way we are detecting which style an image field uses in image_field_formatter_view() and for consistency it should be done the same way.

yched’s picture

+++ modules/image/image.module
@@ -354,6 +354,39 @@ function image_image_default_styles() {
+        foreach ($instance['display'] as $display_name => $display) {

Minor : please name the var $view_mode instead of $display_name.

About the regexp : true, that's the way to do it currently. The style name is buried in the string name of the formatter. In an ideal world, there would just be a single 'use_image_style' formatter with the style name as a formatter setting, but we could not come up with a UI for formatter settings, so they are as good as dead in current D7...

Powered by Dreditor.

eojthebrave’s picture

Changes $display_name to $view_mode as per #10.

andypost’s picture

Great now it works!

I just added a change of 'preview_image_style' and a bit optimizations - $instance should not be saved in loop!

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

worked for me!!!! so happy this worked.

eojthebrave’s picture

The fix to image_style_load() in the patch above also fixes an issue where overriding an image style defined in code that has more than one image effect causes a WSOD. The problem is that image_effect_save() is called twice and as a result image_style_flush() is also called twice. The call to image_style_save() inside of image_effect_save() tries to load the style by ID and fails so image_style_flush just recursively deletes the entire files/style/* directory in the first call, and then tries to delete it again on the second call. Can't find it, and blam!

You can test this by creating a module which implements hook_image_styles_alter() and adds a second effect to one of the existing thumbnails, or by implementing hook_image_default_styles() and adding a new style with two or more effects. See: #750238: image_example.module demonstrates use of Drupal 7 Image styles and effects. And then clicking the "Override" option for a style with two or more effects.

sun’s picture

Critical bump.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

can't apply image.test part, needs a reroll :)

andypost’s picture

Status: Needs work » Needs review
FileSize
11.78 KB

Re-roll with changes from HEAD

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD. Thanks!

Status: Fixed » Closed (fixed)

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