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

getParsedBody() not working in slim v4 #99

Closed
esetnik opened this issue Aug 7, 2019 · 14 comments · Fixed by #100
Closed

getParsedBody() not working in slim v4 #99

esetnik opened this issue Aug 7, 2019 · 14 comments · Fixed by #100

Comments

@esetnik
Copy link
Contributor

esetnik commented Aug 7, 2019

In https://github.com/slimphp/Slim-Http/blob/master/src/ServerRequest.php#L165

        $parsedBody = $this->serverRequest->getParsedBody();
        if ($parsedBody !== null) {
            return $parsedBody;
        }

The $parsedBody is checked against null. When a POST request is received with Content-Type: application/json the php server will set $_POST to an empty array.

In
https://github.com/Nyholm/psr7-server/blob/master/src/ServerRequestCreator.php#L54
https://github.com/Nyholm/psr7-server/blob/master/src/ServerRequestCreator.php#L75

The serverRequest $parsedBody is initialized to the empty array and fails the check above so no parsed body data is returned from $request->getParsedBody();.

@shadowhand
Copy link

This probably requires adding a middleware to parse application/json bodies and replace the parsed body.

@esetnik
Copy link
Contributor Author

esetnik commented Aug 7, 2019

There's already extensive handling for json bodies in slim-http. See https://github.com/slimphp/Slim-Http/blob/master/tests/ServerRequestTest.php#L722. The issue is that it's not compatible with Nyholm\Psr7Server.

@shadowhand
Copy link

Ah yes, I see. The decorator is missing the handling for parsing JSON bodies.

@esetnik
Copy link
Contributor Author

esetnik commented Aug 7, 2019

Well not exactly. The issue is that Nyholm\Psr7Server does not return null for getParsedBody() when the Content-Type: application/json because it sets the parsedBody to $post here

In php, when Content-Type is application/json $_POST is [] not null.

@l0gicgate
Copy link
Member

l0gicgate commented Aug 7, 2019

@esetnik this is a tough problem. We could perhaps make a method which forces the body parsing no matter what the existing parsed body is?

My concern with removing the null check is do we want to interfere with the underlying implementations’s parsed body and completely override it?

@esetnik
Copy link
Contributor Author

esetnik commented Aug 7, 2019

Right. And I made a change which fixed the issue for me.

        $parsedBody = $this->serverRequest->getParsedBody();

        if (!empty($parsedBody)) {
            return $parsedBody;
        }

But it's weird because this test case is already passing and fails with my change, whereas my sample project doesn't work. So I need to understand why this test case is behaving differently from my project.

@l0gicgate
Copy link
Member

The problem with using empty() is that an empty array is still a valid parsed body in my opinion, that’s why this is a hard problem. What do you truly consider to be empty? For me, a null check is the only way to assert that.

@esetnik
Copy link
Contributor Author

esetnik commented Aug 7, 2019

The problem with using empty() is that an empty array is still a valid parsed body in my opinion, that’s why this is a hard problem. What do you truly consider to be empty? For me, a null check is the only way to assert that.

I agree, a null check is the appropriate way to ensure the body has not already been parsed by the implementation.

It looks like the real issue might be the requirement of slim v4 to use Nyholm/psr7-server which passes an empty array for parsedBody in all cases. The reason the test cases still pass is that they don't actually test the Nyholm/psr7-server implementation of ServerRequestCreator::fromGlobals().

Unfortunately it looks like it's also broken in zend/dictatoros https://github.com/zendframework/zend-diactoros/blob/master/src/ServerRequestFactory.php#L79

@esetnik esetnik changed the title getParsedBody() not working in slim v4 with Nyholm\Psr7Server getParsedBody() not working in slim v4 Aug 7, 2019
@l0gicgate
Copy link
Member

@esetnik using Nyholm/psr7-server isn’t a requirement for Slim 4, the framework is PSR-7 agnostic, theoretically you can use whichever implementation you want. You just have to find the one that fits your needs.

What we can do however is provide a workaround in Slim-Http so you can force body parsing using the logic in this repo if needed since we will never be able to accommodate all the different implementations’ quirks.

@esetnik
Copy link
Contributor Author

esetnik commented Aug 7, 2019

Yeah the thing is that there's currently no option that I found which works with slim v4 and slim-http. I've tried each of the compatible factories and they all pass empty array not null. So there's currently no way to use getParsedBody() from slim-http with slim v4 if you're using json content-type as far as I can tell. And I didn't find any integration testing between slim-http and slim which would have caught this issue. We might want to add that, since I think the use-case for using slim-http without slim will be rare, and compatibility should always be ensured.

@esetnik
Copy link
Contributor Author

esetnik commented Aug 7, 2019

I think part of the confusion is that the psr7 standard says that getParsedBody() should return null only in the absence of body content. So the other psr7 libraries are behaving correctly by returning an empty array since in the case of json the body content is not absent. Technically I think that slim-http should be conforming to this specification and checking for an empty array before it decides to do it's own parsing. If the underlying implementation returns null it means no body content at all, so slim-http should return null. If the underlying implementation returns a non-empty object or array, sim-http should return the underlying implementations result. And if the underlying implementation returns an empty array, slim-http should try to parse the body.

    /**
     * Retrieve any parameters provided in the request body.
     *
     * 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.
     *
     * @return null|array|object The deserialized body parameters, if any.
     *     These will typically be an array or object.
     */
    public function getParsedBody();

@l0gicgate
Copy link
Member

I’m fine with using empty() instead of the null check. Would you like to raise a PR?

@esetnik
Copy link
Contributor Author

esetnik commented Aug 7, 2019

@l0gicgate I opened #100 for you to review. Thanks for the feedback.

@BusterNeece
Copy link

The same issue affects GuzzleHttp's PSR-7 implementation. It appears numerous "correct" implementations of PSR-7 demonstrate this same tendency, and the PR in question is a quick and easy solution.

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

Successfully merging a pull request may close this issue.

4 participants