Improve resilience to PHP Notices and Warnings

[bobbingwide]

Issue Overview

As mentioned in today's core-editor meeting
I believe that Gutenberg could be significantly improved if it could detect unexpected server responses, especially things like PHP Notices and Warnings

and it should

1. Ignore them for the purposes of determining what actually happened.
2. Let the user know about the problem gracefully.

I have experienced a number of situations, where PHP Notices and Warnings get returned along with the validly formed JSON response.

• Some of these are out of my control.
• Most of them are good to know about.
• Especially if you're a developer running with WP_DEBUG true
• They don't cause a problem with the Classic editor

Steps to Reproduce (for bugs)

1. Find a plugin or theme that produces Notices or Warnings or simply echoes things when it shouldn't
2. Add a new post
3. Use it for a while
4. Notice Updating Failed messages
5. Look in the Browser Console log to see unwanted HTML
6. But notice that the updates did actually work.

For a particularly good example of a misbehaving plugin see https://github.com/bobbingwide/dinlo

Expected Behavior

It would be nice if Gutenberg could filter out what's not part of the JSON response and report the results based on that information.

It would also be nice if it was able to report nicely what the problem was.

Current Behavior

In the classic editor the messages appear. Many are just an annoyance but the editor works.

Possible Solution

Screenshots / Video

Here's the simplest example I could think of.
Add an HTML comment to the top of the gutenberg.php plugin.
<!-- 4936 -->

Related Issues and/or PRs

Todos

• Tests
• Documentation

[youknowriad]

Not sure this is very actionable, most of the time parsing network results is done by low level APIs (window.fetch or alternatives) and a JSON can't be separated from the extra output.

If there's something to be done here, it's more likely server-side in the REST API framework to catch all these errors and invalid output when the response type is expected to be JSON and issue a proper issue response.

[bobbingwide]

Not dealing with this issue is a failure to satisfy Non Functional Requirements in the area of Resilience, Robustness and Reliability. If we don’t have documented NFR’s then we need them.

Yes, the server could attempt to trap some responses, but not all of them.
With regards to being non-actionable, see the simplest example above.

• It’s the client that sees the unwanted data in the response.
• The REST API had nothing to do with it.

What does window.fetch get before it attempts to parse the JSON? The console log shows it pretty clearly.

[danielbachhuber]

Thanks again for the suggestion, @bobbingwide.

I've filed this away in the Ideas project for future consideration. At this point in the Gutenberg development cycle, we're focusing on finishing the remaining features and then switching to bug fixing / polish.

[bobbingwide]

From https://make.wordpress.org/core/handbook/about/philosophies/#striving-for-simplicity

Every version of WordPress should be easier and more enjoyable to use than the last.

[danielbachhuber]

@bobbingwide Yes, I agree with that.

[danielbachhuber]

@WordPress/gutenberg-core "Detecting broken REST API responses" has come up a few more times (#3102, #9285). Is it worth exploring seeing if we can detect if a response's JSON isn't unserializable?

[aduth]

So is the issue that the REST API response is prefixed by PHP notice, rather than being proper JSON? I don't see what this has to do with Gutenberg at all, it's an issue of the server not rendering a JSON entity correctly.

Should Gutenberg be tolerant to these? Maybe. It's certainly a bandaid fix that helps brush over the underlying issue. What would be the fallback which should occur when it's not unserializable?

Here's the current logic:

gutenberg/packages/api-fetch/src/index.js

Lines 54 to 60 in 186716f

	const parseResponse = ( response ) => {
	if ( parse ) {
	return response.json ? response.json() : Promise.reject( response );
	}
	return response;
	};

By this, it would appear we're trying to see if it's a JSON response before trying to parse. So either the server is misreporting the non-JSON as application/json content type, or we aren't handling the rejection well. What would be a good way of handling the rejection?

[youknowriad]

I think a proper way to deal with this is trying to catch these errors/warnings in PHP before sending the response if the response is JSON and return a well-formatted JSON error instead. It could be a generic behavior to all REST API endpoints calls.

[youknowriad]

Also, I don't like the idea of being tolerant personally because we'll be just hiding real issues.

[chrisvanpatten]

I agree about not being tolerant. It might be nice though to make the underlying errors a little more visible. Right now if you're getting an error you have to do a bit of spelunking inside devtools to track down the error message.

There might be some inspiration to be found in how error messages are loaded in when trying to activate a plugin with PHP fatals:

Some of the work the #core-php team is doing around WSODs during PHP upgrades might also be useful.

[danielbachhuber]

So is the issue that the REST API response is prefixed by PHP notice, rather than being proper JSON?

Correct.

I think a proper way to deal with this is trying to catch these errors/warnings in PHP before sending the response if the response is JSON and return a well-formatted JSON error instead. It could be a generic behavior to all REST API endpoints calls.

How would this work in practice, and reliably?

There might be some inspiration to be found in how error messages are loaded in when trying to activate a plugin with PHP fatals:

Notably, it's the client with the error handling in this case. If we wanted to follow this as inspiration, then the error handling would need to go in Gutenberg.

[chrisvanpatten]

@danielbachhuber Yeah, sorry I meant more in the UI inspiration not the technical implementation :)

[danielbachhuber]

By this, it would appear we're trying to see if it's a JSON response before trying to parse. So either the server is misreporting the non-JSON as application/json content type, or we aren't handling the rejection well. What would be a good way of handling the rejection?

Some more user-friendly error handling than "Updating failed." or the other cryptic messages Gutenberg currently provides.

[youknowriad]

@danielbachhuber I was thinking we could use this http://php.net/manual/en/function.set-error-handler.php and return something like { type: 'unknown-error', message } and of course return the response as 500

That way the frontend could catch these errors properly and show a more meaningful message.

[danielbachhuber]

I was thinking we could use this http://php.net/manual/en/function.set-error-handler.php and return something like { type: 'unknown-error', message } and of course return the response as 500

Could be worth exploring. It wouldn't have caught #3102 though, as any sort of error handling would be registered midway through application load. I'm also not sure how reliable set_error_handler() is across systems.

[Clorith]

We also need to take into consideration that requests can fail before hitting the backend.

If the JSON API is unavailable is one of them, but what if your request was blocked by mod_security or similar security implementations? These will usually provide a non-successful status code, and not much more without looking into logs.

As an example, this is a rule blocking a user from saving, publishing, or having content autosaved:

ModSecurity: Access denied with code 403 (phase 2). Match of "within %{tx.allowed_request_content_type}" against "TX:0" required. [file "/usr/local/apache2/conf/modsecurity/base_rules/modsecurity_crs_30_http_policy.conf"] [line "63"] [id "960010"] [msg "Request content type is not allowed by policy"] [data "application/json"] [severity "WARNING"] [tag "POLICY/ENCODING_NOT_ALLOWED"] [tag "WASCTC/WASC-20"] [tag "OWASP_TOP_10/A1"] [tag "OWASP_AppSensor/EE2"] [tag "PCI/12.1"] [hostname "www.domain.com"] [uri "/wp-json/wp/v2/posts/6/autosaves"] [unique_id "W3dK9goASzoAABfURiAAAAA-"]

They are seeing a message that saving failed, unfortunately there's not much information on why it failed, and what actions they need to take to not lose their content.

[danielbachhuber]

It seems https://core.trac.wordpress.org/ticket/44534 could address at least some of these problems. It appears WordPress isn't disabling error reporting like it's expected to, which could be the cause of some of these problems.

[danielbachhuber]

Closing in favor of #10492