Image.php - Wrap Error Suppressors in WP_DEBUG Conditionals

[Howdy_McGee]

Ideally, we want to remove all PHP error suppression flags. In some cases this may not be optimal so an alternative is to wrap any necessary suppressors in a WP_DEBUG conditional:

if( defined( 'WP_DEBUG' ) && WP_DEBUG ) {
	$imagesize = getimagesize( $file );
} else {
	$imagesize = @getimagesize( $file );
}

This ticket is to track anything and everything we can do to minimize the suppression flags in the image.php file.

[Howdy_McGee]

WP_DEBUG Wrapped

[Howdy_McGee]

This patch wraps all @ suppressed methods with WP DEBUG conditionals. If anyone has ideas to further minimize the suppressed methods please submit an additive patch. Related to #24780 ( ​Spreadsheet )

[SergeyBiryukov]

Thanks for the patch!

With ~15 instances of @getimagesize() in core, I think it would make sense to introduce wp_get_image_size() as a wrapper with the defined( 'WP_DEBUG' ) && WP_DEBUG check, to avoid repetition.

@iptcparse() and @exif_read_data(), on the other hand, are only used once or twice, so probably don't need a wrapper at this point.

[Howdy_McGee]

This patch introduces a new function wp_getimagesize( $filename, &$imageinfo ); and functions exactly like PHP getimagesize(). The 15 instances of getimagesize() across 8 files has also been switched over to the new wp_getimagesize() function. Additionally, the instance of iptcparse() and exif_read_data() were left with the WP_DEBUG conditional check. Let me know if we want to split this patch down into a per-file patch.

[Howdy_McGee]

Introduces wp_getimagesize()

[mukesh27]

Hi there, Can you please add version number for the new function doc block as the ticket is set to the milestone 5.5?

/**
 * Allow PHPs getimagesize() to be debuggable when necessary.
 *
 * @since 5.5.0
 * 
 * @param string $filename      The file path.
 * @param array $imageinfo      Extended image information.
 * @return array|false Array of image information or false on failure. 
 */
function wp_getimagesize( $filename, &$imageinfo = array() ) {

[Howdy_McGee]

@since 5.5.0

[mukesh27]

Thanks for the update @Howdy_McGee. Patch 49889-3.diff looks fine for me

[antpb]

This issue was discussed in the recent Media meeting by @sageshilling @pbiron and myself. It was agreed that the most recent patch looks good and is probably ready for inclusion. Some testing should be done to ensure that it works properly, but from a high level view it looks right. Will go ahead and remove dev-feedback and keep this on our radar for commit in 5.5.

Thank you!

[davidbaumwald]

The new function probably deserves a small call-out on the Misc Dev note.

[davidbaumwald]

With Beta 1 for 5.5 landing in just a few hours, this is being moved to Future Release. If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

Image.php - Wrap Error Suppressors in WP_DEBUG Conditionals

Trac ticket: https://core.trac.wordpress.org/ticket/49889

[SergeyBiryukov]

49889.4.diff​ refreshes the patch and fixes some coding standard issues.

However, there are still ​18 test errors with this change. It's likely that the tests need some adjustment.

Moving to the next release for now.

[noisysocks]

Adding needs-unit-tests to reflect that tests need to be updated.

[antpb]

One note that @hellofromtonya mentioned in the recent Media Meeting that in PHP 8 the @ suppression of errors may have different behavior. Just reading through this article: ​https://php.watch/versions/8.0/fatal-error-suppression

In my digging I found

In PHP 8.0, the @ operator does not suppress certain types of errors that were silenced prior to PHP 8.0. This includes the following types of errors:
E_ERROR - Fatal run-time errors.
E_CORE_ERROR - Fatal errors occurred in PHP's initial startup.
E_COMPILE_ERROR - Fatal compile-time errors (from Zend engine).
E_USER_ERROR - User-triggered errors with trigger_error() function.
E_RECOVERABLE_ERROR - Catchable fatal error.
E_PARSE - Compile-time parse errors.

So I think that the suppression here is safe, but we might want to consider why we're suppressing the errors for these and document it for future folks that are curious. I wasnt able to find why @getimagesize is being supressed.

[hellofromTonya]

Capturing PHP 8 notes from conversation with @jrf. She has reviewed the @ suppression for getimagesize():

Ok, I've just had a look.
References:

• ​https://www.php.net/manual/en/function.getimagesize.php#refsect1-function.getimagesize-errors
• ​https://wiki.php.net/rfc/engine_warnings#proposed_classification

At first glance, this error suppression operator looks to be justified and will not cause any PHP 8 issues, though I'd recommend doing some testing with 3v4l or another tool to verify.

Trac ticket: https://core.trac.wordpress.org/ticket/49889

• Applies 49889.4.diff​ patch
• Fixes failing tests by skipping debug mode when running unit tests (ie suppress errors during tests)
• Ignores phpcs NoSilencedErrors sniff for intentional error suppressions and adds reason why
• Changes @since to 5.7.0

[hellofromTonya]

Removing needs-unit-tests as PR 927 resolves the failing tests. How? When the tests are running, skips the unsilenced code, i.e. the for debug mode code, and uses the production code. Uses the same strategy in the REST API of checking defined( 'DIR_TESTDATA' ).

[hellofromTonya]

@antpb @SergeyBiryukov This ticket is for ready for your code review. Marking it for commit consideration.

[antpb]

In 50146:

Media: Avoid suppressing errors when using getimagesize().

Previously, all logic utilizing getimagesize() was supressing errors making it difficult to debug usage of the function.

A new wp_getimagesize() function has been added to allow the errors to no longer be suppressed when WP_DEBUG is enabled.

Props Howdy_McGee, SergeyBiryukov, mukesh27, davidbaumwald, noisysocks, hellofromTonya.
Fixes #49889.

[antpb]

Just marking here that we still need dev note for this one in 5.7 :)

Closed in changeset https://core.trac.wordpress.org/changeset/50146

Closed in changeset https://core.trac.wordpress.org/changeset/50146

Closed in changeset https://core.trac.wordpress.org/changeset/50146

[SergeyBiryukov]

In 50148:

Media: Move wp_getimagesize() to wp-includes/media.php, for consistency with other media functions.

Follow-up to [50146].

See #49889.

[SergeyBiryukov]

Checking DIR_TESTDATA seems a bit random, could we check WP_RUN_CORE_TESTS instead?

[hellofromTonya]

@SergeyBiryukov Yes that is more clear. I had opted to use what already existed by following the pattern in ​`WP_REST_Attachments_Controller`.

We could standardize on WP_RUN_CORE_TESTS. I think that's a better approach.

[hellofromTonya]

Changes test running constant from DIR_TESTDATA to WP_RUN_CORE_TESTS.

[antpb]

In 50170:

Media: Consistency in logic to pass wp_getimagesize() tests.

Previously, we used DIR_TESTDATA to determine if a test should skip a newly silenced error in wp_getimagesize().

We are now using WP_RUN_CORE_TESTS instead for consistency.

Props hellofromTonya, SergeyBiryukov.
See #49889.

[antpb]

I think we can close this now that the proper check is in.

[kraftbj]

~Are there any particular downsides to setting WP_RUN_CORE_TESTS when running plugin unit tests? The error suppression for getimagesize was expected for one of our Jetpack tests, so looking to see if setting the constant is good enough or if we need to rewrite some tests :)~

Disregard. We're going to have to rewrite the tests.

[audrasjb]

Dev note published: ​https://make.wordpress.org/core/2021/02/23/miscellaneous-developer-focused-changes-in-wordpress-5-7/