GitHub Issue #177: make reading from php:https://input configurable #181
Conversation
src/DataBuilder.php
Outdated
|
|
||
| protected function setIncludeRawRequestBody($config) | ||
| { | ||
| $fromConfig = $this->tryGet($config, 'include_raW_request_body'); |
There was a problem hiding this comment.
s/include_raW_request_body/include_raw_request_body/
| If you still want to read the request body for your PUT requests Rollbar SDK saves | ||
| the content of php:https://input in $_SERVER['php:https://input'] | ||
|
|
||
| Default: `false` |
There was a problem hiding this comment.
Is this something that most people assume should be there though? I feel like the default should be what we have been doing so far and allow people using < 5.6 to deal with the problem. But I am not sure, it is a shitty situation that I don't see a better solution for.
There was a problem hiding this comment.
This alters the default behavior of PHP in what I would call an unexpected place. Because of this I would opt for keeping this disabled by default.
src/DataBuilder.php
Outdated
| $this->requestBody = file_get_contents("php:https://input"); | ||
| $_SERVER['php:https://input'] = $this->requestBody; |
There was a problem hiding this comment.
Saving request body into $_SERVER make sense to keep only for old PHP versions due to performance reason. And even withing old PHP versions it's a huge hack, IMO, to build the logic of own application based on some workaround behavior of vendor logging library. I would recommend to just remove this line.
There was a problem hiding this comment.
@rokob For the same reasons I opt in for include_raw_request_body to be disabled by default.
There was a problem hiding this comment.
What if we did
if (version_compare(PHP_VERSION, '5.6.0') < 0) {
$_SERVER['php:https://input'] = $this->requestBody;
}At least then we only do this hack when we need it.
coryvirok
added
the
Type: Bug
| @see http:https://php.net/manual/pl/wrappers.php.php#refsect2-wrappers.php-unknown-unknown-descriptiop | ||
| If you still want to read the request body for your PUT requests Rollbar SDK saves | ||
| the content of php:https://input in $_SERVER['php:https://input'] | ||
|
|
There was a problem hiding this comment.
Let's mention the default being false for this
src/DataBuilder.php
Outdated
| $this->requestBody = file_get_contents("php:https://input"); | ||
| $_SERVER['php:https://input'] = $this->requestBody; |
There was a problem hiding this comment.
Let's do something like to only do this when absolutely necessary
if (version_compare(PHP_VERSION, '5.6.0') < 0) {
$_SERVER['php:https://input'] = $this->requestBody;
}
src/Defaults.php
Outdated
| @@ -99,6 +99,11 @@ public function localVarsDump($localVarsDump = null) | |||
| return $localVarsDump !== null ? $localVarsDump : $this->defaultLocalVarsDump; | |||
| } | |||
|
|
|||
| public function rawRequestBody($rawRequestBody = null) | |||
| { | |||
| return $rawRequestBody ? $rawRequestBody : $this->defaultRawRequestBody; | |||
There was a problem hiding this comment.
We should be testing for $rawRequestBody !== null
tests/DataBuilderTest.php
Outdated
| )); | ||
| $output = $dataBuilder->makeData(Level::fromName('error'), "testing", array()); | ||
| $requestBody = $output->getRequest()->getBody(); | ||
|
|
||
| $this->assertEquals($streamInput, $requestBody); | ||
| $this->assertEquals($streamInput, $_SERVER['php:https://input']); |
There was a problem hiding this comment.
Only do this check on less than 5.6
|
Other than the comments this looks good |
|
This LGTM but there is still an issue with the tests taking too long, I'm going to merge this and deal with the test timeouts elsewhere. |
In response to feedback from #177 I decided to make reading the raw request body from php:https://input disabled by default and configurable through
include_raw_request_body.Unfortunately, I don't see any way to overcome PHP < 5.6 limitation of single read from php:https://input so I combined making it configurable and also saving the content of the stream for later use in
$_SERVER['php:https://input']. This is not perfect, but I don't think we can do anything more for < 5.6 users due to PHP limitations.