Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#47606 closed task (blessed) (fixed)

Display Site Health score on Dashboard

Reported by: guddu1315's profile guddu1315 Owned by: clorith's profile Clorith
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.2.2
Component: Site Health Keywords: has-patch dev-feedback
Focuses: ui Cc:

Description

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,

Attachments (7)

site-health-dashboard.png (151.7 KB) - added by guddu1315 5 years ago.
dashbaord-widget.png (69.5 KB) - added by Clorith 5 years ago.
47606.patch (13.6 KB) - added by Clorith 5 years ago.
47606.diff (539 bytes) - added by dlh 5 years ago.
47606.2.diff (780 bytes) - added by xkon 5 years ago.
47606.3.diff (762 bytes) - added by xkon 5 years ago.
47606.2.patch (666 bytes) - added by Clorith 5 years ago.

Download all attachments as: .zip

Change History (49)

#1 @joyously
5 years ago

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

#2 @afercia
5 years ago

  • Focuses accessibility removed
  • Keywords needs-design added

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 :)

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


5 years ago

#4 @hedgefield
5 years ago

  • Keywords needs-design removed

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.

#5 @Clorith
5 years ago

#48986 was marked as a duplicate.

#6 @SergeyBiryukov
5 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.4

#7 @Clorith
5 years ago

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

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
5 years ago

#8 @Clorith
5 years ago

  • Keywords has-patch added; needs-patch removed

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).

#9 @SergeyBiryukov
5 years ago

In 47062:

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

Props Clorith.
See #47606.

#10 @SergeyBiryukov
5 years ago

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.

#11 @SergeyBiryukov
5 years ago

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.

#12 @afercia
5 years ago

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

#13 @dlh
5 years ago

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().

@dlh
5 years ago

#14 @SergeyBiryukov
5 years ago

In 47068:

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

Props afercia.
See #47606.

#15 @SergeyBiryukov
5 years ago

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.

#16 follow-up: @afercia
5 years ago

  • Keywords needs-patch added; has-patch removed

@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
5 years ago

#17 @xkon
5 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

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.

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


5 years ago

#19 @Clorith
5 years ago

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

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


5 years ago

#21 @SergeyBiryukov
5 years ago

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.

#22 in reply to: ↑ 16 @SergeyBiryukov
5 years ago

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?

#23 @Otto42
5 years ago

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.

#24 @Otto42
5 years ago

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.

#25 @Otto42
5 years ago

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.

#26 @Otto42
5 years ago

  • Severity changed from normal to blocker

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

#27 @Otto42
5 years ago

  • Priority changed from normal to high

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
5 years ago

#28 @xkon
5 years ago

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 with the sql_server running first both should be fine. 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.

Version 0, edited 5 years ago by xkon (next)

#29 @Otto42
5 years ago

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

#30 @Clorith
5 years ago

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).

#31 @SergeyBiryukov
5 years ago

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.

#32 @Otto42
5 years ago

  • Priority changed from high to normal
  • Severity changed from blocker to normal

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

#33 @SergeyBiryukov
5 years ago

  • Type changed from feature request to task (blessed)

#34 follow-up: @Clorith
5 years ago

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.

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


5 years ago

#36 in reply to: ↑ 34 @pbiron
5 years ago

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
5 years ago

#37 @Clorith
5 years ago

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.

#38 @SergeyBiryukov
5 years ago

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.

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


5 years ago

#40 @SergeyBiryukov
5 years ago

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

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

Thanks everyone!

This ticket was mentioned in Slack in #forums by sterndata. View the logs.


5 years ago

#42 @garrett-eclipse
5 years ago

Related #49577
*On initial load dashboard widget shows more items than when navigating to the full Site Health Status screen.

Note: See TracTickets for help on using tickets.