WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#34999 closed enhancement (wontfix)

Calling wp_die() in a REST API request should return valid JSON

Reported by: joehoyle Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

See https://github.com/WP-API/WP-API/issues/790, whenever wp_die() is used in an API Request (which ideally it should not, but it can still happen) we should return a valid JSON response with the error.

I've been working on this, and started with an initial patch which just output the JSON and called exit, however this circumvents the rest of the API's routing etc _post_ die as script execution must stop at that point.

I ended up using PHP exceptions, which I know it not that common within WordPress, however this is the only way we can stop PHP execution of the scope that call wp_die() and still have the REST API handle the response as usual, as if the endpoint returned a WP_Error, essentially.

Attached a patch that implements this, though right now is a hack to checking if wp_die() caused the exception, this should use a WP_Die_Exception class if we actually decide to do this.

Attachments (1)

34999.diff (2.7 KB) - added by joehoyle 3 years ago.

Download all attachments as: .zip

Change History (8)

@joehoyle
3 years ago

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


3 years ago

#2 @joehoyle
3 years ago

From slack:

rmccue [8:36 AM] I think we might break things by using exceptions

try { something(); throw Exception( 'remote request 404ed' ); } catch ( Exception $e ) { /* ignore */ } If something() calls wp_die

#3 follow-up: @danielbachhuber
3 years ago

At this point, I'd much prefer we avoid wp_die() invocations REST requests. I don't think we'll be able to create a robust wp_die() handler that does what we need it to without compatibility concerns.

#4 @swissspidy
3 years ago

  • Keywords has-patch needs-unit-tests added

#5 in reply to: ↑ 3 @rmccue
2 years ago

  • Keywords close added

Replying to danielbachhuber:

At this point, I'd much prefer we avoid wp_die() invocations REST requests. I don't think we'll be able to create a robust wp_die() handler that does what we need it to without compatibility concerns.

Agreed and I think adding handling here will encourage people to use it. The current behaviour (breaking the response) is probably a good thing.

Recommend close.

#6 @rmccue
2 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Closing as wontfix; please don't use wp_die in your code.

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


2 years ago

Note: See TracTickets for help on using tickets.