WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 3 months ago

Last modified 3 months ago

#49889 closed enhancement (fixed)

Image.php - Wrap Error Suppressors in WP_DEBUG Conditionals

Reported by: Howdy_McGee Owned by: 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 13 months ago.
WP_DEBUG Wrapped
49889-2.diff (7.7 KB) - added by Howdy_McGee 13 months ago.
Introduces wp_getimagesize()
49889-3.diff (7.7 KB) - added by Howdy_McGee 13 months ago.
@since 5.5.0
49889.4.diff (7.8 KB) - added by SergeyBiryukov 7 months ago.
49889.5.diff (1.4 KB) - added by hellofromTonya 3 months ago.
Changes test running constant from DIR_TESTDATA to WP_RUN_CORE_TESTS.

Download all attachments as: .zip

Change History (47)

@Howdy_McGee
13 months ago

WP_DEBUG Wrapped

#1 @Howdy_McGee
13 months 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
13 months 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.


13 months ago

#4 @Howdy_McGee
13 months 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
13 months ago

Introduces wp_getimagesize()

#5 @mukesh27
13 months 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
13 months ago

@since 5.5.0

#6 @mukesh27
13 months 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.


13 months ago

#8 @antpb
13 months 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.


13 months ago

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


11 months ago

#11 @davidbaumwald
10 months ago

  • Keywords needs-dev-note added

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

#12 @davidbaumwald
10 months 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
10 months ago

  • Milestone changed from Future Release to 5.6

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


7 months ago

#15 @SergeyBiryukov
7 months 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.


7 months ago

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.


7 months ago

#18 @SergeyBiryukov
7 months 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.


4 months ago

#20 @noisysocks
4 months 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.


4 months ago

#22 @antpb
4 months 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.


4 months ago

#24 @hellofromTonya
4 months 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.


4 months ago

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.


4 months ago

#27 @hellofromTonya
4 months 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
4 months 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 months ago

#30 @antpb
3 months 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 months ago

  • Keywords commit removed

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

#35 @SergeyBiryukov
3 months 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 months 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 months 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 months 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 months ago

#39 @antpb
3 months 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 months 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 months 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 months ago by kraftbj (previous) (diff)
Note: See TracTickets for help on using tickets.