Add method to merge one WP_Error into another

[rmccue]

WP_Error can store multiple error messages and codes, but this requires building the error object manually. If you have two existing error instances and want to merge them into a single one, you have to do this manually:

foreach ( $other_errors as $code => $messages ) {
	foreach ( $messages as $message ) {
		$error->add( $code, $message );
	}
	if ( $error_data = $error->get_error_data( $code ) ) {
		$error->add_data( $error_data, $code );
	}
}

While this multi-error feature isn't widely used, it's very helpful in the REST API, where multi-errors are natively supported via an additional_errors key in the error response.

[whyisjake]

Hey @rmccue, this does seem like a great request. I'm going to move this under the purview of the REST API component where it might get a few more eyes.

[dlh]

I've needed this functionality on occasion as well. I have an implementation and tests that I'd be happy to try to put into a core patch for consideration for 5.5.

[desrosj]

Thanks @dlh! I am moving to the 5.5 milestone for now and assigning to you.

[desrosj]

Thanks @dlh! I am moving to the 5.5 milestone for now and assigning to you.

[TimothyBlynJacobs]

One of the questions here I think would be what to do with the error data. As far as I know, WP_Error only supports one data entry per-code.

So what should happen if two WP_Error objects both have data for the same code?

[dlh]

38777.diff​ takes a shot at the new WP_Error::merge() method, working basically as outlined in the ticket description, along with tests.

@TimothyBlynJacobs Your question about conflicting error data is a good one. This patch will cause incoming error data to overwrite existing error data, which was an arbitrary choice; I'm open to other ideas.

As mentioned, I'd previously implemented this method for other purposes in my own work. As part of that work, I've found it convenient to also have an WP_Error::export() method, which copies the current error into another one. It's sometimes been semantically helpful to me to think of either performing an action on an error (merge()) or causing an effect on another error (export()).

I've included an export() with the patch. But, no one asked for it, so I'm also happy to remove it.

[dlh]

38777.2.diff​ just includes @since tags.

[davidbaumwald]

@dlh What else is needed to help this one move forward? The deadline for this one is Beta 1 for 5.5 in just a few days, otherwise, it will have to be bumped.

[dlh]

@davidbaumwald I think the current patch just needs a review and decision from a maintainer.

[davidbaumwald]

@dlh, thanks for the feedback! @desrosj and @SergeyBiryukov as the maintainers here, any thoughts on including this in 5.5 in the time allotted?

[TimothyBlynJacobs]

From the REST API perspective, I think it would be most helpful if we could merge the error data somehow.

Perhaps if both error datas are an array, merge them? With the $from error taking precedence?

Alternately, perhaps we could try actually supporting having multiple data entries for each error code? Perhaps with an additional_data property?

Something to keep in mind is that the REST API uses a status key in the error data to set the response status code.

[whyisjake]

Chatting with @TimothyBlynJacobs, I don't think we can get this wrapped ahead of the 5.5 beta. Punting to 5.6.

[johnbillion]

Are there instances in the REST API where this would be useful right away?

Minor: I'd like the method naming to be a little clearer, eg. merge_from() and export_to().

[TimothyBlynJacobs]

Replying to johnbillion:

Are there instances in the REST API where this would be useful right away?

Yeah, we have an issue with how we handle errors returned from validate_callback and sanitize_callback in WP_REST_Request::has_valid_params and ::sanitize_params(). We only grab the first error message as a string.

But ideally, we'd combine all of those errors into one error object.

[dlh]

38777.3.diff​ adds support for multiple error data, as discussed above and in Slack. It also renames the new methods as suggested in comment:20.

@TimothyBlynJacobs I made one deviation from our last discussion, but I'm happy to reconsider it. It seemed to me as I was working on the patch that it would be a little more straightforward for the new property to be just a record of all the error data added over time, rather than shifting messages from one property to another, which also simplifies get_all_error_data(). Let me know what you think!

[dlh]

Redo of the last patch without package-lock.json

[TimothyBlynJacobs]

I think the worry about not reading from the old property is if people write to error_data directly. If we do the extra work of shifting messages, get_all_error_data() should, I think, return the correct values. Whereas if we don't do the message shifting, a direct write to error_data would be lost.

[dlh]

Great point. It would be unexpected for get_all_error_data() to not include whatever was in error_data, however it got there. 38777.5.diff​ makes the switch.

[TimothyBlynJacobs]

This looks great to me! @johnbillion how does it look to you?

[johnbillion]

38777.6.diff​ adds a test for the direct error_data property manipulation that Timothy mentioned above, and tweaks a bunch of docblocks.

[garrett-eclipse]

Minor improvement to docs.

[johnbillion]

In 49115:

General: Introduce the ability to merge multiple WP_Error objects into one another, and to store more than one item of data for an error.

This allows multiple errors to be instantiated independently but collected into one without having to manually combine their properties.

Props rmccue, dlh, TimothyBlynJacobs

Fixes #38777

[garrett-eclipse]

Replying to johnbillion:

38777.6.diff​ adds a test for the direct error_data property manipulation that Timothy mentioned above, and tweaks a bunch of docblocks.

Looks good @johnbillion I made a minor improvement in 38777.7.diff; 1. most-recently shouldn't be hyphenated, 2. Add more description to From/To than just the single words.

[garrett-eclipse]

Oops looks like I wasn't fast enough before your commit @johnbillion.
My changes were just most-recently => most recently.
And expand on this part of the copy_errors docblock;

* @param WP_Error $from The WP_Error to copy from.
* @param WP_Error $to   The WP_Error to copy to.

Not sure if worth a follow-up commit.

[johnbillion]

In 49116:

General: Docblock improvements for the WP_Error class.

Props garrett-eclipse

See #38777

[desrosj]

This would be nice to document in the miscellaneous dev note.

[daisyo]

Dev note published: ​https://make.wordpress.org/core/2020/11/20/miscellaneous-developer-focused-changes-in-wordpress-5-6/