WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 3 weeks ago

Last modified 7 days ago

#50145 closed defect (bug) (fixed)

Site Health - Defensively handle empty/improperly formed test results

Reported by: dogwithblog Owned by: SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Site Health Keywords: has-screenshots has-patch has-dev-note
Focuses: Cc:

Description

Hi team,

I am getting the message, "results are still loading" on Site Health even after being on wait for over half an hour. This is for my blog: https://dogwithblog.in

Attachments (3)

dwbw.jpg (40.1 KB) - added by dogwithblog 7 months ago.
Site health error
50145.diff (3.0 KB) - added by SergeyBiryukov 5 months ago.
50145.2.patch (2.7 KB) - added by Clorith 7 weeks ago.

Download all attachments as: .zip

Change History (20)

@dogwithblog
7 months ago

Site health error

#1 @davidbaumwald
7 months ago

  • Component changed from General to Site Health
  • Keywords has-screenshots added

#2 @kraftbj
7 months ago

  • Milestone changed from Awaiting Review to 5.5
  • Summary changed from Site Health - Results are still loading to Site Health - Defensively handle empty/improperly formed test results

Hi @dogwithblog,

Welcome to Trac. Generally, checking in for support at https://wordpress.org/support/ is a good step before filing a bug report since the issue may not be an issue with WordPress itself.

Checking out your site, I believe you have Jetpack installed. The recent version has a bug that is preventing the site health area from reporting a "grade".

Resolving this on the Jetpack side is being tracked at https://github.com/Automattic/jetpack/issues/15709

On the WordPress side, @clorith will be working on a patch to better handle a situation where a plugin returns an unexpected value.

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


7 months ago

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


5 months ago

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


5 months ago

  • Keywords has-patch added

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

This PR adds a validator to the issue respones for Site Health tests, which will discard any invalid responses instead of causing fatal errors.

It also makes badges optional, on the same basis as actions are optional, it's expected, but there may be situations where they're not present.

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


5 months ago

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

This PR adds a validator to the issue respones for Site Health tests, which will discard any invalid responses instead of causing fatal errors.

It also makes badges optional, on the same basis as actions are optional, it's expected, but there may be situations where they're not present.

#7 @Clorith
5 months ago

  • Keywords commit added

I've been doing some testing over the last few days, and this seems to be handling things well.

It also makes the badge optional. This is intentional. I like the badges as they give some context, especially the way large plugins have used them to flag where tests come from, but I can see scenarios where a badge may not make sense as well, so just future-proofing here.

#8 @SergeyBiryukov
5 months ago

  • Keywords needs-refresh added; commit removed

50145.diff is a refresh of https://github.com/WordPress/wordpress-develop/pull/349.diff.

It seems to work well in my testing, however it also introduces some JSHint issues that would result in test failures:

Running "jshint:core" (jshint) task
src/js/_enqueues/admin/site-health.js
108 |    Object.entries( minimumExpected ).forEach( ( [ key, value ] ) => {
                                                    ^ 'destructuring binding' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
108 |    Object.entries( minimumExpected ).forEach( ( [ key, value ] ) => {
                                                                     ^ 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
110 |            Object.entries( value ).forEach( ( [ subKey, subValue ] ) => {
                                                  ^ 'destructuring binding' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
110 |            Object.entries( value ).forEach( ( [ subKey, subValue ] ) => {
                                                                         ^ 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').

Core currently uses the "esversion": 3 (ECMAScript 3) setting in .jshintrc.

Would it be possible to rewrite the patch for compatibility with the current core standard?

Last edited 5 months ago by SergeyBiryukov (previous) (diff)

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


5 months ago

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


4 months ago

#11 @whyisjake
4 months ago

Also seeing this:

 {
  filename: 'site-health.js',
  line: 108,
  col: 64,
  pos: 3278
}
>> Uglifying source build/wp-admin/js/site-health.js failed.
Warning: Uglification failed.
Invalid assignment. 
Line 108 in build/wp-admin/js/site-health.js

Warning: Cannot read property 'min' of undefined

#12 @SergeyBiryukov
4 months ago

  • Milestone changed from 5.5 to 5.6

@Clorith
7 weeks ago

#13 @Clorith
7 weeks ago

  • Keywords needs-refresh removed

50145.2.patch refreshes the patch, simplifies some of the checks to not rely on a polyfill, and now builds like it should.

#14 @SergeyBiryukov
7 weeks ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#15 @SergeyBiryukov
3 weeks ago

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

In 49537:

Site Health: Validate the test result data format in JS before using it.

This will discard any invalid responses instead of causing fatal errors.

It also makes badges optional, on the same basis as actions are optional. They are expected, but there may be situations where they are not present.

Props Clorith, dogwithblog, kraftbj, whyisjake, SergeyBiryukov.
Fixes #50145.

#16 @Clorith
13 days ago

  • Keywords has-dev-note added
Note: See TracTickets for help on using tickets.