WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 12 days ago

Last modified 12 days ago

#48105 closed enhancement (fixed)

Move Site Health async checks to a REST API endpoint

Reported by: Clorith Owned by: Clorith
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: has-patch
Focuses: rest-api Cc:

Description

Currently various tests are ran asyncronously with a call to admin-ajax.php, which has some limitations, specifically in core filters not being loaded when using old style ajax calls (see the loopback to wp-admin/site-health.php?health-check-test-wp_version_check to verify the existence of the wp_version_check filter).

Moving things out into REST API endpoints would provide more flexibility, and better handling of permission checks as well. It also means that any code extending tests and adding their own async checks would have a much ore robust platform to test against, if they choose to utilize it.

There's some backwards compatibility considerations to be kept in mind, the current implementation of calling an admin-ajax call needs to be maintained, likely by introducing a has_rest => true flag to the async test registration to decide where to forward the call.

Attachments (4)

48105.patch (12.5 KB) - added by Clorith 13 months ago.
48105.2.patch (20.4 KB) - added by Clorith 11 months ago.
48105.3.patch (22.0 KB) - added by Clorith 4 months ago.
test-unavailable.PNG (43.7 KB) - added by Clorith 4 months ago.

Download all attachments as: .zip

Change History (43)

@Clorith
13 months ago

#1 @Clorith
13 months ago

48105.patch is a first iteration of the above, it retains backwards compatibility for async tests created with admin-ajax.php in mind, but switches to using a REST endpoint if has_rest is defined and set to true.

It also adds failure handling to the async tests, so that if a test fails, it shouldn't break the JS from continuing with the next available check.

There's still some cleaning needed before this is ready (removing references to the old ajax hooks)

@Clorith
11 months ago

#2 @Clorith
11 months ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 5.4

48105.2.patch provides REST endpoints for the previous ajax calls for the core provided asynchronous Site Health tests.

It also removed the now unused ajax handlers, and provides deprecation notices, and _doing_it_wrong calls to the callback functions from those ajax calls. I don't believe anyone is calling those functions directly, but better safe than sorry, as the saying goes.

It also improves on the failure handling for those tests, if a test fails, a notice will now be displayed in the recommended section, the badge has the label Unavailable, with a red outline, as opposed to the normal blue used by core for all other tests.

The URL invoked to perform the test is provided, and either the string No details available (as a fallback), or the error returned by the site.

#3 @TimothyBlynJacobs
10 months ago

I think this would be a great time to take a look at using a dashboard/v1 namespace for these dashboard endpoints that aren't modeling WP data objects. https://github.com/WP-API/proposals/pull/1

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-site-health by timothybjacobs. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-restapi by clorith. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-site-health by clorith. View the logs.


9 months ago

#9 @Clorith
9 months ago

  • Milestone changed from 5.4 to 5.5

Bumping this to the 5.5. milestone, there's still discussions happening in relation to namespaces on the REST API, which I don't think it's realistic there will be a decision on before beta-1 for 5.4, and this enhancement is nice to have, but not critical.

#10 @afragen
8 months ago

@Clorith there's a typo in the translators comment taht. Copy/pasted several times. ;)

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


6 months ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


6 months ago

#14 @afragen
6 months ago

  • Keywords needs-refresh added

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 months ago

#17 @TimothyBlynJacobs
5 months ago

  • Owner set to Clorith
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


4 months ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


4 months ago

@Clorith
4 months ago

#20 @Clorith
4 months ago

  • Keywords needs-refresh removed

48105.3.patch is what I hope is the final iteration here.

It removes the now redundant REST API test, as the asynchronous tests use the REST endpoint and will display a warning if they fail (see comment:2 and test-unavailable.PNG), this makes the original test no longer useful in this scenario (although I know there's a ticket to introduce JavaScript-only tests to account for edge cases here, but that's out of scope for the upcoming release).

A new field is introduces to the site_status_tests filter, named $has_rest, if this is a truthy value, then the $test check is fired off as a REST call instead of passing it as an action to admin-ajax.

The space-calculation from the Info screen has also been moved to use this, to remove _all_ Site Health dependencies on admin-ajax for consistency.

A new filter also comes along with this named site_health_test_rest_capability_{$check}, this will allow developers to change what capability is needed to access the various tests, using the existing view_site_health_checks as a default.

#21 @Clorith
4 months ago

  • Keywords commit added

#22 @TimothyBlynJacobs
4 months ago

@Clorith can you turn this into a PR? I have some line-to-line feedback.

This ticket was mentioned in PR #381 on WordPress/wordpress-develop by Clorith.


4 months ago

See [trac ticket comments](https://core.trac.wordpress.org/ticket/48105#comment:20) for changes in the initial PR

Trac ticket: https://core.trac.wordpress.org/ticket/48105

#24 @whyisjake
4 months ago

  • Keywords commit removed
  • Milestone changed from 5.5 to 5.6

Would love to get this into the 5.5 beta, but not sure about the timing here. Checked in with @TimothyBlynJacobs, and going to punt. There is a lot of feedback to be addressed in the PR.

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


5 weeks ago

#26 @hellofromTonya
5 weeks ago

  • Keywords needs-refresh added

Looks like this one needs a refresh.

#27 @prbot
4 weeks ago

TimothyBJacobs commented on PR #381:

Thanks for refreshing @Clorith! Can we make the REST API calls use wp.apiRequest?

#28 @prbot
3 weeks ago

TimothyBJacobs commented on PR #381:

Sorry, I think the last thing. Do we need each REST API endpoint to wrap its results in the data property? We don't typically do this. Looking at the JS it looks like it passes on response.data, can we then just change that to response?

#29 @prbot
3 weeks ago

Clorith commented on PR #381:

Sorry, I think the last thing. Do we need each REST API endpoint to wrap its results in the data property? We don't typically do this. Looking at the JS it looks like it passes on response.data, can we then just change that to response?

Good catch, in my head I was trying to maintain the same format as the admin-ajax calls had when using wp_send_json_success for the sake of the filters in place, but the filters take just the response data any way, and not the whole response object, so this shouldn't need ot be mimiced.

#30 @prbot
2 weeks ago

TimothyBJacobs commented on PR #381:

@Clorith Can you rebase this please?

#31 @prbot
2 weeks ago

Clorith commented on PR #381:

Well, that rebase was an absolute horror, let's try that again, this time with feeling (and less branch-bleed) 😔 !

#32 @prbot
2 weeks ago

TimothyBJacobs commented on PR #381:

Thanks @Clorith!

Looks like there is a relevant test failure:

1) Tests_User_Capabilities::testMetaCapsTestsAreCorrect
These meta capabilities are not tested
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    30 => 'view_site_health_check'
+    31 => 'view_site_health_debug_info'
+)

#33 @prbot
2 weeks ago

Clorith commented on PR #381:

Thanks @Clorith!

Looks like there is a relevant test failure:

Yeah, the rebase went a bit nuts on me and added some ide configs, and commits from a different branch and I've no idea what exploded, but exploded it did, just doing a reset and re-rebase here to clean it up :)

This ticket was mentioned in PR #592 on WordPress/wordpress-develop by Clorith.


2 weeks ago

  • Keywords needs-refresh removed

See trac ticket comments for changes in the initial PR

Trac ticket: core.trac.wordpress.org/ticket/48105

This is a clean branch, after a rather large rebase went bonkers and is unable to revert it self in #381. (Live and learn).

For sanity sake, referencing the ticket above as the origin, as it has some history included, also making note of contributions to this feature by @chrisvanpatten

#35 @prbot
2 weeks ago

Clorith commented on PR #381:

Closing this in favor of #592 (unrecoverable rebase issue, so just reworked it instead).

#36 @TimothyBlynJacobs
12 days ago

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

In 49154:

Site Health, REST API: Move async tests to REST API endpoints.

This provides more flexibility when writing tests and benefits from running in a front-end context which is necessary for some tests like checking that updates are supported. Additionally, this provides a more robust interface for developers who want to integrate with Site Health tests.

Because the wp/v2 endpoint is reserved for modeling core entities, site health is registered in its own wp-site-health/v1 namespace.

The existing ajax actions have been maintained for backward compatibility.

Props Clorith, chrisvanpatten, afragen, pokhriyal, TimothyBlynJacobs.
Fixes #48105.

#37 @prbot
12 days ago

TimothyBJacobs commented on PR #592:

Merged in efe06cdceac62f928f1adf008bb55a4293f9a3c2.

#38 @TimothyBlynJacobs
12 days ago

In 49155:

REST API: Grant super admin to site health test user.

The current user needs to be a super admin to access Site Health on multisite.

Follow up to [49154].
See #48105.

#39 @TimothyBlynJacobs
12 days ago

In 49156:

REST API: Exclude custom site health capability check on multisite.

Super admins will always pass capability checks.

Follow up to [49154], [49155].
See #48105.

Note: See TracTickets for help on using tickets.