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.

Sign up for GitHub

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

parse body if empty and has media type detected #100

Merged
merged 1 commit into from
Aug 29, 2019
Merged

parse body if empty and has media type detected #100

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/ServerRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,13 @@ public function getParsedBody()
{
$parsedBody = $this->serverRequest->getParsedBody();

if ($parsedBody !== null) {
if (!empty($parsedBody)) {
return $parsedBody;
Copy link

Choose a reason for hiding this comment

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

Honestly, I don't think this is the correct solution. nyholm/psr7 should be updated to not automatically set the parsed body, per PSR-7 spec:

     * If the request Content-Type is either application/x-www-form-urlencoded
     * or multipart/form-data, and the request method is POST, this method MUST
     * return the contents of $_POST.
     *
     * Otherwise, this method may return any results of deserializing
     * the request body content; as parsing returns structured content, the
     * potential types MUST be arrays or objects only. A null value indicates
     * the absence of body content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand it, nyholm/psr7 is correct implementation. In the case of a json $_POST they should not send null they should send empty array. See the discussion in #99.

Copy link
Member

@l0gicgate l0gicgate Aug 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We could do a null check based on content type then. If it is application/x-www-form-urlencoded or multipart/form-data we do a null check and return the implementation’s parsed body if not null otherwise parse the body.

Copy link

Choose a reason for hiding this comment

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

@l0gicgate Slim doesn't need to solve this problem, IMO. It is a bug in nyholm/psr7, which is clearly not following the spec by blindly setting withParsedBody($post) without checking the request Content-Type.

Copy link
Member

Choose a reason for hiding this comment

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

It’s also a problem with Zend-Diactoros.

Copy link

Choose a reason for hiding this comment

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

Alright, if the problem exists beyond one implementation then I agree this is a reasonable solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that the spec wasn't really considering the possibility that multiple layers would attempt to parse the body. There's no way to pass information on whether body parsing was attempted already.

}

$mediaType = $this->getMediaType();
if ($mediaType === null) {
return null;
return $parsedBody;
}

// Check if this specific media type has a parser registered first
Expand Down