Support non pre-parsed PSR-7 request body#202
Merged
vladar merged 1 commit intowebonyx:masterfrom Nov 28, 2017
Merged
Conversation
Because PSR-7 specification only specify that `getParsedBody()` **may** return the parsed body for `application/json`, we cannot assume that it is always the case. So if the value returned parsed body is an empty array, it means we should try to parse it ourselves (`null` would mean no body at all according to spec). With this modification we try to used given parsed body, but fallback on trying to parse the body if necessary. This leave the door open to custom implementation of parsing if needed, while making it easier to use out of the box.
vladar
reviewed
Nov 27, 2017
|
|
||
| if (empty($bodyParams)) { | ||
| $bodyParams = json_decode($request->getBody(), true); | ||
| } |
Member
There was a problem hiding this comment.
Empty is a bit too loose. I guess we should check for null only. Thoughts?
Contributor
Author
There was a problem hiding this comment.
This is intended, as mentioned in commit message a null value would mean no body at all:
A null value indicates the absence of body content.
http://www.php-fig.org/psr/psr-7/#321-psrhttpmessageserverrequestinterface
So we test for an empty array, to try to parse the body ourselves. But if it was null it should throw an exception. IIRC the null case is correctly covered by pre-existing tests.
vladar
added a commit
that referenced
this pull request
Nov 28, 2017
PowerKiKi
added a commit
to PowerKiKi/graphql-php
that referenced
this pull request
Dec 21, 2017
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.
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
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.
Because PSR-7 specification only specify that
getParsedBody()mayreturn the parsed body for
application/json, we cannot assume that itis always the case. So if the value returned parsed body is an empty array,
it means we should try to parse it ourselves (
nullwould mean no body atall according to spec).
With this modification we try to used given parsed body, but fallback on
trying to parse the body if necessary. This leave the door open to custom
implementation of parsing if needed, while making it easier to use out of
the box.
This is in direct response to #120 (comment)