Open danielbachhuber opened 6 years ago
Flagging with Needs Copy Review
because we need direction on human-friendly language for the aforementioned two scenarios.
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.
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:
If we can give the user a more specific error message they can Google, they could end up at documentation that way.
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.
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).
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.
Here's an example: https://github.com/WordPress/gutenberg/issues/3948#issuecomment-429345198
It looks like we'd need to accommodate multiple REST API failures.
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.
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.
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!
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.
@danielbachhuber Please feel free to test to see whether it's still an issue, otherwise we can go ahead and close this out. Thanks!
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:
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:
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;
} );
};
Currently, there are two similar scenarios where a REST API request can fail:
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 (https://github.com/WordPress/gutenberg/issues/4936#issuecomment-420375974) the following as a possible place we could interpret failed responses:
https://github.com/WordPress/gutenberg/blob/186716f1e8a0ca9357d1b240f0d23d500251ad4a/packages/api-fetch/src/index.js#L54-L60
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.