GitHub Issue #177: make reading from php://input configurable#181
GitHub Issue #177: make reading from php://input configurable#181rokob merged 12 commits intorollbar:masterfrom
Conversation
|
|
||
| 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://input in $_SERVER['php://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.
| if (!$this->requestBody) { | ||
| if (!$this->requestBody && $this->includeRawRequestBody) { | ||
| $this->requestBody = file_get_contents("php://input"); | ||
| $_SERVER['php://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://input'] = $this->requestBody;
}At least then we only do this hack when we need it.
coryvirok
added
the
Type: Bug
| @see http://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://input in $_SERVER['php://input'] | ||
|
|
There was a problem hiding this comment.
Let's mention the default being false for this
| if (!$this->requestBody) { | ||
| if (!$this->requestBody && $this->rawRequestBody) { | ||
| $this->requestBody = file_get_contents("php://input"); | ||
| $_SERVER['php://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://input'] = $this->requestBody;
}
|
|
||
| public function rawRequestBody($rawRequestBody = null) | ||
| { | ||
| return $rawRequestBody ? $rawRequestBody : $this->defaultRawRequestBody; |
There was a problem hiding this comment.
We should be testing for $rawRequestBody !== null
| $requestBody = $output->getRequest()->getBody(); | ||
|
|
||
| $this->assertEquals($streamInput, $requestBody); | ||
| $this->assertEquals($streamInput, $_SERVER['php://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. |
4 participants
In response to feedback from #177 I decided to make reading the raw request body from php://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://input so I combined making it configurable and also saving the content of the stream for later use in
$_SERVER['php://input']. This is not perfect, but I don't think we can do anything more for < 5.6 users due to PHP limitations.