WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 2 years ago

#28077 new enhancement

Use exif_imagetype instead of getimagesize in file_is_displayable_image

Reported by: joehoyle Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

From http://us3.php.net/manual/en/function.exif-imagetype.php:

exif_imagetype — Determine the type of an image

When a correct signature is found, the appropriate constant value will be returned otherwise the return value is FALSE. The return value is the same value that getimagesize() returns in index 2 but exif_imagetype() is much faster.

For whatever reason, getimagesize also fails to read the type from a Custom Stream Wrapper, whereas exif_imagetype reports that correctly.

Attachments (3)

file_is_displayable_image.diff (812 bytes) - added by joehoyle 4 years ago.
28077.diff (4.6 KB) - added by joehoyle 2 years ago.
28077.2.diff (4.7 KB) - added by joehoyle 2 years ago.

Download all attachments as: .zip

Change History (13)

#1 @jdgrimes
4 years ago

You need to check if is_callable( 'exif_imagetype' ) and fall back to getimagesize() if not, because the exif extension may not be loaded. That may negate the speed increases.

#2 follow-up: @jdgrimes
4 years ago

Also, I don't think we need the @ when calling exif_imagetype(). getimagesize() can emit a warning on error, which is why it is being suppressed. But exif_imagetype() appears to only emit a notice, which will be silenced by WordPress unless WP_DEBUG is enabled, in which case it may prove useful to let it give a notice on failure. Thoughts?

(I believe there are some places in core where error suppression is used only if WP_DEBUG is false, so maybe getimagesize() should be suppressed only conditionally here as well. ?)

#3 in reply to: ↑ 2 @joehoyle
4 years ago

Replying to jdgrimes:

(I believe there are some places in core where error suppression is used only if WP_DEBUG is false, so maybe getimagesize() should be suppressed only conditionally here as well. ?)

Yup I'd agree with that.

You need to check if is_callable( 'exif_imagetype' ) and fall back to getimagesize() if not, because the exif extension may not be loaded. That may negate the speed increases.

Ahh, good point - perhaps a static variable to reduce repetition of the check? Though I'd imagine is_callable() maybe internally caches that.

I finally tracked out why getimagesize fails on a Stream, it's if the Stream is not seekable, as getimagesize needs to read a much larger chunk of the file - hence why exif_imagetype is a lot faster I would presume.

There is probably another 5+ calls to getimagesize that could be migrated to exif_imagetype, so I'd only be interested in doing that if it's going to be "worth" it. At this point it's seeming like a micro optimisation that may not be necessary.

Cons:

  • Needs to check if exif_imagetype and fallback anyway
  • Slightly higher code-complexity for little gains

Pros:

  • Makes plugins with non seekable streams (AWS SDK for S3 for example) work
  • Slightly less disk / network IO
  • Slightly faster

I'd be happy if a more experience core contributor decides this is not worth and closes this ticket :)

#4 @ericlewis
3 years ago

  • Keywords needs-refresh added

Sounds like a minor improvement, but definitely a safer bet than getimagesize(). Anyone else into this?

#5 @ericlewis
3 years ago

  • Version changed from 3.9 to 2.5

#6 @ericlewis
3 years ago

  • Severity changed from normal to minor

#7 @chriscct7
2 years ago

  • Severity changed from minor to normal

@joehoyle
2 years ago

#8 @joehoyle
2 years ago

  • Keywords has-patch has-unit-tests added; needs-refresh removed

Added patch with a new function wp_get_image_type which returns the IMAGE_TYPE constant for an image, which wraps the usage of exif_imagetype when available, also added tests.

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


2 years ago

@joehoyle
2 years ago

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


2 years ago

Note: See TracTickets for help on using tickets.