Opened 8 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 | Owned by: | joemcgill |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | Media | Keywords: | has-patch 2nd-opinion |
Focuses: | Cc: |
Description (last modified by )
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)
Change History (32)
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#7
follow-up:
↓ 8
@
8 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
@
8 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.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by swissspidy. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#19
@
8 years ago
- Owner changed from mikeschroder to joemcgill
I wasn't able to find an elegant solution here.
Reassigning to @joemcgill for a pass.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
7 years ago
#23
@
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:
- Non-image attachment IDs are now passed to the
image_downsize
filter, where they previously weren't. - 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.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
7 years ago
#25
follow-up:
↓ 27
@
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.
#26
@
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:
↓ 28
@
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
#28
in reply to:
↑ 27
@
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.
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.