Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#39883 assigned defect (bug)

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

Reported by: stephdau's profile stephdau Owned by: joemcgill's profile joemcgill
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords: has-patch 2nd-opinion
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.

Attachments (2)

39883.diff (1001 bytes) - added by joemcgill 7 years ago.
39883.2.diff (1000 bytes) - added by joemcgill 7 years ago.

Download all attachments as: .zip

Change History (32)

#1 @stephdau
7 years ago

  • Description modified (diff)

#2 @ocean90
7 years ago

  • Version changed from trunk to 4.7

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


7 years ago

#4 @kirasong
7 years 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.


7 years ago

#6 @joemcgill
7 years ago

  • Milestone changed from 4.7.3 to 4.7.4

#7 follow-up: @azaozz
7 years 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
7 years 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.


7 years ago

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


7 years ago

#11 @kirasong
7 years ago

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

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


7 years ago

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


7 years ago

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


7 years ago

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


7 years ago

#16 @swissspidy
7 years ago

  • Keywords needs-patch added
  • Milestone changed from 4.7.4 to 4.7.5

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


7 years ago

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


7 years ago

#19 @kirasong
7 years ago

  • Owner changed from mikeschroder to joemcgill

I wasn't able to find an elegant solution here.

Reassigning to @joemcgill for a pass.

#20 @kirasong
7 years ago

  • Milestone changed from 4.7.5 to 4.8

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


7 years ago

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


7 years ago

@joemcgill
7 years ago

#23 @joemcgill
7 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

[Update: fixed link to correct patch]

As I understand it, there were essentially two main problems introduced in r38949:

  1. Non-image attachment IDs are now passed to the image_downsize filter, where they previously weren't.
  2. Non-supported attachment types trigger unnecessary DB calls when attempting to get _wp_attached_file and _wp_attachment_metadata types.

Of those two, the second seems to be the most important to address, since there's no way for a developer to avoid this on their own, as @stephdau helpfully explained in the description of this issue.

39883.2.diff resolves the second problem and partially addresses the first by only passing supported types to the filter. Here are my current assumptions:

  • wp_get_attachment_image_src() (and related functions) should be able to take a post ID for a PDF and produce image attributes (Since 4.7).
  • Only supported attachment types (i.e., image and pdf) should pass to image_downsize filter.
  • image_downsize() should bail early when the attachment type isn't a supported type so we can avoid extra DB hits when getting post meta for _wp_attached_file or _wp_attachment_metadata.

For now, I think it makes sense to continue passing PDFs to the image_downsize filter so integrators can still rewrite URLs for PDF previews with CDN addresses (which seems to be a common use case for this filter). In the future, It makes sense to consider splitting all PDF functionality into a separate set of functions to avoid confusion with the image functions, as described in #39980. Additionally, we should avoid adding additional filters for PDF handling until we can consider approaches to #39980.

Last edited 7 years ago by joemcgill (previous) (diff)

@joemcgill
7 years ago

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


7 years ago

#25 follow-up: @stephdau
7 years ago

In 39883.2.diff, we should make $supported_types = array( 'image', 'pdf' ); filterable. In our install, we currently do not support PDF thumbnails, for example, and that would let us short-circuit the function when facing a PDF. It would also let integrators add their own supported types, should they need to.

Last edited 7 years ago by stephdau (previous) (diff)

#26 @azaozz
7 years ago

Frankly I don't like the approach in 39883.2.diff. It feels more like a temp hack rather than a fix, still leaves a "backdoor" to sneak in PDF attachments to be treated as images. For that reason it doesn't fix this ticket either.

Needing to add a filter would only make things worse, with one plugin possibly adding another type that is unexpected in another plugin :(

Think this should be fixed properly, by adding the missing handler functions for this case, possibly in #39980.

This doesn't seem to affect many sites, so perhaps consider punting and fixing early in 4.9?

#27 in reply to: ↑ 25 ; follow-up: @kirasong
7 years ago

Replying to stephdau:

In 39883.2.diff, we should make $supported_types = array( 'image', 'pdf' ); filterable. In our install, we currently do not support PDF thumbnails, for example, and that would let us short-circuit the function when facing a PDF. It would also let integrators add their own supported types, should they need to.

This was the only thing that stood out to me when reviewing as well.

I like the idea of having a filterable list of types that are supported for thumbnail creation -- in particular because I can see a future where we might want to add additional types to this.

[edit] Would it need to be more granular than "image", since image represents more than one file type? edit

Last edited 7 years ago by kirasong (previous) (diff)

#28 in reply to: ↑ 27 @stephdau
7 years ago

Replying to mikeschroder:

[edit] Would it need to be more granular than "image", since image represents more than one file type? edit

Probably not if we leave the other one as 'pdf'. It's a shame neither match the same part of the mime-type format (image/jpg vs application/pdf), but going with mime-types will lead to that list getting out of hand pretty quick. In that light, I'd leave it as 'image', 'pdf', which is clear and concise. Custom additions would be left to the integrator's choice.

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


7 years ago

#30 @obenland
7 years ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.