WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 11 months ago

Last modified 10 months 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 has-dev-note
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 17 months ago.
38777.2.diff (3.2 KB) - added by dlh 17 months ago.
38777.3.diff (8.9 KB) - added by dlh 12 months ago.
38777.4.diff (7.0 KB) - added by dlh 12 months ago.
Redo of the last patch without package-lock.json
38777.5.diff (7.2 KB) - added by dlh 12 months ago.
38777.6.diff (9.7 KB) - added by johnbillion 11 months ago.
38777.7.diff (9.8 KB) - added by garrett-eclipse 11 months ago.
Minor improvement to docs.

Download all attachments as: .zip

Change History (40)

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


17 months ago

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

  • Component changed from General to REST API

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

  • Keywords needs-unit-tests added

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

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

#11 @dlh
17 months ago

38777.2.diff just includes @since tags.

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


16 months ago

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


15 months ago

#14 @davidbaumwald
15 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
15 months ago

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

#16 @davidbaumwald
15 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
15 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
15 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.


12 months ago

#20 follow-up: @johnbillion
12 months 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
12 months 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
12 months ago

#22 @dlh
12 months 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
12 months ago

Redo of the last patch without package-lock.json

#23 @TimothyBlynJacobs
12 months 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
12 months ago

#24 @dlh
12 months 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
12 months ago

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

#26 @johnbillion
11 months ago

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

#27 follow-up: @johnbillion
11 months 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
11 months ago

Minor improvement to docs.

#28 @johnbillion
11 months 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
11 months 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
11 months 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
11 months ago

In 49116:

General: Docblock improvements for the WP_Error class.

Props garrett-eclipse

See #38777

#32 @desrosj
10 months ago

  • Keywords needs-dev-note added

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

Note: See TracTickets for help on using tickets.