Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#47188 assigned defect (bug)

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

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile 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 5 years ago.
47188.2.diff (1.3 KB) - added by spacedmonkey 4 years ago.

Download all attachments as: .zip

Change History (34)

#1 @spacedmonkey
5 years ago

  • Keywords needs-patch added; has-patch removed

Related: #34999

@spacedmonkey
5 years ago

#2 @spacedmonkey
5 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.


5 years ago

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


5 years ago

#5 @spacedmonkey
5 years ago

  • Component changed from REST API to Site Health

#6 @spacedmonkey
5 years 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.


5 years ago

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


4 years ago

@spacedmonkey
4 years ago

#9 @spacedmonkey
4 years ago

Updated patch based on @TimothyBlynJacobs 's feedback.

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


4 years ago

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


4 years ago

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


4 years ago
#12

<!--
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

spacedmonkey commented on PR #215:


4 years ago
#13

@TimothyBJacobs For your review

#14 @spacedmonkey
4 years 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.


4 years ago

#16 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Future Release to 5.5

#17 @TimothyBlynJacobs
4 years 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.


4 years ago

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


4 years ago

#20 @TimothyBlynJacobs
4 years ago

  • Milestone changed from 5.5 to 5.6

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

#21 follow-up: @TimothyBlynJacobs
4 years ago

@lpawlik Do you want to pick this up?

#22 in reply to: ↑ 21 @lpawlik
4 years ago

Replying to TimothyBlynJacobs:

@lpawlik Do you want to pick this up?

@TimothyBlynJacobs - would love to 👍

lukaspawlik commented on PR #215:


4 years ago
#23

@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.

TimothyBJacobs commented on PR #215:


4 years ago
#24

@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.


4 years ago

#26 @spacedmonkey
4 years 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.

lukaspawlik commented on PR #215:


4 years ago
#27

@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).

TimothyBJacobs commented on PR #215:


4 years ago
#28

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.

lukaspawlik commented on PR #215:


4 years ago
#29

@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?).

TimothyBJacobs commented on PR #215:


4 years ago
#30

Yep! Thanks for the refresh!

lukaspawlik commented on PR #215:


3 years ago
#31

@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

TimothyBJacobs commented on PR #215:


3 years ago
#32

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.