Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JanTvrdik
Copy link
Contributor

No description provided.

@@ -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;
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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 :)

@Majkl578
Copy link
Contributor

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 addBodyCallback('application/json', $cb)).

@JanTvrdik
Copy link
Contributor Author

@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 addBodyCallback('application/json', $cb) more than the current one.

@JanTvrdik
Copy link
Contributor Author

JanTvrdik commented Apr 23, 2015

Sidenote: it must somehow workaround the JSON decode DOS vulnerability.

@dg dg force-pushed the master branch 7 times, most recently from 1dc599c to f35d20f Compare November 5, 2015 00:41
@JanTvrdik JanTvrdik force-pushed the request_get_body branch 2 times, most recently from f73e5f9 to 48409af Compare June 5, 2016 07:15
@JanTvrdik JanTvrdik changed the title WIP: Request::getBody(), see #57 Request::getBody(), see #57 Jun 5, 2016
@JanTvrdik
Copy link
Contributor Author

I think that this is now ready to merge.

* @throws InvalidRequestBodyException
*/
public function getBody()
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

@enumag
Copy link
Contributor

enumag commented Jun 5, 2016

@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).

@JanTvrdik
Copy link
Contributor Author

JanTvrdik commented Jun 5, 2016

@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.

@JanTvrdik
Copy link
Contributor Author

BTW: PSR7 calls this method getParsedBody() instead of just getBody(). It is longer but a bit more descriptive.

if ($body === NULL) {
$contentType = $request->getHeader('Content-Type');
foreach ($this->bodyParsers as $parserContentType => $parser) {
if (stripos($contentType, $parserContentType) === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strcasecmp is better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean strncasecmp?

Copy link
Member

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.

@dg dg force-pushed the master branch 4 times, most recently from 2a0c595 to 4ca7b03 Compare January 26, 2023 22:47
@dg dg force-pushed the master branch 6 times, most recently from da24b94 to 540335c Compare March 20, 2023 13:43
@dg dg force-pushed the master branch 2 times, most recently from e7c7e2d to bf945f3 Compare August 5, 2023 19:08
@dg dg force-pushed the master branch 3 times, most recently from 9a14e6e to a20fb8f Compare November 14, 2023 18:31
@dg dg force-pushed the master branch 5 times, most recently from 55488bd to 2042d2e Compare December 11, 2023 13:01
@dg dg force-pushed the master branch 2 times, most recently from 4960652 to 5e67add Compare May 2, 2024 10:56
@dg dg force-pushed the master branch 5 times, most recently from 689f4ae to 33aae19 Compare November 5, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants