Display Site Health score on Dashboard

[guddu1315]

Hello,

Can we add site health score and list of critical issues in Dashboard ?
Not many users would know that there is something called Site Health Check in new version of WordPress. So displaying the score and critical issues list in Dashboard will notify users who are not aware about it.

Plus every time user logs in to the backend, he/she is reminded that his/her site needs updating.

Thank you,

[joyously]

The things that need attention already have nags.
Given #47046 I think this ticket is just asking for another dashboard widget.

[afercia]

A Site Health dashboard widget sounds like an interesting idea to me, thanks for your proposal @guddu1315! Seems to me it needs some design feedback rather than accessibility feedback, I'm going to adjust the ticket properties :)

[hedgefield]

Good shout @guddu1315. Site Health is pretty hidden down in some submenu for what is arguably a feature that could use a little more attention as part of a good site hygiene strategy. An optional dashboard widget would be a good way to surface it.

The plugin version of Site Health already has a basic dashboard widget implemented: ​https://github.com/WordPress/health-check/issues/226 @SergeyBiryukov is currently looking at whether this can be merged into core I believe.

[Clorith]

#48986 was marked as a duplicate.

[Clorith]

Let's start off by importing the widget from the plugin, it's tried and tested, non-intrusive, and doesn't end with information overload from too much happening at once.

I'll get a patch up later this week.

The implementation that exists right now looks like the screenshot in dashbaord-widget.png​ , and I think this is a great starting point, if changes turn out to be needed beyond this, we can iterate on them.

[Clorith]

47606.patch​ has a few pieces to it for functionality sake.

It makes sure the WP_Site_Health class can be instantiated once, so we don't have to recreate it for multiple page loads, this to facilitate that the class has to run in a few locations depending on context.

An instance is called in wp-settings.php, this is to make sure the introduced WP_Cron event can be picked up and handled (much like the fatal error handler, found right above it, is done, only without the assistance of helper functions).

The Dashboard widget it self is for the most part a straight copy from the plugin, as I mentioned I wanted to use that, it has a slight adjustment to the default string, shown only when no site health checks have been collected yet, to account for scheduled events being filterable so the frequency may not be what was originally implemented.

This also enqueues Site Health's JavaScript and styles on the Dashboard page, as well as the individual site-health.php pages.

The task of running tests was separated out to its own function, WP_Site_Health->perform_test( $callback ), to prevent repeating our selves in too many places.

And finally, a new cron schedule is added for weekly events (I've always been puzzled by this not already existing, but it's not been needed, and now it is, so makes sense to introduce it).

[SergeyBiryukov]

In 47062:

Cron API: Add a new cron schedule for weekly events.

Props Clorith.
See #47606.

[SergeyBiryukov]

In 47063:

Site Health: Introduce Site Health Status dashboard widget.

The widget informs administrators of any potential issues that should be addressed to improve the performance or security of their website, and directs them to the Site Health screen for more details.

Props Clorith, hedgefield, guddu1315.
See #47606.

[SergeyBiryukov]

In 47064:

Tests: In Tests_Site_Health, create a WP_Site_Health instance before clearing the cron array, as the constructor schedules its own task now.

See #47606.

[afercia]

Re [47062]: why not to use the existing constant WEEK_IN_SECONDS?

[dlh]

A stdClass isn't considered empty, so the empty( $issue_counts ) check in wp_dashboard_site_health() fails when $issue_counts is still in its initialized state as a new stdClass(). This generates Undefined property notices in the widget when the transient doesn't exist because the defaults aren't set.

The attached patch would switch the check to get_object_vars(), which returns an empty array for a new stdClass().

[SergeyBiryukov]

In 47068:

Cron API: Use WEEK_IN_SECONDS constant for the weekly schedule added in [47062].

Props afercia.
See #47606.

[SergeyBiryukov]

In 47069:

Site Health: Avoid "Undefined property" PHP notices in wp_dashboard_site_health() when the status result transient does not exist yet.

Props dlh for initial patch.
See #47606.

[afercia]

@SergeyBiryukov 👋 When you have a chance: seems to me that after [47063] all tests run twice. Both the WP built-in tests and the ones added by plugins.

To my understanding, [47063] introduced an initialize method that returns or creates an instance of the class. When it's called in wp-settings.php it initializes the class.

Since the class is initialized in wp-settings.php, the class __construct() runs on any admin page and calls the method within it e.g. maybe_create_scheduled_event, prepare_sql_data run on all admin pages.

More importantly, the class is initialized again in the Site Health pages thus the tests run twice.

[xkon]

I could replicate what @afercia mentioned in comment 16 and 47606.2.diff​ is a proposal for a solution as it still "allows" the ajax calls to pass on wp-settings. Not sure if this is optimal though so a 2nd look would be great :).

This avoids running the extra instance on all page loads but still keeps the calls enabled when necessary via dashboard + ajax.

[Clorith]

I think it might be more appropriate to add an "is initialized" to the class, and if true, skip the test setups and such?

[SergeyBiryukov]

In 47149:

Site Health: Rename WP_Site_Health::initialize() introduced in [47063] to ::get_instance(), for clarity and consistency with other core classes.

Use WP_Site_Health::get_instance() where it's needed, instead of creating multiple instances of the class.

Props afercia, xkon, Clorith, SergeyBiryukov.
See #47606.

[SergeyBiryukov]

Replying to afercia:

More importantly, the class is initialized again in the Site Health pages thus the tests run twice.

Thanks for catching that! I think [47149] should address the issue, could you double-check?

[Otto42]

This early initialization is also causing an issue on w.org because of the need for $wpdb-dbh to be defined.

The w.org systems use hyperdb, which delays connecting the dbh to a connection until an actual query is called. So when you call _construct and it calls prepare_sql_data, at this point, $wpdb->dbh is set to null, which causes Warning: mysqli_get_server_info() expects parameter 1 to be mysqli, null given in /wp-admin/includes/class-wp-site-health.php on line 191.

This is currently happening on pretty much every dynamic page on w.org, although the warning is suppressed at present.

Seems like the actual functionality for this db versioning check needs to be moved out of the constructor or the initialization of the class needs to happen later in the cycle.

[Otto42]

Possible workaround: Call $wpdb->db_version(); at any point before using $wpdb->dbh. This will cause hyperdb to initiate a connection and set the dbh variable. Adding $wpdb->db_version(); to the prepare_sql_data() function eliminates the issue.

[Otto42]

Note that this is a workaround only. The calls that depends on the dbh need to not be in the constructor if we're constructing it on every hit. This is too much work to do too early, and will cause other problems than just hyperdb.

[Otto42]

Marking as blocker because this cannot be released like this. Too much breakage.

[Otto42]

This is beginning to affect the w.org API responses. If it cannot be solved soon, then it will need to to be fully reverted.

[xkon]

I believe that we can move the prepare_sql_data() inside the get_test_sql_server() to move it out of the constructor as in 47606.3.diff​ .

As far as I've seen only the sql_server & utf8mb4_support tests needs this and with the sql_server running first both should be fine (unless we want to call it again in the utf8mb4 check just for being safe). I didn't find any other test needing this at the moment but can't be entirely sure so I'll ping @Clorith here for a check :-).

With this move, the constant calls from every page are also "fixed" as tests would only run under Site Health page.

The class still runs everywhere due to wp-settings.php but only the scheduled_event check would run that is still within the constructor.

[Otto42]

Seems like a good fix to me, delaying these mysqli calls to when they're actually needed makes the most sense.

[Clorith]

Looks good @xkon!

Let's get 47606.3.diff​ in so we can resolve this, they shouldn't be needed except for that test, and not separating them out was an oversight on my part when the class was moved to be available more frequent (for the WP_Cron events).

[SergeyBiryukov]

In 47215:

Site Health: Only get MySQL server information in tests where it's actually needed.

This resolves an issue with plugins like HyperDB, which doesn't have a database connection until a query is made.

Props xkon, Otto42, Clorith.
See #47606.

[Otto42]

Thanks. Deployed to w.org, will notify if any further issues develop.

[Clorith]

During beta-release testing, it was discovered that the widget is "stuck" in the network admin screen, this is caused by the enqueues never loading on this screen (so no data exists). This has no detremental impacto n performance at this time, and was therefore not a blocked for the beta.

After some quick looking into it, this is very simple to fix, but the results are related ot the root site in the network, and the action link also takes you to the root sites Site Health checks page when clicked.

I'm not sure if this scenario should be considered expected, or unexpected, behavior. On the one side, the root site is a known element in multisite, but an inexperienced network admin (or an experienced one as well, to be honest) might expect the results shown on that screen to revolve around the network as a whole.

[pbiron]

Replying to Clorith:

I'm not sure if this scenario should be considered expected, or unexpected, behavior. On the one side, the root site is a known element in multisite, but an inexperienced network admin (or an experienced one as well, to be honest) might expect the results shown on that screen to revolve around the network as a whole.

Thinking about this for the 1st time, so take this with a grain of salt.

Maybe on the Network Dashboard the widget could do tests that are common to all sites (e.g., PHP/MySQL versions, etc) and but not run tests whose results could be different on specific sites (e.g., unused plugins/themes).

[Clorith]

47606.2.patch​ prevents the widget from loading on the network admin screen , which I think is the more appropriate solution right now, as many of the tests are geared towards the individual sites, and the user who has access to this information is restricted.

I've milestoned #47085 for 5.5 with the intention of improving network installs there.

[SergeyBiryukov]

In 47300:

Site Health: Prevent the Site Health Status dashboard widget from loading on network admin screen for now.

Props Clorith, pbiron.
See #47606, #47085.

[SergeyBiryukov]

Let's call this fixed. Please create new tickets for any follow-up issues.

Thanks everyone!