Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Present a more specific and descriptive error message when a REST API request fails #10492

Open
danielbachhuber opened this issue Oct 11, 2018 · 13 comments
Labels
Needs Copy Review Needs review of user-facing copy (language, phrasing) Needs Technical Feedback Needs testing from a developer perspective. REST API Interaction Related to REST API [Type] Enhancement A suggestion for improvement.
Milestone

Comments

@danielbachhuber
Copy link
Member

Currently, there are two similar scenarios where a REST API request can fail:

  1. The REST API isn't enabled (because it's been disabled by a plugin, rewrites are missing, etc.). Originally Detect and show a notice when REST API is disabled #8549
  2. The server returned a malformed JSON response (because of PHP error notices, etc.). Originally Improve resilience to PHP Notices and Warnings #4936

In both of these cases, Gutenberg receives a response but is unable to deserialize it. As a result, the end user sees a generic 'Updating failed.' error message.

While hopefully many of these failures will be addressed with #10476, it would be ideal if Gutenberg presented the end user a more specific and descriptive error message.

@aduth identified (#4936 (comment)) the following as a possible place we could interpret failed responses:

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

In order to resolve this issue, we'd need to catch the two scenarios listed above, and display a more specific and descriptive error message accordingly.

@danielbachhuber danielbachhuber added [Type] Enhancement A suggestion for improvement. Backwards Compatibility Issues or PRs that impact backwards compatability REST API Interaction Related to REST API labels Oct 11, 2018
@danielbachhuber danielbachhuber added this to the WordPress 5.0 milestone Oct 11, 2018
@danielbachhuber danielbachhuber added the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Oct 11, 2018
@danielbachhuber
Copy link
Member Author

Flagging with Needs Copy Review because we need direction on human-friendly language for the aforementioned two scenarios.

@michelleweber
Copy link

What are the user's next steps in those two cases? Ideally, the error provides some detail on the way forward along with detail/context for the actual issue.

If you can write the ideal error messages from the technical perspective -- full of detail and without worrying about human-friendlieness -- we can re-shape them from there.

@danielbachhuber
Copy link
Member Author

What are the user's next steps in those two cases?

Great question. I'll drop this in the #hosting-community channel to see if anyone can weigh in with the guidance they normally offer.

Ideally, the error provides some detail on the way forward along with detail/context for the actual issue.

I agree, although we may be a bit space constrained. Here's some relevant prior art from core:

image

If we can give the user a more specific error message they can Google, they could end up at documentation that way.

@jadonn
Copy link

jadonn commented Oct 12, 2018

From a hosting support standpoint, it may be worthwhile to dump the raw response in the absence of a better error displaying solution. I am not sure if many hosting companies' support personnel will know to either check the browser's console log for error output or to use their browsers' network/waterfall tool to look at the raw responses for API request.

Perhaps the output could be wrapped in a box like in the prior art that says something like "The server returned an unexpected response. Please confirm if the operation was successful. The response was: [raw response output]".

I think from a hosting support standpoint putting the raw errors, which I feel would be most Google-able, may be the way to go. If nothing else, the file path in the notices/warnings/errors would let the support person know what files, plugins, or themes are worth investigating.

I think "next steps" in this kind of case would be to check the PHP version in use since using deprecated code in a newer version of PHP will case notices and warnings, review the plugins in themes that are causing the issue, potentially disabling notices and warnings, or removing the plugin (which is an undesirable last resort I admit). It's tough to have a consistent "do this thing to fix this problem", which is why I think having the raw response show may be worthwhile.

@Clorith
Copy link
Member

Clorith commented Oct 12, 2018

I think the HTTP status may be valuable here, in the case where it's blocked (for example by mod_sec) then the HTTP status code generally indicates, although in an indirect way, what happened. Mostly because mod_sec often end up giving a 401 Unauthorized for example, while generic code errors result in 500 (to name two quick examples).

@danielbachhuber
Copy link
Member Author

Thanks @jadonn @Clorith, this was super helpful.

As a next step, it would be great if someone might prepare three real-world examples of errors and what their error messages might be.

@danielbachhuber
Copy link
Member Author

Here's an example: #3948 (comment)

image

It looks like we'd need to accommodate multiple REST API failures.

@danielbachhuber danielbachhuber removed the Backwards Compatibility Issues or PRs that impact backwards compatability label Oct 17, 2018
@dkanchev
Copy link

I agree with @Clorith that the HTTP code needs to be displayed. I am sure in some cases some servers will generate 502 gateway timeout errors. Thus, you should not handle only 500 and 401/403 but all other 5xx and 4xx errors as well.

@danielbachhuber
Copy link
Member Author

Given the timeline for shipping 5.0, it's unlikely this particular enhancement will make it for this release. Reassigning to WP 5.1 Onwards to pick up in the future.

@kathrynwp
Copy link

Heya @danielbachhuber – checking in to see whether this is something you'd like to pick up again if it's still an issue in 6.0.1? Thanks!

@danielbachhuber
Copy link
Member Author

checking in to see whether this is something you'd like to pick up again if it's still an issue in 6.0.1? Thanks!

@kathrynwp I'm not sure if it's still an issue. I imagine it would be helpful to have if it was, though.

@kathrynwp
Copy link

@danielbachhuber Please feel free to test to see whether it's still an issue, otherwise we can go ahead and close this out. Thanks!

@kathrynwp kathrynwp added the Needs Testing Needs further testing to be confirmed. label Jul 12, 2022
@danielbachhuber
Copy link
Member Author

Please feel free to test to see whether it's still an issue, otherwise we can go ahead and close this out. Thanks!

@kathrynwp Sure.

It appears the first two scenarios are resolved. When a server returns a malformed JSON response, or the REST API is disabled, the error message indicates such:

image

image

I created the two scenarios by adding the following to wp-content/mu-plugins/local.php:

// Malformed JSON response.
echo 'sosos';
// Disable REST API.
add_action(
	'plugins_loaded',
	function() {
		remove_action( 'init', 'rest_api_init' );
	}
);

However, there's a third scenario I just discovered.

After I loaded Gutenberg, I added some invalid PHP to my mu-plugin wp-content/mu-plugins/local.php:

<?php

dfdfgdf

When I tried to save my already-open post, WordPress correctly caught the fatal error and returned it in the response, but Gutenberg gave me a generic "Updating failed" message:

image

Here's a fix I couldn't quite get working:

diff --git a/packages/api-fetch/src/utils/response.js b/packages/api-fetch/src/utils/response.js
index b5be8009fd..855c0b2d6e 100644
--- a/packages/api-fetch/src/utils/response.js
+++ b/packages/api-fetch/src/utils/response.js
@@ -17,6 +17,10 @@ const parseResponse = ( response, shouldParseResponse = true ) => {
 			return null;
 		}
 
+		if ( response.status === 500 ) {
+			Promise.reject( response );
+		}
+
 		return response.json ? response.json() : Promise.reject( response );
 	}
 
@@ -31,7 +35,7 @@ const parseResponse = ( response, shouldParseResponse = true ) => {
  * @return {Promise<any>} Parsed response.
  */
 const parseJsonAndNormalizeError = ( response ) => {
-	const invalidJsonError = {
+	let invalidJsonError = {
 		code: 'invalid_json',
 		message: __( 'The response is not a valid JSON response.' ),
 	};
@@ -40,7 +44,15 @@ const parseJsonAndNormalizeError = ( response ) => {
 		throw invalidJsonError;
 	}
 
-	return response.json().catch( () => {
+	return response.json().then( (data) => {
+		if (data.code && data.message) {
+			invalidJsonError = {
+				code: data.code,
+				message: data.message,
+			};
+		}
+		throw invalidJsonError;
+	} ).catch( () => {
 		throw invalidJsonError;
 	} );
 };

@kathrynwp kathrynwp added Needs Technical Feedback Needs testing from a developer perspective. and removed Needs Testing Needs further testing to be confirmed. labels Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Copy Review Needs review of user-facing copy (language, phrasing) Needs Technical Feedback Needs testing from a developer perspective. REST API Interaction Related to REST API [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

6 participants