-
-
Notifications
You must be signed in to change notification settings - Fork 88
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
Request::getBody(), see #57 #58
base: master
Are you sure you want to change the base?
Conversation
@@ -293,7 +298,17 @@ public function getRemoteHost() | |||
*/ | |||
public function getRawBody() | |||
{ | |||
return $this->rawBodyCallback ? call_user_func($this->rawBodyCallback) : NULL; | |||
return $this->rawBodyCallback ? call_user_func($this->rawBodyCallback, $this) : NULL; |
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.
IMHO rawbody should return rawbody, not parsed 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.
? This commit does not change getRawBody()
result.
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.
You're using the same callback, that also parses the JSON for application/json, aren't you?
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.
Oh, you've added new parameter, sorry :)
Although it's a nice closure magic, I'm afraid it's gonna be slow, it's not even lazy... I mean, those unnecessary body calls could be easily avoided if type check was extracted outside (like |
@Majkl578 That's how I originally wanted to do it, but then I though that it is not powerful enough. What if you want to make decision based on something different than Content-Type (e.g. HTTP method)? What if you want to handle all something/* types? That being said I like the simple solution |
|
1dc599c
to
f35d20f
Compare
f73e5f9
to
48409af
Compare
I think that this is now ready to merge. |
48409af
to
48a3e5f
Compare
src/Http/Request.php
Outdated
* @throws InvalidRequestBodyException | ||
*/ | ||
public function getBody() | ||
{ |
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.
indentation
48a3e5f
to
6363c41
Compare
@JanTvrdik What about the JSON decode DOS vulnerability? I didn't see anything that would solve it in your code (but I might have missed it of course). |
@enumag I ignore it the same way everybody in the PHP worlds ignores it. Those few that are concern may replace the body parser for JSON with sth smarter. |
6363c41
to
1627f10
Compare
BTW: PSR7 calls this method |
if ($body === NULL) { | ||
$contentType = $request->getHeader('Content-Type'); | ||
foreach ($this->bodyParsers as $parserContentType => $parser) { | ||
if (stripos($contentType, $parserContentType) === 0) { |
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.
strcasecmp is better
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.
You mean strncasecmp?
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.
Yes. Its PHP shame that there is no startsWith function.
2a0c595
to
4ca7b03
Compare
da24b94
to
540335c
Compare
e7c7e2d
to
bf945f3
Compare
9a14e6e
to
a20fb8f
Compare
55488bd
to
2042d2e
Compare
4960652
to
5e67add
Compare
689f4ae
to
33aae19
Compare
No description provided.