Make WordPress Core

Opened 4 years ago

Closed 3 months ago

Last modified 3 months ago

#48823 closed enhancement (fixed)

Collect all REST API meta errors at once

Reported by: timothyblynjacobs's profile TimothyBlynJacobs Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.5 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit needs-dev-note
Focuses: Cc:

Description

Currently, when updating meta, we return immediately after encountering a WP_Error. If there are issues with multiple meta keys, you have to correct each value and send a new request one at a time. I think it'd be ideal if we could process all the meta fields and then return all the errors that are encountered at once.

WP_Error supports multiple error messages with the same code, but it does not support multiple bits of error_data for the same code. This would be a problem for the errors that include the affected meta key in the error data. Right now, the REST API duplicates this data for each error with the same code ( WP_REST_Server::error_to_response ). I think we'd need to find a way to retain the error message and data correlation in WP_Error.

Change History (31)

#1 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Awaiting Review to 5.7

Going to try taking a look at this for 5.7 now that we can merge WP_Error objects and keep additional data.

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


3 years ago

#3 @hellofromTonya
3 years ago

  • Keywords needs-patch added

Per Timothy in today's core scrub:

Needs patch probably. Planning on working on it this week

This ticket was mentioned in PR #877 on WordPress/wordpress-develop by TimothyBJacobs.


3 years ago
#4

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

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


3 years ago

#6 @hellofromTonya
3 years ago

Per Timothy, this ticket is blocked by #46191.

#7 @TimothyBlynJacobs
3 years ago

  • Milestone changed from 5.7 to 5.8

Punting as multi error data isn't solved yet.

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


3 years ago

#9 @desrosj
3 years ago

  • Milestone changed from 5.8 to Future Release

This one has not seen any activity during the 5.8 cycle. I'm going to punt to Future Release to prevent repeated punting. But it can be moved back to a numbered milestone at any time.

#10 @spacedmonkey
15 months ago

This change looks good @TimothyBlynJacobs . I think we can get this in the next release.

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


11 months ago

#12 @spacedmonkey
11 months ago

  • Milestone changed from Future Release to 6.4
  • Owner set to spacedmonkey
  • Status changed from new to assigned

#13 @oglekler
9 months ago

The patch is still applicable, no rebase is needed. @TimothyBlynJacobs and @spacedmonkey what is the next step, testing? If so, we need testing info.

#14 @spacedmonkey
9 months ago

@TimothyBlynJacobs Can you add testing instructions.

#15 @oglekler
8 months ago

  • Keywords needs-testing-info added

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


8 months ago

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


8 months ago

#18 @hellofromTonya
8 months ago

In discussing with @azaozz, we both have concerns of potential regressions and/or unexpected behaviors due to the code not bailing out immediately with an error, i.e. being allowed to continue processing.

Given how important the REST API is, we're thinking this one is a good early candidate. Rather than committing it right before 6.4 Beta 1, instead commit it when 6.5 Alpha opens.

I noted:

early would give it a long soak time to potentially discover if there are indeed regressions or unexpected behaviors. For example, the Gutenberg plugin uses the REST API. It's a good candidate to flush out if there are any unknown issues.

@azaozz noted:

Yea, thinking it may be a bit risky, and testing it and confirming it doesn't bring a regression would be great

@TimothyBlynJacobs @spacedmonkey what do you both think? Are there risks?

#19 @spacedmonkey
8 months ago

  • Milestone changed from 6.4 to 6.5

Let's punt.

#20 @spacedmonkey
8 months ago

  • Owner spacedmonkey deleted

#21 @spacedmonkey
4 months ago

  • Owner set to spacedmonkey

#22 @spacedmonkey
4 months ago

@hellofromTonya I am not worried about this change. Multiple keys could be updated at once before. It could have error mid way through the process. But after this change, values will be updated to the key that are getting valid values and how all errored keys. This is much cleaner and predictable behaviour.

I want to get this committed early. Are you happy for me to commit?

#23 @TimothyBlynJacobs
4 months ago

I think this is fine to commit now.

#24 @hellofromTonya
4 months ago

  • Keywords commit added

Sorry for my delay in responding @spacedmonkey. Would have preferred this one to be committed much earlier in the Alpha cycle. That said, given there's consensus of little to no risks, seems okay to commit. Then through Beta and RC, can be monitored if there is any feedback.

I think this is fine to commit now.

Marking for commit per @TimothyBlynJacobs feedback comment:23.

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


3 months ago

#26 @audrasjb
3 months ago

  • Keywords needs-testing-info removed

@spacedmonkey are you available to commit this patch in the next few hours, before beta 1? Thanks!

#28 @spacedmonkey
3 months ago

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

In 57611:

REST API: Improve error handling in REST meta fields

This update modifies the error handling mechanism in the REST API meta fields functionality. Instead of halting execution and returning on the first encountered error, it now collects all errors in a WP_Error object and continues execution. Thus, this enhancement enables handling and displaying of multiple errors in a single response, improving the debugging process.

Props TimothyBlynJacobs, spacedmonkey, hellofromTonya, oglekler.
Fixes #48823.

#29 @spacedmonkey
3 months ago

  • Keywords needs-dev-note added

@spacedmonkey commented on PR #6099:


3 months ago
#30

Committed.

@spacedmonkey commented on PR #877:


3 months ago
#31

Committed

Note: See TracTickets for help on using tickets.