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

Improve error message on post save fail #14644

Closed
spacedmonkey opened this issue Mar 26, 2019 · 10 comments · Fixed by #15657

Comments

@spacedmonkey
Copy link
Contributor

commented Mar 26, 2019

Is your feature request related to a problem? Please describe.
Currently, when a post fails to save it displayed the message. 'Updating failed'. However in #44625 (See #45933), if the json api fails it will return a a nice formatted json. An example of which.

{
   additional_errors: [],
   code: "wp_die",
   data: {status: 500},
   status: 500,
   message: "<h1>Error establishing a database connection</h1>"
}

In WordPress 5.2, #44458 even php fatal error will return valid json.

Describe the solution you'd like
This message returned to users should display the message returned in json. This will help users debug the issue they are having.

@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

If useful, we could also return a header, like WP_DIE: true, so it is easier to know to display this message.

@aduth

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Related: #4590 (comment)

Implementation notes:

  • From the JSON blob, the message would need to be manipulated to remove the h1. Is it safe to assume the plaintext message would always be wrapped as h1?
  • Notice creation occurs here:
    • const notifyFailArgs = getNotificationArgumentsForSaveFail( {
      post,
      edits,
      error,
      } );
      if ( notifyFailArgs.length > 0 ) {
      yield dispatch(
      'core/notices',
      'createErrorNotice',
      ...notifyFailArgs
      );
      }
    • /**
      * Builds the fail notification arguments for dispatch.
      *
      * @param {Object} data Incoming data to build the arguments with.
      *
      * @return {Array} Arguments for dispatch. An empty array signals no
      * notification should be sent.
      */
      export function getNotificationArgumentsForSaveFail( data ) {
      const { post, edits, error } = data;
      if ( error && 'rest_autosave_no_changes' === error.code ) {
      // Autosave requested a new autosave, but there were no changes. This shouldn't
      // result in an error notice for the user.
      return [];
      }
      const publishStatus = [ 'publish', 'private', 'future' ];
      const isPublished = publishStatus.indexOf( post.status ) !== -1;
      // If the post was being published, we show the corresponding publish error message
      // Unless we publish an "updating failed" message
      const messages = {
      publish: __( 'Publishing failed' ),
      private: __( 'Publishing failed' ),
      future: __( 'Scheduling failed' ),
      };
      const noticeMessage = ! isPublished && publishStatus.indexOf( edits.status ) !== -1 ?
      messages[ edits.status ] :
      __( 'Updating failed' );
      return [ noticeMessage, { id: SAVE_POST_NOTICE_ID } ];
      }
@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

@aduth

I have been working on how WordPress handles fatal errors. See these tickets from core #45974 #45975 #41655.

So the reason for the call to wp_die maybe many different reasons and the content of the error message will different depending on the which error is triggered. One thing that is more predictable is the error code. Maybe this, with a simple switch statement could be used to give more detail on the type of error.

@aduth

This comment has been minimized.

Copy link
Member

commented May 2, 2019

One thing that is more predictable is the error code. Maybe this, with a simple switch statement could be used to give more detail on the type of error.

I could see this working well.

As a plan for action, it may require:

  • Auditing to discover which response codes are possible (or at least reasonable to account for as a reason for save failure)
  • Devising copy for each to present to the user as both accurate and well-understood (may be good to use "Needs Copy Feedback" label or otherwise involve some copywriters).

Example:

503 -> "Updating failed. The site is briefly unavailable for maintenance."

@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I think there are some key error states we need to handle here.

Not all of these return a usable error code, but this is easy to change.
Also, if the rest api can return WP_Error object, for different reasons, like invalid params or auth errors. If those errors are not displaying a usable message, then that should be handled.

But now there are is a format of these error message, this can be handled in one place and with a simple switch statement.

@pbiron

This comment has been minimized.

Copy link

commented May 13, 2019

I think there are some key error states we need to handle here.

Another one to add to the list (it just happened to me):

  • login cookie has expired but haven't yet gotten the Heartbeat API tick for the pop-up re-login
@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Yeah, these are two examples of errors rest api that allow should be handled.

{
    "code": "rest_cookie_invalid_nonce",
    "message": "Cookie nonce is invalid",
    "data": {
        "status": 403
    }
}

and

{
    "code": "rest_cannot_edit",
    "message": "Sorry, you are not allowed to edit this post.",
    "data": {
        "status": 401
    }
}
@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Some more examples of error output.

{
    "code": "internal_server_error",
    "message": "The site is experiencing technical difficulties.",
    "data": {
        "status": 500
    },
    "additional_errors": []
}
{
    "code": "wp_die",
    "message": "Briefly unavailable for scheduled maintenance. Check back in a minute.",
    "data": {
        "status": 503
    },
    "additional_errors": []
}
{
    "code": "rest_post_invalid_id",
    "message": "Invalid post ID.",
    "data": {
        "status": 404
    }
}
{
    "code": "wp_die",
    "message": "<h1>Error establishing a database connection</h1>",
    "data": {
        "status": 500
    },
    "additional_errors": []
}
{
    "code": "wp_die",
    "message": "<p><code>No database selected</code></p>\n<h1>Can&#8217;t select database</h1>\n<p>We were able to connect to the database server (which means your username and password is okay) but not able to select the <code></code> database.</p>\n<ul>\n<li>Are you sure it exists?</li>\n<li>Does the user <code>wordpress</code> have permission to use the <code></code> database?</li>\n<li>On some systems the name of your database is prefixed with your username, so it would be like <code>username_</code>. Could that be the problem?</li>\n</ul>\n<p>If you don&#8217;t know how to set up a database you should <strong>contact your host</strong>. If all else fails you may find help at the <a href=\"https://wordpress.org/support/forums/\">WordPress Support Forums</a>.</p>\n",
    "data": {
        "status": 500
    },
    "additional_errors": []
}

@aduth After reviewing the messages here I think it should work in the following page.

  1. Check for error status code.
  2. Check if valid json
  3. Check if message is set and not empty.
  4. Check if string contains html.
  5. If string contains html, then default to generic error message. Updating failed
  6. If just string, output this string as error message.

This means, we don't have to double to effort with new translatable strings.

@spacedmonkey spacedmonkey referenced this issue May 15, 2019
5 of 5 tasks complete
@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

PR created #15657

@swissspidy

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Closing as the PR has been merged.

@swissspidy swissspidy closed this Jul 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.