Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#49889 closed enhancement (fixed)

Image.php - Wrap Error Suppressors in WP_DEBUG Conditionals

Reported by: howdy_mcgee's profile Howdy_McGee Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-dev-note
Focuses: coding-standards Cc:

Description

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.

Attachments (5)

49889.diff (3.2 KB) - added by Howdy_McGee 4 years ago.
WP_DEBUG Wrapped
49889-2.diff (7.7 KB) - added by Howdy_McGee 4 years ago.
Introduces wp_getimagesize()
49889-3.diff (7.7 KB) - added by Howdy_McGee 4 years ago.
@since 5.5.0
49889.4.diff (7.8 KB) - added by SergeyBiryukov 4 years ago.
49889.5.diff (1.4 KB) - added by hellofromTonya 3 years ago.
Changes test running constant from DIR_TESTDATA to WP_RUN_CORE_TESTS.

Download all attachments as: .zip

Change History (47)

@Howdy_McGee
4 years ago

WP_DEBUG Wrapped

#1 @Howdy_McGee
4 years ago

  • Keywords has-patch dev-feedback added

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 )

#2 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5

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.

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


4 years ago

#4 @Howdy_McGee
4 years ago

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
4 years ago

Introduces wp_getimagesize()

#5 @mukesh27
4 years ago

  • Keywords needs-refresh added

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
4 years ago

@since 5.5.0

#6 @mukesh27
4 years ago

  • Keywords needs-refresh removed

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

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


4 years ago

#8 @antpb
4 years ago

  • Keywords dev-feedback removed

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!

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


4 years ago

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


4 years ago

#11 @davidbaumwald
4 years ago

  • Keywords needs-dev-note added

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

#12 @davidbaumwald
4 years ago

  • Milestone changed from 5.5 to Future Release

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.

#13 @SergeyBiryukov
4 years ago

  • Milestone changed from Future Release to 5.6

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


4 years ago

#15 @SergeyBiryukov
4 years ago

  • Keywords commit added
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in PR #561 on WordPress/wordpress-develop by dream-encode.


4 years ago
#16

Image.php - Wrap Error Suppressors in WP_DEBUG Conditionals

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

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


4 years ago

#18 @SergeyBiryukov
4 years ago

  • Keywords commit removed
  • Milestone changed from 5.6 to 5.7

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.

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


3 years ago

#20 @noisysocks
3 years ago

  • Keywords needs-unit-tests added

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

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


3 years ago

#22 @antpb
3 years ago

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.

This ticket was mentioned in Slack in #core-php by hellofromtonya. View the logs.


3 years ago

#24 @hellofromTonya
3 years ago

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

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

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.

This ticket was mentioned in PR #927 on WordPress/wordpress-develop by hellofromtonya.


3 years ago
#25

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

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


3 years ago

#27 @hellofromTonya
3 years ago

  • Keywords needs-unit-tests removed

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' ).

#28 @hellofromTonya
3 years ago

  • Keywords commit added

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

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


3 years ago

#30 @antpb
3 years ago

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

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.

#31 @antpb
3 years ago

  • Keywords commit removed

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

#35 @SergeyBiryukov
3 years ago

In 50148:

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

Follow-up to [50146].

See #49889.

#36 @SergeyBiryukov
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#37 @hellofromTonya
3 years ago

@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
3 years ago

Changes test running constant from DIR_TESTDATA to WP_RUN_CORE_TESTS.

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


3 years ago

#39 @antpb
3 years ago

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.

#40 @antpb
3 years ago

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

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

#41 @kraftbj
3 years ago

~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.

Last edited 3 years ago by kraftbj (previous) (diff)
Note: See TracTickets for help on using tickets.