Maybe put site-health in the root directory instead of inside includes since there are no other directories in there?

I thought in future if we added something like admin dashboard for managing URL metrics or any other admin dashboard related thing then it would be better to add that feature in includes/admin. But we can just refactor things later when we need it so moving site-health to root makes sence.

Yeah, let's put it in the root for now since all other directories are there.

if we added something like admin dashboard for managing URL metrics or any other admin dashboard related thing

Aside: I did put together a rough utility plugin for this: https://github.com/westonruter/od-admin-ui

This is the first PR that adds an option to to Optimization Detective. We'll need to make sure that the relevant delete_option() calls get added to the plugin's uninstall.php.

Actually, this check wouldn't make sense here. It should rather be done in od_maybe_add_template_output_buffer_filter() to short-circuit if the REST API it is not available.

Should the $error->get_message() and maybe $error->get_code() be stored here?

Instead of storing the string, what about storing the $status_code instead?

The else condition should be added as an error result as well. Here especially the $status_code could be used.

Instead of having update_option() appearing in multiple places, each condition could populate an $info variable which is then sent into update_option() once at the end of the function.

Is this a best practice? Should it rather go in admin_init to avoid frontend writes? I'm not sure what others do here.

I think scheduling on plugin activation hook will be better than admin_init.

The problem is that the plugin activation hook doesn't work when network-activating a plugin in multisite.

In looking at WP_Site_Health, it goes ahead and schedules an event even for frontend requests, since it calls its maybe_create_scheduled_event method in the constructor. And the instance is loaded in wp-settings.php. Nevertheless, since a database write is involved, it is preferable if event scheduling happens via an admin request and not unauthenticated frontend requests.

Please look at this comment regarding which hook should be used.

I think hourly is too much. Maybe weekly would make sense.

I currently have it set to run weekly, but that might be too infrequent. If a configuration change disables the REST API, it could take an entire week for user to detect the issue. I believe running it daily would be a better choice.

I think weekly is fine. It's not likely that a user would be changing the availability of the REST API. If we check at the moment that a plugin is activated, and then check weekly thereafter, then this should be good. Note that Site Health's own checks run on a weekly basis via the wp_site_health_scheduled_check action.

See note below about how the activation hook does not run when network-activating a plugin. I think this should switch to use admin_init. For example, add this to hooks.php:

add_action( 'admin_init', 'od_rest_api_health_check_plugin_activation' );

These are the two reasons why I chose the activation hook approach:

1. Immediate Notice Display:
   With the activation hook approach, when the user activates the plugin, they will see the notice about the REST API during the same page load as the plugin activation.

   • If we use the admin_init hook instead, the user will not see the notice immediately after activating the plugin because the health check is only scheduled during the plugin activation page load. It will execute asynchronously on the next load, and the notice will only appear on the third page load.
   • If a delayed notice is acceptable, we can proceed with the admin_init hook.

2. Efficient Option Handling:
   As mentioned here, we should add an empty option during plugin activation. If we use the admin_init hook, the code to check whether the option exists and create it if not will run on every admin page load, which is inefficient.

Alternative Approach:
We can also adopt a hybrid approach where the event is scheduled in the admin_init hook, while the logic for adding the option and immediate notice display is handled in the activation hook. This approach would also address the multisite issue where the event may not get scheduled.

2. If we use the admin_init hook, the code to check whether the option exists and create it if not will run on every admin page load, which is inefficient.

Will this be inefficient? WordPress will remembers options which don't exist in notoptions so that no DB query occurs with the next get_option() call (when an external object cache is being used). Nevertheless, if upon activation the plugin has an admin_init action which will immediately call the Site Health logic to see if the REST API is available, then this will set the option so that no such request happens with the next admin page load. Also, if this request is gated behind admin requests in the first place, then this further reduces the performance impact since frontend users would experience nothing.

So I propose something like this:

/**
 * Plugin activation hook for the REST API health check.
 *
 * @since n.e.x.t
 */
function od_rest_api_health_check_plugin_activation(): void {
	// The option already exists, so do nothing.
	if ( false !== get_option( 'od_rest_api_info' ) ) {
		return;
	}

	// This will populate the od_rest_api_info option so that the function won't execute for the next page load.
	od_run_scheduled_rest_api_health_check();
}

and:

add_action( 'admin_init`, 'od_rest_api_health_check_plugin_activation' );

As discussed in #1762 (comment), we don't want to schedule any cron events at this time. To ensure our check runs for the first time, we can modify the provided function by directly calling od_optimization_detective_rest_api_test instead of od_run_scheduled_rest_api_health_check.

/**
 * Plugin activation hook for the REST API health check.
 *
 * @since n.e.x.t
 */
function od_rest_api_health_check_plugin_activation(): void {
    // If the option already exists, do nothing.
    if ( false !== get_option( 'od_rest_api_info' ) ) {
        return;
    }

    // This will populate the od_rest_api_info option so that the function won't execute on the next page load.
    od_optimization_detective_rest_api_test();
}

add_action( 'admin_init', 'od_rest_api_health_check_plugin_activation' );

Also for the admin notices which needs to be displayed only once after plugin activation can be included in this function.

/**
 * Plugin activation hook for the REST API health check.
 *
 * @since n.e.x.t
 */
function od_rest_api_health_check_plugin_activation(): void {
	// If the option already exists, do nothing.
	$rest_api_info = get_option( 'od_rest_api_info' );
	if ( false !== $rest_api_info ) {
		return;
	}

	// This will populate the od_rest_api_info option so that the function won't execute on the next page load.
	od_optimization_detective_rest_api_test();

	if (
		isset( $od_rest_api_info['available'] ) &&
		! (bool) $od_rest_api_info['available'] &&
		isset( $od_rest_api_info['error_message'] )
	) {
		wp_admin_notice(
			esc_html( $rest_api_info['error_message'] ),
			array(
				'type'               => 'warning',
				'additional_classes' => array( 'notice-alt' ),
			)
		);
	}
}

add_action( 'admin_init', 'od_rest_api_health_check_plugin_activation' );

What about moving these to a section in the plugin's existing hooks.php?

I think this could be moved to a /site-health.php file.

Since there is only one Site Health test, what about eliminating the load.php file and moving this file to /site-health.php?

As noted below, I think the directory can be removed in favor of just a simpler site-health.php for now:

I think this file can be removed in favor of moving the /site-health/rest-api/helper.php file below up to /site-health.php.

Do we really need to check this weekly? That seems excessive as it is very unlikely to change. How about checking when the plugin is activated and when users visit the Site Health screen?

Oh, right, won't this test automatically get run anyway due to wp_site_health_scheduled_check? cf. #1762 (comment)

In that way, we don't need to schedule our own action at all but just re-use Site Health. Our test can persist the result in an option in a way similar to how site health in core stores issue counts in a transient.

just run check directly, caching results?

In wp-env, which doesn't support loopback requests, it's expected for there an error message to display. And I am indeed getting one, but it's not telling me what impact the error has:

In other words, this error is happening, but it should explain that Optimization Detective will not work because of the error. In other words, the same strings shown on the Site Health screen should be shown here as well:

Currently, I am displaying $result['label'] in the notices, but would using the more descriptive $result['description'] make more sense for the inline and also for the one time notice at top?

Sorry, where are you displaying $result['label'] in the notices? I only see it in od_optimization_detective_rest_api_test(). I think the same notice can be displayed inline as is what is shown not-inline in admin_notices.

Like this my question is should we display more detailed notice everywhere or this is fine?

Show the more detailed notice. In fact, perhaps only show the notice in admin_notices once (after activating the plugin) but then thereafter only show it inline. This would avoid the notice from appearing duplicated.

Question: Why use the after_plugin_row_meta action? If it so happens that the user has many plugins active, which is common, then Optimization Detective may not appear on the initial page and so the error will never be seen. I think the admin_notices action is more appropriate.

Or rather there could be an inline notice with the plugin row which displays always, and then there could also be a notice shown at admin_notices for the first time that the logic in https://github.com/WordPress/performance/pull/1762/files#r1907965242 runs. In this way, a user should see the notice after activating the plugin on the plugin list table screen, but then it will go away after reloading to stop nagging them forever. But they'll still be able to see the notice if they scroll down to Optimization Detective as well as when they open Site Health.

A potential solution could be to display a one-time notice upon plugin activation, as suggested in this comment #1762(comment). Additionally, a persistent error message can be shown inline using the after_plugin_row_meta hook.

Yeah, a one-time notice upon activation, and then otherwise if this one-time notice is not displayed at admin_notices to then instead show it as an inline notice at after_plugin_row_meta.

Related to https://github.com/WordPress/performance/pull/1762/files#r1907962460, I think this should not use inline since a user may not scroll down to see the error message with the activated plugin row.