#47606 closed task (blessed) (fixed)
Display Site Health score on Dashboard
Reported by: | guddu1315 | Owned by: | 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)
Change History (49)
#2
@
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
@
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.
#7
@
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.
#8
@
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).
#13
@
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()
.
#16
follow-up:
↓ 22
@
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.
#17
@
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
@
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
#23
@
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
@
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
@
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
@
5 years ago
- Severity changed from normal to blocker
Marking as blocker because this cannot be released like this. Too much breakage.
#27
@
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.
#28
@
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.
#29
@
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
@
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).
#32
@
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.
#34
follow-up:
↓ 36
@
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
@
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).
#37
@
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.
This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.
5 years ago
#40
@
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!
The things that need attention already have nags.
Given #47046 I think this ticket is just asking for another dashboard widget.