-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@see 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.