WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 4 days ago

#49889 reviewing enhancement

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 needs-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 (4)

49889.diff (3.2 KB) - added by Howdy_McGee 7 months ago.
WP_DEBUG Wrapped
49889-2.diff (7.7 KB) - added by Howdy_McGee 6 months ago.
Introduces wp_getimagesize()
49889-3.diff (7.7 KB) - added by Howdy_McGee 6 months ago.
@since 5.5.0
49889.4.diff (7.8 KB) - added by SergeyBiryukov 4 days ago.

Download all attachments as: .zip

Change History (22)

@Howdy_McGee
7 months ago

WP_DEBUG Wrapped

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


6 months ago

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

Introduces wp_getimagesize()

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

@since 5.5.0

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


6 months ago

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


6 months ago

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


5 months ago

#11 @davidbaumwald
4 months ago

  • Keywords needs-dev-note added

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

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

  • Milestone changed from Future Release to 5.6

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


3 weeks ago

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


3 weeks 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.


2 weeks ago

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

Note: See TracTickets for help on using tickets.