WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 12 days ago

Last modified 12 days ago

#38777 closed feature request (fixed)

Add method to merge one WP_Error into another

Reported by: rmccue Owned by: johnbillion
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

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.

Attachments (7)

38777.diff (3.2 KB) - added by dlh 6 months ago.
38777.2.diff (3.2 KB) - added by dlh 6 months ago.
38777.3.diff (8.9 KB) - added by dlh 2 weeks ago.
38777.4.diff (7.0 KB) - added by dlh 2 weeks ago.
Redo of the last patch without package-lock.json
38777.5.diff (7.2 KB) - added by dlh 13 days ago.
38777.6.diff (9.7 KB) - added by johnbillion 12 days ago.
38777.7.diff (9.8 KB) - added by garrett-eclipse 12 days ago.
Minor improvement to docs.

Download all attachments as: .zip

Change History (38)

#1 @desrosj
19 months ago

  • Keywords needs-patch 2nd-opinion added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to feature request

This ticket was mentioned in Slack in #core by noisysocks. View the logs.


6 months ago

#3 @whyisjake
6 months ago

  • Keywords 2nd-opinion removed

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.

#4 @whyisjake
6 months ago

  • Component changed from General to REST API

#5 @dlh
6 months ago

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.

#6 @desrosj
6 months ago

  • Milestone changed from Future Release to 5.5

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

#7 @desrosj
6 months ago

  • Component changed from REST API to General
  • Owner set to dlh
  • Status changed from new to assigned

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

#8 @johnbillion
6 months ago

  • Keywords needs-unit-tests added

#9 @TimothyBlynJacobs
6 months ago

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
6 months ago

#10 @dlh
6 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

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
6 months ago

#11 @dlh
6 months ago

38777.2.diff just includes @since tags.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


4 months ago

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 months ago

#14 @davidbaumwald
4 months ago

@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.

#15 @dlh
4 months ago

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

#16 @davidbaumwald
4 months ago

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

#17 @TimothyBlynJacobs
4 months ago

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.

#18 @whyisjake
4 months ago

  • Milestone changed from 5.5 to 5.6

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

This ticket was mentioned in Slack in #core-restapi by dlh. View the logs.


4 weeks ago

#20 follow-up: @johnbillion
4 weeks ago

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().

#21 in reply to: ↑ 20 @TimothyBlynJacobs
3 weeks ago

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
2 weeks ago

#22 @dlh
2 weeks ago

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
2 weeks ago

Redo of the last patch without package-lock.json

#23 @TimothyBlynJacobs
2 weeks ago

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
13 days ago

#24 @dlh
13 days ago

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.

#25 @TimothyBlynJacobs
13 days ago

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

#26 @johnbillion
12 days ago

  • Owner changed from dlh to johnbillion
  • Status changed from assigned to reviewing

@johnbillion
12 days ago

#27 follow-up: @johnbillion
12 days ago

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
12 days ago

Minor improvement to docs.

#28 @johnbillion
12 days ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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

#29 in reply to: ↑ 27 @garrett-eclipse
12 days ago

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.

#30 @garrett-eclipse
12 days ago

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.

#31 @johnbillion
12 days ago

In 49116:

General: Docblock improvements for the WP_Error class.

Props garrett-eclipse

See #38777

Note: See TracTickets for help on using tickets.