Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50145 closed defect (bug) (fixed)

Site Health - Defensively handle empty/improperly formed test results

Reported by: dogwithblog's profile dogwithblog Owned by: sergeybiryukov's profile 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 4 years ago.
Site health error
50145.diff (3.0 KB) - added by SergeyBiryukov 4 years ago.
50145.2.patch (2.7 KB) - added by Clorith 4 years ago.

Download all attachments as: .zip

Change History (20)

@dogwithblog
4 years ago

Site health error

#1 @davidbaumwald
4 years ago

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

#2 @kraftbj
4 years 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.


4 years ago

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


4 years ago

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


4 years ago
#5

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


4 years ago
#6

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
4 years 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
4 years 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 4 years ago by SergeyBiryukov (previous) (diff)

@SergeyBiryukov
4 years ago

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


4 years ago

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


4 years ago

#11 @whyisjake
4 years 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 years ago

  • Milestone changed from 5.5 to 5.6

@Clorith
4 years ago

#13 @Clorith
4 years 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
4 years ago

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

#15 @SergeyBiryukov
4 years 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
4 years ago

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