WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 6 days ago

#39883 assigned defect (bug)

Code hooking on `image_downsize` can no longer assume the file is an image

Reported by: stephdau Owned by: mikeschroder
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords:
Focuses: Cc:

Description (last modified by stephdau)

In r38949, Core pretty drastically changed the expectations that any code hooking onto the image_downsize filter could make until then, potentially leading to issues for integrators.

We went from having the image_downsize() function immediately return false if a file wasn't an image, not getting to that filter application at all, to only setting a variable with the result of the wp_attachment_is_image( $id ) test and now applying the filter (not even passing said result, for that matter).

This was a pretty big safe assumption to take away from under integrators' feet.

Coupled to this, a wise integrator that might have picked up on this change could have wanted to have its own code have image_downsize() still returning false if it does not want to have the file further processed by the function, but there is no opportunity to, since returning false will cause image_downsize() to keep on with its processing instead of proceeding with return $out.

Returning anything but false or null will cause image_downsize() to return, but that might not always be desirable to preserve the way WP worked prior to 4.7.

What's better here for general use isn't as clear cut as what r38949 made it to be. I've discussed the case with @mikeschroder, and we've agreed to open this ticket so we can further discuss what should be done, if anything.

Change History (13)

#1 @stephdau
6 weeks ago

  • Description modified (diff)

#2 @ocean90
6 weeks ago

  • Version changed from trunk to 4.7

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 weeks ago

#4 @mikeschroder
6 weeks ago

  • Milestone changed from Awaiting Review to 4.7.3

Adding to 4.7.x milestone to bring to the forefront for discussion.

If it looks like this is either not a thing that will change, or the change is too big for a minor, we can always re-milestone.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


5 weeks ago

#6 @joemcgill
5 weeks ago

  • Milestone changed from 4.7.3 to 4.7.4

#7 follow-up: @azaozz
4 weeks ago

I don't see a good way to fix this. The "real" regression is that image_downsize() and image_get_intermediate_size() are used for non-image attachments. This makes no sense even given the names :)

Best option imho is to introduce new function(s) specifically for handling the cases where a non-image attachment post also contains preview image, then revert the changes to the above two functions.

That will mean some code repetition, but will be many many times clearer and easier to understand and follow. The new function(s) will also have their own filters allowing plugins to better manage these cases.

Opened #39980 for this. Wondering if we should move it to the 4.7.4 milestone and close this one as duplicate, or try to fix this now and do the other for 4.8.

#8 in reply to: ↑ 7 @stephdau
4 weeks ago

Replying to azaozz:

Best option imho is to introduce new function(s) specifically for handling the cases where a non-image attachment post also contains preview image, then revert the changes to the above two functions.

As discussed, I wholeheartedly agree. Thanks @azaozz :)

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


13 days ago

#11 @mikeschroder
13 days ago

  • Owner set to mikeschroder
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


6 days ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 days ago

Note: See TracTickets for help on using tickets.