Plugin errors

[oguzkocer]

Builds on #91.

Implements errors for plugins as found in the source code at the time of this PR. It also implements integration tests for the error types that are feasible to test.

---

The PR introduces WPInternalErrorCode, which at the moment is a dangling enum - meaning it is not used anywhere yet. These are internal errors in the WordPress implementation that - in theory - shouldn't be visible to the REST client. However, there are several checks in the codebase where the REST endpoint will return an internal error directly. Most of the time, there are several prior checks that should prevent the internal error from being returned, however it's still possible to get these internal errors.

Occasionally, these internal errors will be directly returned by the REST implementation, such is the case for fs_unavailable error. For example here, the error is returned without being wrapped in a rest_{error_type} variant.

The question is, how do we want to deal with these error types?

My take is that, any rest_{error_type} should go into the WPRestErrorCode type. That makes it a one to one mapping between the two and significantly simplify things.

Any other WP_Error should go into the WPInternalErrorCode even if it's returned from the REST implementation - meaning it's implemented in one of the class-wp-rest-{endpoint}-controller.php classes in the source code.

I think directly returning an internal error from the server is an edge case. Furthermore, the error types that are being handled also seem to be edge cases themselves. So, including them in the WPInternalErrorCode shouldn't create any additional headache to the clients. So, in my opinion having a one to one mapping as such will be really beneficial for us.

If we do keep this setup, the next question is, do we want to parse the response for WPInternalErrorCode?

We probably should change the WPRestErrorWrapper to something like:

pub enum WPRestErrorWrapper {
    RestError(WPRestError),
    InternalError(WPInternalError),
    Unrecognized(WPUnrecognizedError),
}

However, I am a little on the fence about it, because I am concerned that this will "force" clients to handle the WPInternalErrorCode variant as well. This is something similar to what @crazytonyli brought up recently in a Slack conversation about providing a SparseUser as a fallback when we are parsing for UserWithEditContext.

I'd love to hear your thoughts on this.

My take is, we should add the InternalError variant to WPRestErrorWrapper and add public documentation about it explaining that it shouldn't be possible to retrieve in most cases. I don't see a perfect solution to this, and adding the variant still seems to be the right way to go.

Note that, I'll not be making any changes to this PR as a result of this discussion. I'll open a separate PR for it once we settle on something.

---

To Test

• make test-server && make dump-mysql && make backup-wp-content-plugins
• cargo test --test '*' -- --nocapture --test-threads 1

[crazytonyli]

IMO this new error should be placed at the same level as RestError, instead of inside RestError, and the consumer apps should handle (i.e. simply showing an error message) these "internal errors".

If I understand it correctly, the new WPInternalErrorCode are for errors that wordpress system errors where users can do nothing about: The site is configured wrong and they'll have to ask their host for help. RestError case is for any rest_ error code, which is the case right now. I don't think we should mix these two together.

Also, IMO the REST client and consumer apps should handle these errors. The apps can show an generic "We received an error from your site. Please contact your host for help. Error code: <...>".

I wonder if we can change the name InternalError to something like SiteError or SystemError. My first impression of Internal is the internal of wordpress-rs the client library, rather than the wordpress site.

[oguzkocer]

IMO this new error should be placed at the same level as RestError, instead of inside RestError, and the consumer apps should handle (i.e. simply showing an error message) these "internal errors".

It's not placed anywhere at the moment - and yes, it wouldn't be placed under RestError. I forgot that we called the wrapper WPRestErrorWrapper and mindlessly copied it.

If I understand it correctly, the new WPInternalErrorCode are for errors that wordpress system errors where users can do nothing about: The site is configured wrong and they'll have to ask their host for help. RestError case is for any rest_ error code, which is the case right now. I don't think we should mix these two together.

Although this is likely the case for most errors, unfortunately it's not a given that all errors are a result off a misconfiguration. As far as I can tell, the rest controller will go through the pre-checks for an endpoint and return an error before calling the function from wp-admin. If that function returns an error, that error is returned directly to the client. (not wrapped in a rest error) So, if the pre-checks are not exhaustive, we might get a relevant error from wp-admin function.

Having said that, I do agree that developers will likely handle this as a whole package, maybe showing the error message they receive from the server. They might handle some specific error types, which is why I'd still like to provide the enum.

I wonder if we can change the name InternalError to something like SiteError or SystemError. My first impression of Internal is the internal of wordpress-rs the client library, rather than the wordpress site.

I agree, but I am not sure of a better alternative though. Maybe we can call it WpAdminError, but I don't think all of these errors are triggered from wp-admin.

Maybe WpError? It's generic, but matches the WordPress source code. The only issue with that is the rest errors are also handled as WpErrors.

[crazytonyli]

My preference would be a more specific name. I'm terrible at naming things though. SiteInternalError maybe?

[oguzkocer]

My preference would be a more specific name. I'm terrible at naming things though. SiteInternalError maybe?

Maybe? @jkmassel any input?