WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 4 months ago

Last modified 2 months ago

#19889 closed defect (bug) (fixed)

Image Editor doesn't apply changes to custom image sizes

Reported by: Otto42 Owned by: 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 2 years ago.
Change image-edit.php to account for additional sizes on save
19889.2.diff (1.3 KB) - added by SergeyBiryukov 2 years ago.
19889.3.diff (1.3 KB) - added by SergeyBiryukov 20 months ago.
19889.4.diff (1.4 KB) - added by SergeyBiryukov 11 months ago.

Download all attachments as: .zip

Change History (56)

Otto422 years ago

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

comment:1 skithund2 years ago

  • Cc toni.viemero@… added

comment:2 SergeyBiryukov2 years ago

  • Keywords has-patch added

comment:3 studiograsshopper2 years ago

  • Cc studiograsshopper added

comment:4 dreamwhisper2 years ago

  • Cc dreamwhisper added

SergeyBiryukov2 years ago

comment:5 SergeyBiryukov2 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.

comment:6 prettyboymp2 years ago

  • Cc mpretty@… added

comment:7 scribu22 months ago

  • Keywords needs-refresh close added

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

comment:8 scribu20 months ago

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

Duplicate of #22100.

SergeyBiryukov20 months ago

comment:9 SergeyBiryukov20 months 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.

comment:10 nacin20 months 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.

comment:11 markoheijnen20 months 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.

comment:12 nacin20 months ago

  • Milestone changed from 3.5 to Future Release

comment:13 helen19 months ago

#22990 was marked as a duplicate.

comment:14 helen19 months ago

  • Keywords close removed

comment:15 follow-up: anatolbroder19 months 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.

comment:16 follow-up: markoheijnen19 months 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.

comment:17 in reply to: ↑ 15 SergeyBiryukov19 months 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.

comment:18 in reply to: ↑ 16 anatolbroder19 months 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 manually crop only the mdpi version, but the hdpi and xhdpi versions still auto-cropped, the solution not acceptable.

Version 0, edited 19 months ago by anatolbroder (next)

comment:19 markoheijnen19 months 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/

comment:20 SergeyBiryukov19 months 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.

comment:21 follow-up: markoheijnen19 months 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.

comment:22 in reply to: ↑ 21 SergeyBiryukov19 months 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.

comment:23 markoheijnen18 months ago

#23145 was marked as a duplicate.

comment:24 goto1018 months ago

  • Cc dromsey@… added

comment:25 sabreuse18 months ago

  • Cc sabreuse added

comment:26 markoheijnen18 months ago

#23274 was marked as a duplicate.

comment:27 SergeyBiryukov16 months ago

#23860 was marked as a duplicate.

comment:28 follow-up: jeremyclarke15 months 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?

comment:29 rereradu14 months ago

  • Cc rereradu added

comment:30 SergeyBiryukov13 months ago

#24632 was marked as a duplicate.

comment:31 TigerDE213 months ago

  • Cc tigerde2@… added

comment:32 markoheijnen12 months ago

#24717 was marked as a duplicate.

comment:33 nacin12 months ago

  • Milestone changed from 3.6 to Future Release

Not new — maybe 3.7.

comment:34 SergeyBiryukov12 months ago

  • Version changed from 3.3.1 to 3.3

comment:35 mau12 months ago

  • Cc ngomau@… added

comment:36 tollmanz11 months ago

  • Cc tollmanz@… added

SergeyBiryukov11 months ago

comment:37 SergeyBiryukov11 months ago

  • Milestone changed from Future Release to 3.7

Refreshed for 3.7.

comment:38 nacin9 months 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.

comment:39 nacin9 months ago

Also, does this change UI expectations?

comment:40 Otto429 months 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.

comment:41 richardmtl8 months ago

  • Cc richard@… added

comment:42 wonderboymusic8 months ago

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

comment:43 in reply to: ↑ 28 ejdanderson7 months 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.

comment:44 markoheijnen6 months ago

#24997 was marked as a duplicate.

comment:45 markoheijnen6 months ago

#16203 was marked as a duplicate.

comment:46 SergeyBiryukov6 months ago

  • Milestone changed from Future Release to 3.9

comment:47 lkraav6 months ago

  • Cc leho@… added

comment:48 kraftner6 months ago

  • Cc thomas@… added

comment:49 ircbot4 months ago

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

comment:50 DH-Shredder4 months 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.

comment:51 wonderboymusic4 months 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.

comment:52 ircbot2 months ago

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

Note: See TracTickets for help on using tickets.