WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 8 days ago

#38777 assigned feature request

Add method to merge one WP_Error into another

Reported by: rmccue Owned by: dlh
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 (2)

38777.diff (3.2 KB) - added by dlh 3 months ago.
38777.2.diff (3.2 KB) - added by dlh 3 months ago.

Download all attachments as: .zip

Change History (20)

#1 @desrosj
16 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.


3 months ago

#3 @whyisjake
3 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
3 months ago

  • Component changed from General to REST API

#5 @dlh
3 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
3 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
3 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
3 months ago

  • Keywords needs-unit-tests added

#9 @TimothyBlynJacobs
3 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
3 months ago

#10 @dlh
3 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
3 months ago

#11 @dlh
3 months ago

38777.2.diff just includes @since tags.

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


5 weeks ago

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


2 weeks ago

#14 @davidbaumwald
2 weeks 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
2 weeks ago

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

#16 @davidbaumwald
12 days 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
12 days 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
8 days 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.

Note: See TracTickets for help on using tickets.