WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#35425 new enhancement

Return HTTP status code in WP_Error objects

Reported by: danielbachhuber Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: REST API Keywords: needs-patch
Focuses: Cc:
PR Number:

Description

The REST API has an existing pattern of using a HTTP status code to better describe the nature of a WP_Error object:

if ( ! empty( $request['id'] ) ) {
	return new WP_Error( 'rest_post_exists', __( 'Cannot create existing post.' ), array( 'status' => 400 ) );
}

Throughout WordPress, it would improve the quality of error reporting to include an equivalent HTTP status when generating a new WP_Error object.

This ticket can serve as placeholder for improvements to this effect.

Previously: https://github.com/WP-API/WP-API/issues/153#issuecomment-170734687

Attachments (2)

35425.wp-admin.diff (39.1 KB) - added by voldemortensen 4 years ago.
35425.wp-admin.1.diff (41.2 KB) - added by voldemortensen 4 years ago.

Download all attachments as: .zip

Change History (13)

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


4 years ago

#2 @voldemortensen
4 years ago

35425.wp-admin.diff updates all returned WP_Errors in the wp-admin dir.

#3 @knutsp
4 years ago

After r36294: Should the new WP_Http::CONSTANTs be used?

#4 @voldemortensen
4 years ago

Feels like cruft to me, but if that's the direction its headed I can update the patch.

#5 @danielbachhuber
4 years ago

I think it makes sense to use the new class of constants.

#6 @voldemortensen
4 years ago

I still think its a load of cruft, but here's an updated patch.

#7 follow-up: @rmccue
4 years ago

I don't think we can change the data already being passed in WP_Error objects, as this'll break compatibility. Also, we can skip specifying WP_HTTP::INTERNAL_SERVER_ERROR explicitly, just falling back to the default should be fine.

#8 in reply to: ↑ 7 ; follow-up: @danielbachhuber
4 years ago

Replying to rmccue:

I don't think we can change the data already being passed in WP_Error objects, as this'll break compatibility.

Can you clarify what the alternative should be? I thought changing the data included in WP_Error objects was the original intent.

#9 @dd32
4 years ago

Altering the data returned by existing WP_Errors is going to be a no-go.

Potentially it could be "upgraded" to accept array( 'status' => 404, 'data' => ... ) and return the data element as the get_error_data() and status via get_error_type() / get_error_status().

This will need some clear cut explanations of what HTTP code represents what as well, for example, a 404 being returned on download failure (as in the patch above) is the closest to what was actually encountered, but probably isn't what should be returned to the Browser or REST client (but it might be?).

#10 in reply to: ↑ 8 @rmccue
4 years ago

Replying to danielbachhuber:

Can you clarify what the alternative should be? I thought changing the data included in WP_Error objects was the original intent.

If the error object already has data, we can't change that (unless it's an assoc array we can just add a key to), since plugins may be relying on accessing that data as-is. For errors without data attached, we can add it, since plugins can't be relying on something that's not there. (Technically they could be relying on it being empty, but I doubt anyone ever does that.)

Replying to dd32:

Potentially it could be "upgraded" to accept array( 'status' => 404, 'data' => ... ) and return the data element as the get_error_data() and status via get_error_type() / get_error_status().

I think at that point, we'd be better off just adding a separate status property. Incidentally, I'm fine with that, but we'd need to keep BC for having it in data anyway, so I don't think it's worth the cruft.

#11 @joehoyle
3 years ago

Given no movement on this, and I don't see a great way to solve the original issue here I'm +1 on closing this out.

Note: See TracTickets for help on using tickets.