Drop support non pre-parsed PSR-7 request body #218
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This revert webonyx#202 (commit 9d37f4c) because trying to parse PSR-7 request was a mistake. The whole point of PSR-7 is to allow for interoperability and be able to use specialized libs for body parsing (amongst many other things). Trying to parse ourselves would be opening a can of worm if/when other content types have to be supported. It is more correct and future safe to require that the body is parsed before being passed to GraphQL.
Member
|
It introduces a possible breaking change, so we'll have to release it in the next major version. Will merge it as soon as I start working on it (unless you need it for your other PRs). |
Contributor
Author
|
I will most likely need it for the next PR, but let's wait and re-consider it when I have something more to share... |
Member
|
So when we merge uploads this PR will make no sense, right? |
Contributor
Author
|
The upload PR does include this PR's commit too, so yes if upload PR is merged, then this one becomes useless. But it could also be merged separately... depending on how long it takes for upload PR to be merged... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've realized that PR #202 was probably a mistake and should be reversed. The fact that
$request->getParsedBody()might be empty was due to an incorrect use of Zend Expressive. And I now think that it is not, and should not be, the responsibility ofgraphql-phpto parse a PSR-7 request.I've since updated my code to properly configure Zend Expressive to parse the request with BodyParamsMiddleware before reaching
graphql-php.If we don't merge this PR, then suddenly
graphql-phpwill have to re-implement all kind of request parsing method (i'm thinkingmultipart/form-data) just in case the request was not parsed beforehand. And that would probably not be something we would like to have in that project. Instead it should live in a different project specialized in request parsing.