Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#50858 closed defect (bug) (fixed)

Site Health Check PHP notices with site_status_tests filter

Reported by: ov3rfly's profile Ov3rfly Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5.1 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: good-first-bug has-patch commit fixed-major
Focuses: Cc:

Description

Some methods get the the tests array from WP_Site_Health::get_tests() where a filter to "modify which site status tests are run on a site" is available.

$tests = apply_filters( 'site_status_tests', $tests );

Found an attempt to reduce any unexpected load on a high profile production site like this, which is a valid return value for a filter for arrays:

add_filter( 'site_status_tests', '__return_empty_array', 999999 );

With this filter in place some PHP notices are triggered e.g. when methods wp_cron_scheduled_check() is called:

[05-Aug-2020 10:56:04 UTC] PHP Notice:  Undefined index: direct in .../wp-admin/includes/class-wp-site-health.php on line 109
[05-Aug-2020 10:56:04 UTC] PHP Warning:  Invalid argument supplied for foreach() in .../wp-admin/includes/class-wp-site-health.php on line 109
[05-Aug-2020 10:56:04 UTC] PHP Notice:  Undefined index: async in .../wp-admin/includes/class-wp-site-health.php on line 127
[05-Aug-2020 10:56:04 UTC] PHP Warning:  Invalid argument supplied for foreach() in .../wp-admin/includes/class-wp-site-health.php on line 127

Similar PHP notices are also triggered if method enqueue_scripts() is called.

Suggested fix: Check if return value of filter is valid before using it, similar to this:

if ( is_array( $tests ) && isset( $tests['direct'] ) && is_array( $tests['direct'] ) ) { ...

Change History (10)

#1 @desrosj
4 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to 5.5.1
  • Version changed from 5.4.2 to 5.2

Thanks @Ov3rfly,

Changing the version to 5.2 as enqueue_scripts() and the site_status_tests filter existed then and the issue looks like it would occur first on that version. The wp_cron_scheduled_check() was introduced in 5.4, so those notices will be triggered after that version.

#2 @khag7
4 years ago

I think it's reasonable to say that $tests['direct'] and $tests['async'] must exist, even if they are empty arrays. If someone unsets them completely using the filter, they should be immediately recreated as empty arrays to prevent errors elsewhere. If I'm right about that, those should not be able to be completely removed by the site_status_tests filter.

What if we took the value returned from the filter and merged it into an empty array containing two keys, direct and async which each have a value of an empty array?

	$tests = array_merge( array(
		'direct' => array(),
		'async' => array(),
	), apply_filters( 'site_status_tests', $tests ) );

This means that even if the direct and async keys get completely unset, we will put them back immediately, but as empty arrays. Methods that make use of get_tests use a foreach loop on tests['direct'] or tests['async'] and if the value of is simply an empty array, the foreach loop doesn't run, and no error is thrown.

Option B is as previously suggested would be to do this every time get_tests is called:

if ( is_array( $tests ) && isset( $tests['direct'] ) && is_array( $tests['direct'] ) ) { ...

This ticket was mentioned in PR #456 on WordPress/wordpress-develop by khag7.


4 years ago
#3

  • Keywords has-patch added; needs-patch removed

#4 @khag7
4 years ago

Also, I've only ever contributed to WP via .patch files thru Trac, never with GitHub. I think I did it right, but if not someone please set me straight.

#5 @Ov3rfly
4 years ago

Thanks for your suggestion, option A sounds good, wp_parse_args() could be used to merge the array.

#6 @Clorith
4 years ago

  • Keywords commit added

The approach taken in your attached PR, @khag7, is absolutely the best way forward, as those array keys are referenced in multiple places, and this would ensure an "empty" return on the filter remains "empty" for all intent and purpose.

#7 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#8 @SergeyBiryukov
4 years ago

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

In 48808:

Site Health: Ensure that the tests returned by WP_Site_Health::get_tests() always have the required array keys: direct and async.

This avoids PHP notices if these keys were accidentally removed using the site_status_tests filter.

Props khag7, Ov3rfly, desrosj, Clorith.
Fixes #50858.

#9 @SergeyBiryukov
4 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the 5.5 branch.

#10 @SergeyBiryukov
4 years ago

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

In 48810:

Site Health: Ensure that the tests returned by WP_Site_Health::get_tests() always have the required array keys: direct and async.

This avoids PHP notices if these keys were accidentally removed using the site_status_tests filter.

Props khag7, Ov3rfly, desrosj, Clorith.
Merges [48808] to the 5.5 branch.
Fixes #50858.

Note: See TracTickets for help on using tickets.