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 | Owned by: | 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
@
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
#2
@
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
Trac ticket: https://core.trac.wordpress.org/ticket/50858
#4
@
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
@
4 years ago
Thanks for your suggestion, option A sounds good, wp_parse_args() could be used to merge the array.
#6
@
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.
Thanks @Ov3rfly,
Changing the version to 5.2 as
enqueue_scripts()
and thesite_status_tests
filter existed then and the issue looks like it would occur first on that version. Thewp_cron_scheduled_check()
was introduced in 5.4, so those notices will be triggered after that version.