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 body incorrectly parsed #386

Closed
bpolaszek opened this issue Aug 24, 2020 · 2 comments · Fixed by #395
Closed

Request body incorrectly parsed #386

bpolaszek opened this issue Aug 24, 2020 · 2 comments · Fixed by #395
Milestone

Comments

@bpolaszek
Copy link

bpolaszek commented Aug 24, 2020

Hello there,

I have implemented a Mercure Hub with ReactPHP and I'm having troubles with request bodies.

I'm currently trying to achieve this:

POST /.well-known/mercure
Content-Type: application/x-www-form-urlencoded

topic=/foo&topic=/bar&data=foobar

Indeed, the Mercure protocol specification states that a topic /foo can have alternate IRIs such as /bar. The payload to be sent is not in the PHP format topic[]=foo&topic[]=bar but topic=/foo&topic=/bar which means that PHP, by default, considers that topic is a string with the only value /bar, which is not what I want.
And so is the React\Http\Middleware\RequestBodyParserMiddleware which internally relies on PHP's parse_str function.

So, I'd like to implement my own logic to parse the body payload by myself, but:

  • It looks like ReactPHP removes any trace of the original body at some point ($request->getBody() returns an empty string), when Content-Type is application/x-www-form-urlencoded (could not reproduce with a random content type)
  • It looks like the only way to short-circuit the RequestBodyParserMiddleware to introduce my own is to set an empty value for enable_post_data_reading in php.ini, but I cannot assume people who will use my project have this tricky tweak in their configuration.

Could we either:

  • Not remove the original body of the request
  • Introduce an interface for RequestBodyParserMiddleware so that we could optionally replace it in Server's constructor
  • Introduce an array $config of options into Server's constructor so that we could pass an option to prevent parsing request bodies and let our own middlewares take care of this?

Thank you,
Ben

@clue
Copy link
Member

clue commented Sep 16, 2020

@bpolaszek Thanks for bringing this up!

I agree that clearing the request body for some requests is somewhat inconsistent, so I've just filed #395 to make sure you can always read the raw request body and apply your own custom parsing rules.

I consider this a new feature rather than a bug fix. IMO the request body is not "incorrectly parsed" because it follows PHP's default handling and I don't see anything that contradicts this.

With the above change in place, you can use your own parser to override this behavior. If you feel this is something that more people could benefit from, perhaps share this as a middleware implementation or consider filing a PR in this repo and I'm happy to take another look!

BTW, love your Mercure protocol implementation, keep it up! 💪

I believe this has been answered, so I'm closing this for now. Please come back with more details if this problem persists and we can always reopen this 👍

@clue clue closed this as completed Sep 16, 2020
@clue clue added this to the v1.2.0 milestone Sep 16, 2020
@clue clue reopened this Sep 16, 2020
@bpolaszek
Copy link
Author

bpolaszek commented Sep 16, 2020

Hello @clue,

Thanks a lot for having worked on this!

Regarding the Mercure hub, it highly depends on your work on clue/redis-react, clue/block-react and clue/reactphp-eventsource (among almost everything else in the reactphp ecosystem), so let's seize the opportunity for thanks and congrats! 🙏 🙏 🙏

I'll keep an eye on the PR merge to test this ASAP.

Thanks again,
Ben

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants