Error checking in site health async cron tasks

[dd32]

The following PHP Notice is generated by the Site Health cron tasks:

Notice: Undefined index: status in wp-admin/includes/class-wp-site-health.php on line 2594

Call Stack:
...
    0.7033   53046672   3. WP_Hook->do_action() wp-includes/plugin.php:544
    0.7033   53046672   4. WP_Hook->apply_filters() wp-includes/class-wp-hook.php:311
    0.7033   53047800   5. WP_Site_Health->wp_cron_scheduled_check() wp-includes/class-wp-hook.php:287

The $result value that it's processing at the time contains:

  array(3) {
    'code' =>
    string(13) "rest_no_route"
    'message' =>
    string(55) "No route was found matching the URL and request method."
    'data' =>
    array(1) {
      'status' =>
      int(404)
    }

It appears that it happens when the wp-site-health/v1/tests/* REST API endpoints are called from cron.

There's two issues here:

1. The code assumes that the only error conditional is a WP_Error from a HTTP failure. A wp-json REST API error is not a WP_Error, it's a standard array response.
2. The Admin Ajax and REST API callbacks as run from cron are not adding any form of authentication, and so, will never work. The nonce attached would be for an anonymous user and as none of these endpoints are available unauthenticated do not pass.

I believe this is a followup to #47606 and it's prominence has only been noticed due to the changed in #48105 triggering these notices.

[Clorith]

Good catch, not sure why I overlooked this. You are correct that it is because the rest endpoint isn't set in this scenario, but it would also fail because of the lack of auth.

Creating a cron-token and setting it as the nonce value would probably be needed here to bypass the need for an active authenticated user session, but shouldn't be a difficult thing to get added.

[Clorith]

As there's no internal way to authenticate against the REST endpoint, 51547.patch​ introduces a direct callback route that async tests can register if they want or can.

It also hardens the check which initially alerted us to the problems here by checking if the response is a WP error, and that it gets a non-error response code from the request as well.

This will need a dev note, but it goes along with the REST endpoint dev note in general.

[TimothyBlynJacobs]

What are the set_site_status_test_cron_rest_token actions used for?

[Clorith]

51547.2.patch​ Removes the tokens which were left over from an authentication experiment.

It also renames the direct test argument to cover any kind of async test, it doesn't make sense to limit this to only REST endpoints, as admin-ajax may also be set up to require authentication.

[TimothyBlynJacobs]

I think async_direct_test should be documented as callable instead of mixed.

[Clorith]

Ahh, I'd never heard of callable there, consider it done!

[TimothyBlynJacobs]

isset( $test['async_direct_test'] ) && ! empty( $test['async_direct_test'] ) can be consolidated to just ! empty( $test['async_direct_test'] ). Besides that, code approach looks good to me. But I haven't tested manually triggering the cron job.

[Clorith]

Picking up all the new tricks this evening I am :) 51547.4.patch​ consolidates the suggestions above into a nice little package.

[desrosj]

In 49232:

Site Health: Improve the reliability of asynchronous tests.

This change adds additional logic to catch HTTP failures that do not return a WP_Error object (for example, a wp-json REST API error error).

This also fixes instances where REST API callbacks performed from cron do not work due to a lack of authentication by introducing a direct callback route that asynchronous tests can register.

Props dd32, clorith, timothyblynjacobs.
Fixes #51547.

[SergeyBiryukov]

In 49266:

Docs: Add a @since note about async_direct_test key to the site_status_tests filter.

Follow-up to [49232].

See #51547.