Opened 7 years ago
Closed 6 years ago
#44534 closed defect (bug) (fixed)
wp_debug_mode() does not turn off display_errors for REST requests
Reported by: |
|
Owned by: |
|
---|---|---|---|
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 )
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)
Change History (34)
#2
@
7 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#3
@
6 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.
#5
@
6 years ago
- Milestone changed from Future Release to 4.9.9
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#7
@
6 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
@
6 years ago
REST_REQUEST
is defined inrest_api_loaded()
where it is run from the parse_request action, afterwp_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
@
6 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
@
6 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?
#11
follow-up:
↓ 12
@
6 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:
↓ 13
@
6 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:
↓ 14
@
6 years ago
Replying to danielbachhuber:
The
/wp-json/
path is filterable, which would happen much later in the stack, so I think inspectingContent-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?
#14
in reply to:
↑ 13
@
6 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.
6 years ago
#16
@
6 years ago
- Owner changed from SergeyBiryukov to earnjam
Assigning to @earnjam per today's #core-restapi
Slack chat.
#17
@
6 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:
↓ 19
@
6 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
@
6 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.
#20
@
6 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
@
6 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 theAccept
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
@
6 years ago
I tested this manually by:
- Adding
error_log( $undefined_variable );
to a mu-plugin. - 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.
#24
@
6 years ago
- Keywords fixed-5.0 added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for merging to trunk.
#25
follow-up:
↓ 27
@
6 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
@
6 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
@
6 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 conditionallyadd_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
@
6 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
.
Confirmed,
wp_debug_mode()
indeed checksREST_REQUEST
earlier than it's defined.