WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

Last modified 23 months ago

#42480 closed defect (bug) (fixed)

Consistent suppression of `getimagesize()` errors

Reported by: jeremyfelt Owned by: joemcgill
Milestone: 4.9.2 Priority: normal
Severity: normal Version: 2.5
Component: Media Keywords: good-first-bug has-patch commit
Focuses: Cc:
PR Number:

Description

In general, it seems common in the PHP world to suppress getimagesize() errors because images and the software creating them can be flaky. The function has a tendency to generate errors like getimagesize(): corrupt JPEG data: 7191 extraneous bytes before marker even when it's able to provide information that you're looking for.

We use getimagesize() 15 times in WP core. In 7 of these cases, we suppress errors with @getimagesize(). By proxy, we usually suppress two more of these cases by suppressing @wp_read_image_metadata() 3 out of 4 times.

Most uses of getimagesize() are to retrieve basic file information like size. We do also use it in the ID3 library and in wp_read_image_metadata() to retrieve IPTC data. It's strange enough (IMO) that getimagesize() is the only PHP function that handles this, but that's what we have.

I think there are probably two options:

  • Suppress errors from wp_read_image_metadata() when used in wp_generate_attachment_metadata(). This would align with the other uses in core.
  • Remove error suppression when calling wp_read_image_metadata() and instead suppress errors on problematic internal PHP functions directly. This *could* be safer as it would unmask other possible errors generated in WordPress core code.

Attachments (3)

42480.diff (2.8 KB) - added by chasewg 2 years ago.
First shot at a patch
42480.2.diff (2.8 KB) - added by joemcgill 2 years ago.
42480.3.diff (2.7 KB) - added by jeremyfelt 2 years ago.

Download all attachments as: .zip

Change History (19)

#1 follow-up: @joemcgill
2 years ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to 4.9.1

Hi @jeremyfelt. Thanks for the report and welcome to Trac! Your second suggestion seems like the right approach here. Let's get this in the next minor.

#2 @joemcgill
2 years ago

  • Type changed from enhancement to defect (bug)

#3 in reply to: ↑ 1 @jeremyfelt
2 years ago

  • Keywords dev-feedback removed
  • Version set to 2.5

Replying to joemcgill:

Hi @jeremyfelt. Thanks for the report and welcome to Trac! Your second suggestion seems like the right approach here. Let's get this in the next minor.

+1 I like that option the best as well.

It looks like possible problematics are getimagesize(), iptcparse(), and exif_read_data(). I've only done a deep dive in the PHP internals for getimagesize(), so I'm not sure how the others react with error-ish data. There may be others in there as well.

@chasewg
2 years ago

First shot at a patch

#4 @chasewg
2 years ago

First patch so any comments or corrections are greatly appreciated!

#5 @johnbillion
2 years ago

  • Milestone changed from 4.9.1 to 5.0

#6 @joemcgill
2 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Owner set to joemcgill
  • Status changed from new to accepted

#7 @jeremyfelt
2 years ago

  • Milestone changed from 5.0 to 4.9.2

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


2 years ago

@joemcgill
2 years ago

#9 @joemcgill
2 years ago

42480.2.diff refreshes 42480.diff.

Also worth noting that @joehoyle filed a bug upstream with PHP – see: https://bugs.php.net/bug.php?id=75708

@jeremyfelt or @joehoyle, could you confirm whether this fixes the issue for you?

#10 @blobfolio
2 years ago

For something completely different, I ended up having to write a wrapper for getimagesize() to address various bugs and shortcomings across PHP versions. That effort is part of #35725, for anyone interested. As a consequence of redirecting Core calls to the wrapper, all error suppression behaviors are consistent. ;)

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


2 years ago

@jeremyfelt
2 years ago

#12 @jeremyfelt
2 years ago

@joemcgill Close, but there was one more getimagesize() that needed to be silenced for our errors to go away. That's updated in 42480.3.diff.

There are a handful of other non-suppressed getimagesize() calls left in core, though they're only ever called with one parameter. It seems to be only when called with two that it's showing the problem we're seeing (confirmed in the PHP bug report).

I'm +1 on 42480.3.diff, though it sounds like @blobfolio's patch on #35725 could be interesting for the future.

#13 @joemcgill
2 years ago

  • Keywords commit added; needs-testing removed

Thanks @jeremyfelt. I agree that the patch on #35725 is an interesting enhancement for the future, but let's go ahead and get this in.

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


2 years ago

#15 @SergeyBiryukov
23 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 42449:

Media: Bring consistency to getimagesize() error suppression.

Props chasewg, joemcgill, jeremyfelt.
Fixes #42480.

#16 @SergeyBiryukov
23 months ago

In 42450:

Media: Bring consistency to getimagesize() error suppression.

Props chasewg, joemcgill, jeremyfelt.
Merges [42449] to the 4.9 branch.
Fixes #42480.

Note: See TracTickets for help on using tickets.