Adding application/json input handling. #546
Conversation
| @@ -789,7 +800,7 @@ public function testGetHttpData() | |||
| 'SERVER_PORT' => '443', | |||
| 'SERVER_PROTOCOL' => 'HTTP/1.1', | |||
| 'REQUEST_METHOD' => 'PATCH', | |||
| 'QUERY_STRING' => 'q=bitch&l=en', | |||
There was a problem hiding this comment.
This seemed inappropriate, was removed while working on similar work.
| @@ -276,6 +276,17 @@ public function setEnvironment($value) | |||
| return $this; | |||
| } | |||
|
|
|||
| /** | |||
| * Note: Prior to PHP 5.6, a stream opened with php://input can | |||
There was a problem hiding this comment.
Since you're targeting the master branch, PHP < 5.6 is allowed, so you should read that only if PHP is >= 5.6; otherwise we would mess with the app.
There was a problem hiding this comment.
It won't really 'mess up the app'. What will happen is the data will be null. I can adjust the comment to explain that. This doesn't introduce a regression issue because the previous implementation and the current one still return the same broken results for php from the dark ages :)
I can add the condition in. I felt that this is really an edge case and there is no clean way to handle support for PHP < 5.6 with this feature.
There was a problem hiding this comment.
The one thing that is a little strange is the unit tests will succeed in < 5.6 but the reality is the feature doesn't work in that version. Not sure if there is a way to resolve that. Just a thought.
(I added the conditional https://github.com/getsentry/sentry-php/pull/546/files#diff-11a1d8314f22caf150ad02a00b294b58R287 )
There was a problem hiding this comment.
The data will be empty on < 5.6 but it can also be we fetched the data before the user could (if the use the library for logging for example) causing it to possibly break the user's application.
And yes this will not go well for < 5.6 but we should try and solve that differently if we must. But this could affect the implementors application and that is something we really should avoid at all cost.
So 👍 for the added conditional.
| @@ -748,6 +759,11 @@ protected function get_http_data() | |||
| // instead of a mapping which goes against the defined Sentry spec | |||
| if (!empty($_POST)) { | |||
| $result['data'] = $_POST; | |||
| } elseif (isset($_SERVER['CONTENT_TYPE']) && stripos($_SERVER['CONTENT_TYPE'], 'application/json') === 0) { | |||
| $raw_data = $this->getInputStream() ?: false; | |||
There was a problem hiding this comment.
Since we will never need the input stream for other stuff, maybe we can wrap the Json handling inside that method?
There was a problem hiding this comment.
You should probably add input stream handling for XML as well. This test is just for JSON. I really didn't want to get into XML parsing so I left that out. We actually have to deal with a legacy XML API for one project. Once I have to debug that I'll make a new patch :)
This could be abstracted into a parse() method as it gets bigger. I didn't want to spend too much time on this knowing that 2.0 is on the horizon.
|
@Jean85 I added the negative test case. This was good as it identified a need to abstract the json encode stuff for future XML tests. Will squish after your approval |
References #531