WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 3 months ago

#47188 assigned defect (bug)

Ensure that valid json is return for rest api, even when correct headers not sent.

Reported by: spacedmonkey Owned by: spacedmonkey
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: servehappy needs-patch
Focuses: rest-api Cc:

Description

Current when wp_die is called in php fatal error handler, valid json is only returned when json headers are returned as the function wp_is_json_request is used. See #45933.

However, core could do more to return json when it knows the current request is a REST API call.

Attachments (2)

47188.diff (1.2 KB) - added by spacedmonkey 2 years ago.
47188.2.diff (1.3 KB) - added by spacedmonkey 13 months ago.

Download all attachments as: .zip

Change History (34)

#1 @spacedmonkey
2 years ago

  • Keywords needs-patch added; has-patch removed

Related: #34999

@spacedmonkey
2 years ago

#2 @spacedmonkey
2 years ago

  • Keywords has-patch added; needs-patch removed

First patch 47188.diff to kick discussion.

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


23 months ago

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


22 months ago

#5 @spacedmonkey
22 months ago

  • Component changed from REST API to Site Health

#6 @spacedmonkey
17 months ago

  • Component changed from Site Health to REST API

Changing to REST API focus, as this makes more sense.

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


17 months ago

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


13 months ago

#9 @spacedmonkey
13 months ago

Updated patch based on @TimothyBlynJacobs 's feedback.

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


13 months ago

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


13 months ago

This ticket was mentioned in PR #215 on WordPress/wordpress-develop by spacedmonkey.


13 months ago

<!--
Hi there! Thanks for contributing to WordPress!

Pull Requests in this GitHub repository must be linked to a ticket in the WordPress Core Trac instance (https://core.trac.wordpress.org), and are only used for code review. No pull requests will be merged on GitHub.

See the WordPress Handbook page on using PRs for Code Review more information: https://make.wordpress.org/core/handbook/contribute/git/github-pull-requests-for-code-review/

If this is your first time contributing, you may also find reviewing these guides first to be helpful:

-->

Trac ticket: https://core.trac.wordpress.org/ticket/47188

#13 @prbot
13 months ago

spacedmonkey commented on PR #215:

@TimothyBJacobs For your review

#14 @spacedmonkey
13 months ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to TimothyBlynJacobs
  • Status changed from new to assigned

Assigning ticket to @TimothyBlynJacobs for review.

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


12 months ago

#16 @TimothyBlynJacobs
12 months ago

  • Milestone changed from Future Release to 5.5

#17 @TimothyBlynJacobs
12 months ago

  • Keywords needs-patch added; has-patch removed
  • Owner changed from TimothyBlynJacobs to spacedmonkey

This doesn't appear to work in my testing. Given the following filter and making a request to http://localhost:8889/wp-json/wp/v2/posts I get the HTML WSOD page.

<?php
add_filter( 'rest_prepare_post', function ( WP_REST_Response $response ) {
        throw new \Exception( 'test' );
} );

rest_pre_serve_request is fired immediately before rendering the output of the response, which is too late for any errors that happen during the meaningful parts of the request.

It may be better to add a defined( 'REST_REQUEST' ) && REST_REQUEST check in wp_die when choosing the wp_die handler.

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


11 months ago

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


10 months ago

#20 @TimothyBlynJacobs
10 months ago

  • Milestone changed from 5.5 to 5.6

As discussed in #core-restapi, punting to 5.6.

#21 follow-up: @TimothyBlynJacobs
6 months ago

@lpawlik Do you want to pick this up?

#22 in reply to: ↑ 21 @lpawlik
6 months ago

Replying to TimothyBlynJacobs:

@lpawlik Do you want to pick this up?

@TimothyBlynJacobs - would love to 👍

#23 @prbot
6 months ago

lukaspawlik commented on PR #215:

@TimothyBJacobs @spacedmonkey How about this approach? Looks like changing a little wp_is_json_request and adding try/catch around dispatch should catch most of the cases. It properly handles die handler and try/catch catches all thrown exceptions inside dispatch. I am curious about your thoughts.

#24 @prbot
6 months ago

TimothyBJacobs commented on PR #215:

@lukaspawlik Wrapping in a try/catch is an interesting idea, but I think we should explore it in a separate ticket and just use the Fatal Error Handler in this patch.

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


6 months ago

#26 @spacedmonkey
6 months ago

  • Milestone changed from 5.6 to Future Release

Punting, as ran out of time to look at this ticket. Hopefully we can look at it in 5.7.

#27 @prbot
5 months ago

lukaspawlik commented on PR #215:

@TimothyBJacobs this is ready for your review - it's using fatal error handler now however, if we reject changes I did to src/wp-includes/class-wp-fatal-error-handler.php and src/wp-includes/functions.php the change I did inside wp_is_json_request() seems to be enough to properly handle fatal errors during REST API request. I consider these additional changes as some enhancement to REST API errors handling (however I am not sure if that's kind of change is desired at all).

#28 @prbot
5 months ago

TimothyBJacobs commented on PR #215:

Thanks for refreshing the patch @lukaspawlik! I don't think we should be making these far reaching changes. Instead, we should really only be making a change to wp_die()'s handler choosing or wp_is_json_request().

My current thinking is that we should localize the change to wp_die() since while wp_is_json_request() is similar to using the REST API, it isn't synonymous with it. See https://core.trac.wordpress.org/ticket/42061.

#29 @prbot
5 months ago

lukaspawlik commented on PR #215:

@TimothyBJacobs I've removed previous changes and left only these that make REST API error handling consistent for REST request that contain Content-Type header with these that don't contain it (it's the core of the problem here right?).

#30 @prbot
5 months ago

TimothyBJacobs commented on PR #215:

Yep! Thanks for the refresh!

#31 @prbot
5 months ago

lukaspawlik commented on PR #215:

@TimothyBJacobs So I've figured out why I did previous condition modification that you asked me about. So when wp_debug_mode() is called for the first time (https://github.com/WordPress/wordpress-develop/blob/master/src/wp-settings.php#L80) then it's too late for checks like here (https://github.com/WordPress/wordpress-develop/blob/master/src/wp-includes/load.php#L453) as REST_REQUEST isn't defined yet. Function wp_is_json_request() is returning true at this time only when proper Content-Type header is set. So if we want to stop displaying errors for REST API requests, without proper headers set, we need to either modify the logic inside wp_debug_mode() or maybe call it again, like in this commit.

What are your thoughts about this? cc @spacedmonkey

#32 @prbot
3 months ago

TimothyBJacobs commented on PR #215:

So the error silencing that happens in wp_debug_mode shouldn't impact this ticket. That wp_debug_mode is too early to use REST_REQUEST is a known issue that we can tackle in that ticket.

Only the changes to wp_is_json_request should be necessary. As long as a fatal error occurs, and the fatal error handler is enabled, it will convert that to a wp_die call regardless of the wp_debug_mode settings.

Note: See TracTickets for help on using tickets.