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

Add BodyParsingMiddleware #2798

Merged
merged 10 commits into from Aug 15, 2019

Conversation

@akrabat
Copy link
Member

commented Aug 13, 2019

This middleware will parse the body of a PSR-7 ServerRequest and if the content-type is known and a parser is registered, it will add the parsed data to the request object before passing to the next middleware.

Usage:

use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use Slim\Factory\AppFactory;
use Slim\Middleware\BodyParsingMiddleware;
use Slim\Psr7\Response;

$app = AppFactory::create();
$app->addBodyParsingMiddleware();

$app->post('/foo', function (ServerRequestInterface $request): ResponseInterface {
    $data = $request->getParsedBody();

    $response = new Response();
    $response->getBody()->write(
        print_r($data, true)
    );
    return new Response();
});

$app->run();

@akrabat akrabat requested review from adriansuter and l0gicgate Aug 13, 2019

@akrabat akrabat force-pushed the akrabat:body-parsing-middleware branch 2 times, most recently from 252e621 to 50520c6 Aug 13, 2019

@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Rebased to tip of 4.x branch

@akrabat akrabat force-pushed the akrabat:body-parsing-middleware branch from 50520c6 to a4d58bc Aug 13, 2019

@adriansuter
Copy link
Contributor

left a comment

Good work. Some minor change requests.

Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
tests/Middleware/BodyParsingMiddlewareTest.php Outdated Show resolved Hide resolved

@adriansuter adriansuter added the Slim 4 label Aug 13, 2019

@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@adriansuter Review items addressed.

@adriansuter
Copy link
Contributor

left a comment

Wonderful.

@adriansuter adriansuter added this to the 4.2.0 milestone Aug 13, 2019

@l0gicgate
Copy link
Contributor

left a comment

Minor changes. Mainly just doc bloc stuff. This is a great addition to our middleware suite.

Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
tests/Middleware/BodyParsingMiddlewareTest.php Outdated Show resolved Hide resolved
Slim/Middleware/BodyParsingMiddleware.php Outdated Show resolved Hide resolved
@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@l0gicgate Review items addressed.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

@akrabat what about adding App::addBodyParsers(array $parsers = []) like we have for error handling, etc.

@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@akrabat what about adding App::addBodyParsers(array $parsers = []) like we have for error handling, etc.

A key reason that addErrorMiddleware() exists is that there's a number of dependencies to the constructor that the app instance can fill in for you.

That's not the case for this one, so we'd be adding it for the dev experience of:

$app->addErrorMiddleware(true, true, true);
$app->addBodyParsing();

looking nicer than:

$app->addErrorMiddleware(true, true, true);
$app->add(new Slim\Middleware\BodyParsingMiddleware());

I'm easy either way and defer to @l0gicgate on it.

@shadowhand

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Before this is merged, I would like to give a more complete review of it. I think it can be improved and be made more friendly to IoC.

}
if (isset($this->bodyParsers[$mediaType])) {
$body = (string)$request->getBody();

This comment has been minimized.

Copy link
@anfly0

anfly0 Aug 13, 2019

What if the stream is not seekable (rewindable)?
This would be a problem if any subsequent middleware, for whatever reason, needed to access the raw body.
An example would be authentication via HMAC.

One solution would be to simply copy the non-seekable stream to a new seekable stream.

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 13, 2019

Author Member

What if the stream is not seekable (rewindable)?
This would be a problem if any subsequent middleware, for whatever reason, needed to access the raw body.
An example would be authentication via HMAC.

__toString() will rewind (if it can) and leaves the pointer back where it was before the call, so the stream won't look any different for subsequent middleware. (If it can't rewind, then nothing's changed of course.)

One solution would be to simply copy the non-seekable stream to a new seekable stream.

If the stream is not rewindable, then there's no guarantee that the data at the beginning is anywhere in memory now, so I don't see any advantage to copying to seekable stream?

At the end of the day, I think that if you're using a non-rewindable string in your ServerRequest, then you're incompatible with getParsedBody() and won't need or want this middleware.

This comment has been minimized.

Copy link
@anfly0

anfly0 Aug 13, 2019

__toString() will rewind (if it can) and leaves the pointer back where it was before the call, so the stream won't look any different for subsequent middleware. (If it can't rewind, then nothing's changed of course.)

According to PSR-7 it is not a requirement to attempt to rewind the stream after the data has been read by __to_string(). PSR-7 only requires the stream to be rewound before the data is read.

If the stream is not rewindable, then there's no guarantee that the data at the beginning is anywhere in memory now, so I don't see any advantage to copying to seekable stream?

At the end of the day, I think that if you're using a non-rewindable string in your ServerRequest, then you're incompatible with getParsedBody() and won't need or want this middleware.

Well most often you will see something like $streamFactory->createStreamFromFile('php://input') when creating the body stream.
The problem is that there is no guarantee php://input is seekable.
So the only real solution if the stream is not seekable and the raw data needs to access more than once is to copy the stream.

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 13, 2019

Author Member

Well most often you will see something like $streamFactory->createStreamFromFile('php://input') when creating the body stream.
The problem is that there is no guarantee php://input is seekable.
So the only real solution if the stream is not seekable and the raw data needs to access more than once is to copy the stream.

Right. I see what you're saying now. The problem is that we can't copy the stream as there's no method in StreamInterface that provide access to the underlying stream object.

Thoughts?

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 13, 2019

Author Member

A further note. It looks like this has to be solved at the PSR-7 implementation level. e.g. Diactoros's PhpInputStream which is then used by ServerRequest.

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 15, 2019

Author Member

@anfly0 I'm confused trying to do this.

If the stream is not seeable, then we can't rewind it and hence can't parse it in this middleware. Have I missed something?

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 15, 2019

Author Member

So maybe I copy the stream if it's not seekable and $stream->tell() is 0?

This comment has been minimized.

Copy link
@anfly0

anfly0 Aug 15, 2019

@akrabat I'm no 100 on this, but I think that $stream->tell() can return 0 on a not seekable stream even if some of the data has been read.
Offsets have no real meaning if the stream is not seekable.

One idea is to copy the stream if it's not seekable and then compare the size of the new stream with what's in the Content-Length header.
If the match it's reasonably safe to assume that all data was copied and if they don't some error handling can be done.
And then parse the contents.

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 15, 2019

Author Member

How do we get a non-seeable stream into a ServerRequest?

This comment has been minimized.

Copy link
@anfly0

anfly0 Aug 15, 2019

I'm not sure I understand the question.
The PSR-7 implementation used is free to use whatever resource or method to create the stream.
It is an edge case, but it is entirely possible for the stream not to be seekable.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

@akrabat

$app->addBodyParsing();

I'd maybe rename that to addBodyParsingMiddleware(), I'd like to keep that suffix for all helper methods.

I mean theoretically we should probably do a helper for all internal middleware:

  • ErrorMiddleware - Already Done
  • RoutingMiddleware - Already Done
  • BodyParsingMiddleware - We should add it
  • ContentLengthMiddleware - We should add it
  • MethodOverrideMiddleware - We should add it

What do you think @adriansuter

@adriansuter

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

As long as there aren't dozens of internal middleware, why not. This would make things definitely more developer friendly.

But we need to pay attention not to blow up our App class.

Unfortunately this works only for internal middleware. So we would mix two "styles" of adding middleware. I'm not too happy with that, as I would prefer to make things consistent. But that is some sort of compromise, I guess.

@juliangut

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

  • ErrorMiddleware - Already Done

  • RoutingMiddleware - Already Done

  • BodyParsingMiddleware - We should add it

  • ContentLengthMiddleware - We should add it

  • MethodOverrideMiddleware - We should add it

Why not basic auth, cors, trailing slash, http cache or gzip/deflate encoding middlewares? I mean routing and error must of course be bundled with Slim but apart from those, there are already great PSR15 middleware out there for all those things

I was surprised yesterday when I saw this PR, I guess it is because you normally receive questions about how to support X payload, but that can be documented

Just my thoughts if they are of any use

@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Why not basic auth, cors, trailing slash, http cache or gzip/deflate encoding middlewares? I mean routing and error must of course be bundled with Slim but apart from those, there are already great PSR15 middleware out there for all those things

The ones that @l0gicgate listed all functionality that was built into Slim 3. With Slim 4 we have moved them into separate components so that the user can now choose to replace with more appropriate solutions for their use-case.

I was surprised yesterday when I saw this PR, I guess it is because you normally receive questions about how to support X payload, but that can be documented

This one has come about because the solution we thought we had with Slim-Http does't work as well as we'd like with PSR-15, so this PR addresses that gap.

I don't think we're against supplying other middleware as optional components if we feel that it's something that Slim should come with out of the box and there's not a wide-used, simple-to-use option out there already.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@adriansuter

I would prefer to make things consistent

I'm hoping that this will be it for our internal middleware suite. I like consistency as well. So with that line of thinking I guess we should add helper methods for all of the internal middleware.

@akrabat

We can do that in a different PR though.

@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

We can do that in a different PR though.

I've added App::addBodyParsingMiddleware() to this PR. I agree that separate PRs for the other middleware makes sense.

@shadowhand
Copy link
Contributor

left a comment

As @juliangut says, middlewares/payload already exists and works for any PSR-15 system.

At minimum, I feel like this kind of solution would be better published as a separate package (or additional functionality contributed to middlewares/payload) that anyone can use with PSR-15 dispatchers. To me this largely feels like "not invented here" and, while pretty easy to use for simple applications, is not particularly flexible.

* @param callable $callable A callable that returns parsed contents for media type.
* @return self
*/
public function registerBodyParser(string $mediaType, callable $callable): self

This comment has been minimized.

Copy link
@shadowhand

shadowhand Aug 14, 2019

Contributor

Rather than a generic callable, why not create an interface?

interface BodyParser
{
    public function canParse(string $contentType): bool;

    /**
     * @return mixed
     */
    public function parse(StreamInterface $stream);
}

Then you can do:

$middleware = new BodyParsingMiddleware();
$middleware->register(new JsonParser());
$middleware->register(new XmlParser());
// etc

This comment has been minimized.

Copy link
@l0gicgate

l0gicgate Aug 14, 2019

Contributor

I don't disagree with you on this. This to me feels like it should be extracted in its own package if we're going to go that route so we can contain the files contextually. The middleware we ship right now are self-contained into one file.

protected function registerDefaultBodyParsers(): void
{
$this->registerBodyParser('application/json', function ($input) {

This comment has been minimized.

Copy link
@shadowhand

shadowhand Aug 14, 2019

Contributor

This is fine for basic applications, but what about users of JSON-API that use application/vnd.api+json as the content type? The parsing is exactly the same but this hard lock on application/json won't work.

// If not, look for a media type with a structured syntax suffix (RFC 6839)
$parts = explode('+', $mediaType);
if (count($parts) >= 2) {
$mediaType = 'application/' . $parts[count($parts) - 1];

This comment has been minimized.

Copy link
@shadowhand

shadowhand Aug 14, 2019

Contributor

Okay, I get that this is supposed to handle application/vnd.api+json using application/json parser... that behavior is not particularly obvious though, and would be better (IMO) handled on a case by case basis in a specific parser.

akrabat added 6 commits Aug 13, 2019
Add BodyParsingMiddleware
This middleware will parse the body of a PSR-7 ServerRequest and if the
content-type is known and a parser is registered, it will add the
parsed data to the request object before passing to the next middleware.

Usage:

    use Slim\Factory\AppFactory;
    use Slim\Middleware\BodyParsingMiddleware;
    use Slim\Psr7\Response;

    $app = AppFactory::create();
    $app->add(new BodyParsingMiddleware());

    $app->post('/foo', function ($request) {
        $data = $request->getParsedBody();

        $response = new Response();
        $response->getBody()->write(
            print_r($data, true)
        );
        return new Response();
    });

    $app->run();
akrabat added 4 commits Aug 13, 2019
Add addBodyParsingMiddleware() to App
This follows the same style as addRoutingMiddleware() and
addErrorMiddleware() in order to have consistent developer experience
when adding a built-in Slim middleware component.

@akrabat akrabat force-pushed the akrabat:body-parsing-middleware branch from 2b3c124 to 84fe283 Aug 15, 2019

@coveralls

This comment has been minimized.

Copy link

commented Aug 15, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 84fe283 on akrabat:body-parsing-middleware into 86a22f7 on slimphp:4.x.

@l0gicgate l0gicgate merged commit 907d149 into slimphp:4.x Aug 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

@l0gicgate I'm happy with this as the first cut. The only potential outstanding issue is related to non-seekable streams and I'd like to see a real use-case for a non-seeable stream in a ServerRequest before try to solve it. The most common case is php://input which reports as seekable, but isn't really for PUT, so that needs to be handled at the PSR-7 implementation level..

@akrabat akrabat deleted the akrabat:body-parsing-middleware branch Aug 16, 2019

@l0gicgate l0gicgate referenced this pull request Aug 20, 2019
@esetnik

This comment has been minimized.

Copy link

commented Aug 21, 2019

Are we supposed to use $app->addBodyParsingMiddleware(); if we are also using slim-http? It seems this functionality is already included in slim-http so maybe it can be removed now that it's available in slim itself? See slimphp/Slim-Http#99

@l0gicgate

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

@esetnik no need to use it no. And we are not removing it as you may use Slim-Http without using Slim

@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

It is beneficial to have it in Slim-Http to help migration from Slim 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.