Make WordPress Core

Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#19889 closed defect (bug) (fixed)

Image Editor doesn't apply changes to custom image sizes

Reported by: otto42's profile Otto42 Owned by: wonderboymusic's profile wonderboymusic
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.3
Component: Media Keywords: has-patch 3.9-early
Focuses: Cc:

Description

The built in image editor doesn't apply changes like cropping, rotation, etc. to image sizes added using add_image_size or similar methods.

This is because the code in image-edit.php is directly looking for the image sizes in the options table, which only valid for the built-in sizes, not for custom image sizes.

Specifically, in the wp_save_image function in image-edit.php, this code is incorrect:

$crop = $nocrop ? false : get_option("{$size}_crop");
$resized = image_make_intermediate_size($new_path, get_option("{$size}_size_w"), get_option("{$size}_size_h"), $crop );

The fix to this is to make the wp_save_image function aware of custom sizes in the $_wp_additional_image_sizes global, and to use the options data as a fallback (for the built in sizes).

Patch for trunk attached. Tested on trunk install with no obvious side effects. Patched version correctly creates the custom image sizes with the applied editing changes, and deletes them properly when the image is deleted in the admin. Meta data thus created looks correct as well.

Attachments (4)

19889.diff (1.4 KB) - added by Otto42 12 years ago.
Change image-edit.php to account for additional sizes on save
19889.2.diff (1.3 KB) - added by SergeyBiryukov 12 years ago.
19889.3.diff (1.3 KB) - added by SergeyBiryukov 11 years ago.
19889.4.diff (1.4 KB) - added by SergeyBiryukov 11 years ago.

Download all attachments as: .zip

Change History (56)

@Otto42
12 years ago

Change image-edit.php to account for additional sizes on save

#1 @skithund
12 years ago

  • Cc toni.viemero@… added

#2 @SergeyBiryukov
12 years ago

  • Keywords has-patch added

#3 @studiograsshopper
12 years ago

  • Cc studiograsshopper added

#4 @dreamwhisper
12 years ago

  • Cc dreamwhisper added

#5 @SergeyBiryukov
12 years ago

  • Milestone changed from Awaiting Review to 3.5

[18996] (for #17475) introduced a bug causing custom image sizes to be removed from metadata when saving the edited image (as described in #21457).

19889.2.diff is a patch combined with the one from #21457.

#6 @prettyboymp
12 years ago

  • Cc mpretty@… added

#7 @scribu
11 years ago

  • Keywords needs-refresh close added

Patch is stale. Also, #22100 provides a more holistic approach.

#8 @scribu
11 years ago

  • Milestone 3.5 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #22100.

#9 @SergeyBiryukov
11 years ago

  • Keywords needs-refresh close removed
  • Milestone set to 3.5
  • Resolution duplicate deleted
  • Status changed from closed to reopened

#22100 is an enhancement, so not exactly a duplicate.

Given the attention to the media revamp, I guess this should be fixed in 3.5. Refreshed the patch.

#10 @nacin
11 years ago

  • Keywords close added

I don't know if this is a good thing to fix at this point. Image editing is something that largely missed the boat for 3.5, both from an API perspective (WP_Image_Editor aside) and from the UI perspective. Fixing this with #22100 — and a load of other tickets relating to cropping/editing abilities — makes sense.

#11 @markoheijnen
11 years ago

I think it's better to fix all of these issues in 3.6. I would love to see a better workflow in 3.6 where you can select the images it should apply too.

#12 @nacin
11 years ago

  • Milestone changed from 3.5 to Future Release

#13 @helen
11 years ago

#22990 was marked as a duplicate.

#14 @helen
11 years ago

  • Keywords close removed

#15 follow-up: @anatolbroder
11 years ago

Let’s talk about the responsive challenge. Imagine we need 3 different image ratios.

  1. Thumbnail (1:1)
  2. Medium (1.5:1)
  3. Large (3:1)

The most common transformation is the crop. I fact we must crop the original image manually, because the automatic crop function would just cut out the middle part of the original. This is an artistic decision. So our edit list would look like:

  • All sizes
  • Thumbnail
  • Medium
  • Large

But for every ratio we need multiple versions. Think on different screen widths and resolutions (mdpi, hdpi, xhdpi, …). So if i. e. we crop the medium ratio, we want a 480 × 320, a 720 × 480, and a 960 × 640 version of it. In fact, we register all the medium versions, but we need to crop every ratio only once.

  • Medium
    • 480 × 320
    • 720 × 480
    • 960 × 640

There are two solutions.

Sizes

The naive one is a check list (☒) with all the registered sizes.

  • All sizes
  • Thumbnail
    • 160 × 160
    • 240 × 240
    • 320 × 320
  • Medium
    • 480 × 320
    • 720 × 480
    • 960 × 640
  • Large
    • 1440 × 480
    • 2160 × 720
    • 2880 × 960

I think you can implement that one fast, till 3.6. Me, the editors of my magazine and Jon Kristian would be happy with that.

Shapes

The smart one is to go with ratios or shapes. We should not define sizes but shapes. (Call it whatever you want, my English is too bad.) The medium shape would be a group of sizes.

So we can introduce an add_image_shape function you can define the shape ratio, minimal size, etc.

add_image_shape( <name>, <ratio>, <min_width>, <max_width>, <crop>);

Then if we add a size with new_add_image_size, we could pass the defined shape through the parameter.

new_add_image_size( <name>, <shape>, <width>);

The edit list would look like.

  • All shapes
  • Thumbnail
  • Medium
  • Large

This is a different way to think. Please read the latest posts of Responsive Images Community Group if you don’t understand what I’m talking about. Maybe a new ticket is a better place to discuss that way.

#16 follow-up: @markoheijnen
11 years ago

The part for every ratio we need multiple versions isn't inside the scope of this ticket and is also something WordPress core shouldn't do. There still a lot of discussions still going and almost no one is using it.

#17 in reply to: ↑ 15 @SergeyBiryukov
11 years ago

Replying to anatolbroder:

I fact we must crop the original image manually, because the automatic crop function would just cut out the middle part of the original.

Related: #19393, #21810

Maybe a new ticket is a better place to discuss that way.

Indeed.

#18 in reply to: ↑ 16 @anatolbroder
11 years ago

  • Cc anatolbroder added

Replying to markoheijnen:

The part for every ratio we need multiple versions isn't inside the scope of this ticket and is also something WordPress core shouldn't do.

So is there another way for creating multiple resolutions of the same size right after the image manipulation? Can you provide a hook I can run to create multiple descendants from the manually edited image?

If I can only crop manually the mdpi version, but the hdpi and xhdpi versions still auto-cropped, the solution is not acceptable.

Last edited 11 years ago by anatolbroder (previous) (diff)

#19 @markoheijnen
11 years ago

This isn't something to discuss on this ticket or on trac. You should discuss this on the hackers mailinglist.
If you are creative you should read http://make.wordpress.org/core/2012/12/06/wp_image_editor-is-incoming/

#20 @SergeyBiryukov
11 years ago

  • Milestone changed from Future Release to 3.6

The issue here is not just that custom image sizes are not updated when editing, it's that they disappear from metadata when saving the edited image.

This causes the full size to be used everywhere instead of the custom size, as mentioned in #21457.

I guess #22985 is a similar issue, exacerbated by the fact that default image sizes disappear from metadata as well in 3.5.

#21 follow-up: @markoheijnen
11 years ago

It did cause disappearance in 3.4 and also in 3.5 because of the bug described in #22985. If that is fixed the data wil exists since we don't unset data anymore at that part of code.

#22 in reply to: ↑ 21 @SergeyBiryukov
11 years ago

Replying to markoheijnen:

If that is fixed the data wil exists since we don't unset data anymore at that part of code.

Confirmed that ticket:22985:22985.diff fixes the disappearance.

So, what's left is actually applying the changes to all sizes.

#23 @markoheijnen
11 years ago

#23145 was marked as a duplicate.

#24 @goto10
11 years ago

  • Cc dromsey@… added

#25 @sabreuse
11 years ago

  • Cc sabreuse added

#26 @markoheijnen
11 years ago

#23274 was marked as a duplicate.

#27 @SergeyBiryukov
11 years ago

#23860 was marked as a duplicate.

#28 follow-up: @jeremyclarke
11 years ago

  • Cc jer@… added

+1 to make this a priority. What are we supposed to do in the meantime? Does someone have a plugin hack that makes the add_image_size sizes be re-rendered when an image is cropped?

#29 @rereradu
11 years ago

  • Cc rereradu added

#30 @SergeyBiryukov
11 years ago

#24632 was marked as a duplicate.

#31 @TigerDE2
11 years ago

  • Cc tigerde2@… added

#32 @markoheijnen
11 years ago

#24717 was marked as a duplicate.

#33 @nacin
11 years ago

  • Milestone changed from 3.6 to Future Release

Not new — maybe 3.7.

#34 @SergeyBiryukov
11 years ago

  • Version changed from 3.3.1 to 3.3

#35 @mau
11 years ago

  • Cc ngomau@… added

#36 @tollmanz
11 years ago

  • Cc tollmanz@… added

#37 @SergeyBiryukov
11 years ago

  • Milestone changed from Future Release to 3.7

Refreshed for 3.7.

#38 @nacin
10 years ago

  • Milestone changed from 3.7 to 3.8

I like this in general, but this needs to happen at the beginning of a cycle.

#39 @nacin
10 years ago

Also, does this change UI expectations?

#40 @Otto42
10 years ago

I think the latest patch for this echoes the original intent of the ticket, and doesn't break anything in any obvious enough ways to matter. The goal of this ticket was to have the image editor create the expected image sizes when the image editor is used, even when image sizes are added by the theme or plugins. If the theme or plugins are adding sizes, then editing an image should produce correct and expected results with those added sizes.

This is still a valid bugfix, as opposed to some new feature. Later additions may improve or enhance this, but it's still in the realm of a problem with how the image editor does not recognize additional sizes.

#41 @richardmtl
10 years ago

  • Cc richard@… added

#42 @wonderboymusic
10 years ago

  • Keywords 3.9-early added
  • Milestone changed from 3.8 to Future Release

#43 in reply to: ↑ 28 @ejdanderson
10 years ago

Replying to jeremyclarke:

+1 to make this a priority. What are we supposed to do in the meantime? Does someone have a plugin hack that makes the add_image_size sizes be re-rendered when an image is cropped?

The current workaround is to just filter the height/width options as they're requested. It attempts to process the image sizes but the values for their height and width are empty.

#44 @markoheijnen
10 years ago

#24997 was marked as a duplicate.

#45 @markoheijnen
10 years ago

#16203 was marked as a duplicate.

#46 @SergeyBiryukov
10 years ago

  • Milestone changed from Future Release to 3.9

#47 @lkraav
10 years ago

  • Cc leho@… added

#48 @kraftner
10 years ago

  • Cc thomas@… added

This ticket was mentioned in IRC in #wordpress-dev by DH-Shredder. View the logs.


10 years ago

#50 @kirasong
10 years ago

If we're going to do this, I'd argue we should land it right away.
If it waits until beta 2+, we should likely leave this alone until next time around.

#51 @wonderboymusic
10 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from reopened to closed

In 27522:

The Image Editor should apply changes to custom image sizes by checking $_wp_additional_image_sizes for $size in wp_save_image() before arbitrarily going to the options table.

Props Otto42, SergeyBiryukov.
Fixes #19889.

This ticket was mentioned in IRC in #wordpress-dev by gordian. View the logs.


10 years ago

Note: See TracTickets for help on using tickets.