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

[spacedmonkey]

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.

[spacedmonkey]

Related: #34999

[spacedmonkey]

First patch 47188.diff to kick discussion.

[spacedmonkey]

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

[spacedmonkey]

Updated patch based on @TimothyBlynJacobs 's feedback.

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

• FAQs for New Contributors: ​https://make.wordpress.org/core/handbook/tutorials/faq-for-new-contributors/
• Contributing with Code Guide: ​https://make.wordpress.org/core/handbook/contribute/
• WordPress Coding Standards: ​https://make.wordpress.org/core/handbook/best-practices/coding-standards/
• Inline Documentation Standards: ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/
• Browser Support Policies: ​https://make.wordpress.org/core/handbook/best-practices/browser-support/
• Proper spelling and grammar related best practices: ​https://make.wordpress.org/core/handbook/best-practices/spelling/

-->

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

@TimothyBJacobs For your review

[spacedmonkey]

Assigning ticket to @TimothyBlynJacobs for review.

[TimothyBlynJacobs]

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.

[TimothyBlynJacobs]

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

[TimothyBlynJacobs]

@lpawlik Do you want to pick this up?

[lpawlik]

Replying to TimothyBlynJacobs:

@lpawlik Do you want to pick this up?

@TimothyBlynJacobs - would love to 👍

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

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

[spacedmonkey]

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

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

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.

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

Yep! Thanks for the refresh!

@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

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.