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

Add support for raw body #11

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Conversation

lainosantos
Copy link
Contributor

Add support for raw_body config implemented at roadrunner-server/http#45

I did not identify any BC making parse of the raw body when it is not empty. Do you see any?

@rustatian rustatian requested a review from butschster November 5, 2022 20:57
@butschster butschster requested a review from roxblnfk November 6, 2022 09:44
@butschster butschster self-assigned this Nov 6, 2022
@butschster butschster added the enhancement New feature or request label Nov 6, 2022
Copy link
Member

@roxblnfk roxblnfk left a comment

Choose a reason for hiding this comment

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

I'm not sure that is a good idea

@lainosantos
Copy link
Contributor Author

Hi, @roxblnfk.

The inclusion of this configuration was discussed a lot on the RoadRunner side (roadrunner-server/roadrunner#1264) and I believe that implementing it in this package will bring the same possibility of use: For some cases it is necessary to get the body without any modification. Enabling raw_body setting will only send the body untouched when the content-type is application/x-www-form-urlencoded. I modified this PR so that it parses raw body only when the content-type is application/x-www-form-urlencoded. Without this PR, when the raw_body setting is enabled, the POST request fields cannot be accessed through the ServerRequestInterface::getParsedBody method, as is determined by PSR-7. One use case is when you need to get the body untouched to calculate the body's validation hash and still need to access the body's fields.

@lainosantos
Copy link
Contributor Author

I changed this PR e rebased the main branch to minimize side effects, please review it again.

@roxblnfk
Copy link
Member

roxblnfk commented Nov 7, 2022

I think it should be responsibility of a PHP middleware.
If you use row_body flag (rare case) you can use a custom middleware to handle it.

@lainosantos
Copy link
Contributor Author

Yes, it may be a PHP middleware, but I think that is great if this package support it as it's a roadrunner's feature.

@lainosantos
Copy link
Contributor Author

Another detail is that in PHP Middleware the request class will already be a PSR-7, there will not be the \Spiral\RoadRunner\Http\Request::$parsed property, which is necessary to decide whether to convert the body.

@butschster butschster requested a review from roxblnfk November 13, 2022 08:39
@butschster
Copy link
Contributor

Another detail is that in PHP Middleware the request class will already be a PSR-7, there will not be the \Spiral\RoadRunner\Http\Request::$parsed property, which is necessary to decide whether to convert the body.

Hm. Probably it can be solved with passing parsed property to the request attributes

@lainosantos
Copy link
Contributor Author

@butschster I made the change in PR to only add a resquest attribute

@@ -157,6 +157,8 @@ private function hydrateRequest(Request $request, array $context): void
$request->cookies = (array)($context['cookies'] ?? []);
$request->uploads = (array)($context['uploads'] ?? []);
$request->parsed = (bool)$context['parsed'];

$request->attributes['_parsed_body'] = $request->parsed;
Copy link
Member

Choose a reason for hiding this comment

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

Attributed value looks better 👍

  1. The literal should be replaced with a public constant
  2. I think the key should contain some prefix like rr_ or roadrunner. @butschster WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@roxblnfk, PR adjusted. 😉

@butschster butschster merged commit 2b397a2 into roadrunner-php:master Nov 30, 2022
@butschster
Copy link
Contributor

Thank you @lainosantos for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants