Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#44534 closed defect (bug) (fixed)

wp_debug_mode() does not turn off display_errors for REST requests

Reported by: chrisl27's profile chrisl27 Owned by: earnjam's profile earnjam
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.5
Component: REST API Keywords: has-patch fixed-5.0
Focuses: rest-api Cc:

Description (last modified by SergeyBiryukov)

There is code in wp-includes/load.php:336 that attempts to ini_set( 'display_errors', 0 ) for REST requests, but it checks defined( 'REST_REQUEST' ) too early in the process.

REST_REQUEST is defined in rest_api_loaded() where it is run from the parse_request action, after wp_debug_mode() has run.

Original changeset was: [36530]

For Trac ticket: #34915

Attachments (3)

44534.diff (351 bytes) - added by duanestorey 5 years ago.
Patch
44534.2.diff (1.2 KB) - added by earnjam 5 years ago.
44534.3.diff (1.4 KB) - added by earnjam 5 years ago.
Adds check for HTTP Accept header

Download all attachments as: .zip

Change History (34)

#1 @SergeyBiryukov
6 years ago

  • Description modified (diff)
  • Version changed from 4.9.7 to 4.5

#2 @SergeyBiryukov
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Confirmed, wp_debug_mode() indeed checks REST_REQUEST earlier than it's defined.

#3 @duanestorey
5 years ago

Seems like the simplest thing to do is simply call wp_debug_mode() again after REST_REQUEST has been defined. Looking at the code in wp_debug_mode, it would detect the variable then and set display_errors to 0. I looked briefly at moving wp_debug_mode in the wp-settings.php, but there is a lot of code in between where it is now and where it would end up, and I'd be worried about unintended consequences.

@duanestorey
5 years ago

Patch

#4 @duanestorey
5 years ago

  • Keywords has-patch added; needs-patch removed

#5 @SergeyBiryukov
5 years ago

  • Milestone changed from Future Release to 4.9.9
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#6 @pento
5 years ago

  • Milestone changed from 4.9.9 to 5.0

#7 @danielbachhuber
5 years ago

This is likely the cause of some not-insignificant number of unexpected REST API failures with Gutenberg. See https://github.com/WordPress/gutenberg/issues/4936 for some related conversation.

#8 @danielbachhuber
5 years ago

REST_REQUEST is defined in rest_api_loaded() where it is run from the parse_request action, after wp_debug_mode() has run.

Worth noting that this is still quite late in the request, which means plugins and themes could still emit errors as their loading.

An alternative we could explore is inspecting the Content-Type header in wp_debug_mode() and assuming PHP notices should be disabled if Content-Type: application/json.

#9 @danielbachhuber
5 years ago

I've created https://github.com/WordPress/gutenberg/issues/10476 so we can test the Content-Type:application/json idea in Gutenberg.

#10 @duanestorey
5 years ago

Another possibly way to maybe approach this is to modify wp_debug_mode() directly and disable display errors when a request header such as

Accept: application/json

is present. It doesn't make any sense to output debug in that case.

The wp_debug_mode() function already does this for several different scenarios:

if ( defined( 'XMLRPC_REQUEST' )
defined( 'REST_REQUEST' ) ( defined( 'WP_INSTALLING' ) && WP_INSTALLING ) wp_doing_ajax() ) {

So perhaps we can add a new one, something like:

if ( defined( 'XMLRPC_REQUEST' )
defined( 'REST_REQUEST' ) ( defined( 'WP_INSTALLING' ) && WP_INSTALLING ) wp_doing_ajax() wp_is_json_request() ) {

where wp_is_json_request() would check $_SERVER[ 'HTTP_ACCEPT' ] in PHP.

Thoughts?

Last edited 5 years ago by duanestorey (previous) (diff)

#11 follow-up: @duanestorey
5 years ago

And yet another possibly way would be to literally check the request URI for either /wp-json/ or ?rest_route=/ and disable. That may be a bit clunky, but would probably capture all scenarios without modifying any requests themselves.

#12 in reply to: ↑ 11 ; follow-up: @danielbachhuber
5 years ago

Replying to duanestorey:

And yet another possibly way would be to literally check the request URI for either /wp-json/ or ?rest_route=/ and disable. That may be a bit clunky, but would probably capture all scenarios without modifying any requests themselves.

The /wp-json/ path is filterable, which would happen much later in the stack, so I think inspecting Content-Type is more reliable and precise.

#13 in reply to: ↑ 12 ; follow-up: @duanestorey
5 years ago

Replying to danielbachhuber:

The /wp-json/ path is filterable, which would happen much later in the stack, so I think inspecting Content-Type is more reliable and precise.

You're talking about the response header? Wouldn't that only get set once WP understands that it's a REST response, which is basically too late (ie after the debug init)? I would think as soon as a debug statement was echoed the web server would output all response headers, which then wouldn't work for detection. Or am I misunderstanding?

Last edited 5 years ago by duanestorey (previous) (diff)

#14 in reply to: ↑ 13 @danielbachhuber
5 years ago

Replying to duanestorey:

You're talking about the response header? Wouldn't that only get set once WP understands that it's a REST response, which is basically too late (ie after the debug init)? I would think as soon as a debug statement was echoed the web server would output all response headers, which then wouldn't work for detection. Or am I misunderstanding?

Sorry, Content-Type as the request header.

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


5 years ago

#16 @danielbachhuber
5 years ago

  • Owner changed from SergeyBiryukov to earnjam

Assigning to @earnjam per today's #core-restapi Slack chat.

@earnjam
5 years ago

#17 @earnjam
5 years ago

44534.2.diff adds a simple wp_is_json_request() function, which does a basic check for the Content-Type:application/json header. This function is then called inside wp_debug_mode().

Even with this, it could still be helpful to have wp_debug_mode() be called again later in the request (like in 44534.diff), in case the Content-Type header is not supplied.

I've also opened a PR to test this out within Gutenberg here: https://github.com/WordPress/gutenberg/pull/10606

#18 follow-up: @danielbachhuber
5 years ago

@earnjam Is there a reason you went with CONTENT_TYPE over HTTP_ACCEPT?

If I jQuery.getJSON('/wp-json/'); in my console, I get:

'HTTP_ACCEPT' => 'application/json, text/javascript, */*; q=0.01',
'CONTENT_TYPE' => '',

#19 in reply to: ↑ 18 @earnjam
5 years ago

@danielbachhuber I think I was thinking of the response format.

You're right though, it makes more sense to use Accept when interpreting the request, since that's what the client expects as the response and Content-Type would only really be used in the request if there was a JSON payload associated, which may not always be the case.

I'll test and refresh the patch.

@earnjam
5 years ago

Adds check for HTTP Accept header

#20 @duanestorey
5 years ago

@earnjam It looks good to me.

The only other question I have is I noticed there is a PHP class for doing Rest API requests. Is it worth making sure that's the Accept header is set there as well for GET requests? I don't know that part of the code enough, but it may make sense to ensure at least WP to WP won't have this issue going forward.

#21 @danielbachhuber
5 years ago

Some relevant conversation in Slack:

danielbachhuber [2:01 PM] @earnjam Do you think strpos( $_SERVER['HTTP_ACCEPT'], 'application/json' ) is reliable enough?

earnjam [2:02 PM] I think so. Problem is it could be anywhere in the header string. Could break it apart and check them one at a time, but wasn’t sure if that was necessary.

danielbachhuber [2:02 PM] Can you spend a quick 10 minutes seeing if there's any prior art on the web that we can follow?

earnjam [2:03 PM] I couldn’t think of a situation where application/json would be in the Accept header, but it wouldn’t actually want a JSON response. Unless some client just stuck it in there by accident. I looked a bit. Found this PEAR HTTP2 class that does MIME type negotiation. It breaks it apart and looks that way, but it also considers weighting, so needs to.
https://github.com/pear/HTTP2/blob/master/HTTP2.php#L208

@duanestorey To your question, that's not a factor in this case.

#22 @danielbachhuber
5 years ago

I tested this manually by:

  1. Adding error_log( $undefined_variable ); to a mu-plugin.
  2. Running jQuery.getJSON('/wp-json/'); in my browser's console.

Without 44534.3.diff, the response is:

<br />
<b>Notice</b>:  Undefined variable: undefined_variable in <b>/Users/danielbachhuber/projects/wordpress-develop/wp-content/mu-plugins/local.php</b> on line <b>3</b><br />
{"name":"wordpress-develop", [...]

With 44534.3.diff, the response is a valid JSON object.

#23 @danielbachhuber
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 43730:

Load: Disable PHP errors for JSON requests

Because WP REST API requests aren't identified until parse_request, it's impractical to reference the REST_REQUEST constant in wp_debug_mode(). Instead, it's more helpful to assume that a request wanting a JSON response probably doesn't want PHP errors breaking the response.

Props chrisl27, duanestorey, earnjam.
Fixes #44534.

#24 @danielbachhuber
5 years ago

  • Keywords fixed-5.0 added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging to trunk.

#25 follow-up: @JustinSainton
5 years ago

@danielbachhuber Would it be unreasonable to make the output of wp_is_json_request a filterable value? I know I've debugged countless issues by noticing errors in JSON output that is often only exposed there. Being able to conditionally add_filter( 'wp_is_json_request', '__return_true' ) in some cases would be really valuable. Happy to patch if we think this is worth the filter-maintenance tradeoff.

#26 @danielbachhuber
5 years ago

@JustinSainton Probably not at this point in time. 5.0 code is evaluated against: is this necessary for Gutenberg?

If you open a new ticket for it though, we can discuss.

#27 in reply to: ↑ 25 @duanestorey
5 years ago

I suspect if you want to just debug and ensure no output happens you can just define REST_REQUEST in your code. It would bypass the same check in the debug init. The actual define later would likely throw an error I think, but display errors would be off then anyways.

Replying to JustinSainton:

@danielbachhuber Would it be unreasonable to make the output of wp_is_json_request a filterable value? I know I've debugged countless issues by noticing errors in JSON output that is often only exposed there. Being able to conditionally add_filter( 'wp_is_json_request', '__return_true' ) in some cases would be really valuable. Happy to patch if we think this is worth the filter-maintenance tradeoff.

#28 @earnjam
5 years ago

@JustinSainton You could always check log output instead of relying on displaying of errors.

Regardless, the call to wp_debug_mode() occurs in wp-settings.php well before any plugins are loaded. So I think the only way you'd be able to filter it would be a preinitialized hook in wp-config.php.

This ticket was mentioned in Slack in #core-editor by danielbachhuber. View the logs.


5 years ago

#31 @jeremyfelt
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 43983:

Load: Disable PHP errors for JSON requests

Because WP REST API requests aren't identified until parse_request, it's impractical to reference the REST_REQUEST constant in wp_debug_mode(). Instead, it's more helpful to assume that a request wanting a JSON response probably doesn't want PHP errors breaking the response.

Merges [43730] to trunk.

Props chrisl27, duanestorey, earnjam.
Fixes #44534.

Note: See TracTickets for help on using tickets.