Middleware #213

WyriHaximus · 2017-08-19T06:27:33Z

During DPC @clue and I had a long chat about how to handle body parsers without loosing the current flexibility. We came to the conclusion that middlewares are the way to go. The following proposal is inspired by the WIP PSR-15 but doesn't implement it (I'll get back on that later in this PR).

Major changes

Where 0.7 only requires you to pass a callable to handle incoming requests, this PR proposes to use middleware for that. (While still leaving the callable way intact by magically wrapping it.) One way to setup middleware by passing an array with middlewares implementing MiddlewareInterface:

$server = new Server([
    new Buffer(),
    new Callback(function ($request) {
        return new Response(200);
    }),
]);

Or by passing in a concrete implementation of MiddlewareStackInterface:

$server = new Server(new MiddlewareStack([
    new Buffer(),
    new Callback(function (ServerRequestInterface $request) {
        return new Response(200);
    }),
]));

The latter is done automatically when doing the former internally in the server. But you can create your own middleware stack implementation and use that instead.

Included middlewares

This PR includes three middlewares Buffer, Callback, and LimitHandlers.

Buffer

Buffers the request body until it is fully in or when it reach the given size:

$middlewares = [
    new Buffer(),
];

Callback

Handles request just like 0.7 using a callback:

$middlewares = [
    new Callback(function (ServerRequestInterface $request) {
        return new Response(200);
    }),
];

LimitHandlers

Limits the number of concurrent request being handled at the same time:

$middlewares = [
    new LimitHandlers(10),
];

Body parsing

Currently one of our main issues with body parsing is the memory usage. But with the LimitHandlers we can pause the incoming request stream until we're ready handle it thus limiting the amount of memory needed to handle request. And with Buffer we can first stream the body in, parse it with for example BodyParser, and then hand it to the next middleware on the stack.

PSR-15

While the middleware implementation in this PR is inspired by the work done for the coming PSR-15 it doesn't implement it due to a small but major different. This implementation relies on promises where PSR-15 always assumes a response to be returned. An adapter for PSR-15 is beyond the scope of this PR but not for a Friends of ReactPHP packages and thus I've 8000 created for/http-middleware-psr15-adapter that utilizes RecoilPHP and on the fly PHP parsing and rewriting of PSR-15 middlewares.

With that package you can wrap the adapter around a PSR-15 as follows:

$middlewares = [
    new PSR15Middleware(
        $loop, 
        'Namespace\MiddlewareClassname', 
        [/** Constructor arguments */]
    ),
];

*Note: I've added last section to show how easy it can be to add PSR-15 middlewares, with the rewriting there is no guarantee for success

# Conflicts: # composer.json

WyriHaximus · 2017-08-19T07:39:25Z

Ping: @clue @jsor @andig @marcj

andig · 2017-08-19T08:31:23Z

Would it make sense- in terms of backwards compatibility- to allow the server to apply a bit of magic? If the parameter is a callable, wrap it in a Callback. If it's not an array, wrap it in an array? This would also allow for a bit less code and indents for something like the 95% use case where you might only want your callable to be called without limiting or similar?

WyriHaximus · 2017-08-19T09:24:33Z

@andig that does make a lot of sense to me, I'll update the PR with that later today 👍

jsor · 2017-08-19T13:05:26Z

src/MiddlewareStack.php

+
+namespace React\Http;
+
+use Interop\Http\ServerMiddleware\MiddlewareInterface;


Must be React\Http\MiddlewareInterface.

jsor · 2017-08-19T13:10:59Z

src/MiddlewareInterface.php

+     * @param ServerRequestInterface $request
+     * @param MiddlewareStackInterface $stack
+     *
+     * @return PromiseInterface<ResponseInterface>


Can also return a response directly: ResponseInterface|PromiseInterface<ResponseInterface>

jsor · 2017-08-19T13:13:10Z

src/Server.php

        }

-        $this->callback = $callback;
+        throw new \InvalidArgumentException();


Exception message missing explaining what types of arguments can be given to the constructor.

…as return types MiddlewareInterface::process

mdrost · 2017-08-19T14:56:36Z

src/Server.php

        }

-        $this->callback = $callback;
+        throw new \InvalidArgumentException('Only MiddlewareInterface[] or MiddlewareStackInterface implementations are supported');


Would not it be better to use fully qualified names?

…apped into a middleware stack; restoring old behavior while still supporting middleware

…tException

WyriHaximus · 2017-08-19T15:59:45Z

@andig, @mdrost, and @jsor I've updated the PR with your suggestions and comments

andig · 2017-08-19T17:09:06Z

Passing a Callback without array is not supported yet. But maybe that case isn‘t that important after all. I really like the original, simple callable stile. Thank you!

WyriHaximus · 2017-08-19T20:39:35Z

@andig it is, I've reverted that change and you can now pass in a callable, an array with middlewares or a middleware stack to handle your request.

jsor · 2017-08-21T11:56:28Z

src/Server.php

-        $promise = new Promise(function ($resolve, $reject) use ($callback, $request, &$cancel) {
-            $cancel = $callback($request);
+        $promise = new Promise\Promise(function ($resolve, $reject) use ($middlewareStack, $request, &$cancel) {
+            $cancel = $middlewareStack->process($request);
            $resolve($cancel);
        });


Since MiddlewareStack::process() always returns a promise, this can be simplified to just

$cancel = $middlewareStack->process($request);

Note, that catching exceptions should be moved to the MiddlewareStack, see #213 (comment)

jsor · 2017-08-21T12:01:39Z

src/MiddlewareStack.php

+                )
+            )
+        );
+    }


To prevent exceptions thrown from process() bubbling out from here, this should be wrapped like this to turn them into rejections:

return new Promise\Promise(function ($resolve, $reject) use ($request, $middlewares, $middleware) { $response = $middleware->process( $request, new self( $this->defaultResponse, $middlewares ) ); $resolve($response); });

…ck; also forward cancels correctly

WyriHaximus · 2017-08-22T21:13:49Z

@jsor Updated with your suggestions, had a lot of fun with the cancellation forwarding

… runners/stacks) are callables. As a result the Callback middleware has been removed as it has become obsolete

jsor · 2017-09-04T20:01:16Z

src/Server.php

- *     );
- * });
+ * $server = new Server([
+ *     new Callback(function (ServerRequestInterface $request) {


Still using Callback removed in b4211aa

jsor · 2017-09-04T20:02:54Z

README.md

+$middlewares = [
+    new LimitHandlers(5),
+    new Buffer(),
+    new Callback(function (ServerRequestInterface $request) {


Still using Callback removed in b4211aa

jsor · 2017-09-04T20:03:01Z

README.md

+        return new Response(200);
+    }),
+];
+```


Leftover from b4211aa

jsor · 2017-09-04T20:03:41Z

examples/12-middleware.php

+$server = new Server(array(
+    new LimitHandlers(3), // Only handle three requests concurrently
+    new Buffer(), // Buffer the whole request body before proceeding
+    new Callback(function (ServerRequestInterface $request) use ($loop) {


Still using Callback removed in b4211aa

jsor · 2017-09-04T20:03:53Z

examples/13-request-sec.php

+    new Response(500),
+    array(
+        new Incre($counts),
+        new Callback(function (ServerRequestInterface $request) use ($loop) {


Still using Callback removed in b4211aa

jsor · 2017-09-04T20:05:41Z

tests/FunctionalServerTest.php

@@ -2,6 +2,8 @@

 namespace React\Tests\Http;

+use React\Http\Middleware\Buffer;
+use React\Http\Middleware\Callback;


Unused use's?

jsor · 2017-09-04T20:12:44Z

src/Server.php

-        $promise = new Promise(function ($resolve, $reject) use ($callback, $request, &$cancel) {
-            $cancel = $callback($request);
+        $promise = new Promise\Promise(function ($resolve, $reject) use ($request, &$cancel) {
+            $callable = $this->callable;


$this isn't available in PHP 5.3, see https://travis-ci.org/reactphp/http/jobs/271815811

jsor · 2017-09-04T20:15:23Z

src/MiddlewareStack.php

+                    $middlewares
+                )
+            );
+            $cancel->done($resolve, $reject);


Since react/promise v1.x is supported, done() can't be used as its only available since v2.0. See https://travis-ci.org/reactphp/http/jobs/271815812

jsor · 2017-09-04T20:16:16Z

src/Server.php

+            $callable = $this->callable;
+            $cancel = $callable($request);
+            if ($cancel instanceof Promise\CancellablePromiseInterface) {
+                $cancel->done($resolve, $reject);


No done() because react/promise v1.x.

WyriHaximus · 2017-09-04T20:18:24Z

Updated the PR (except documentation, examples and readme) turning all middlewares into invokables and also the MiddlewareStack. The idea behind it is that it doesn't require any code changes and thus is backwards compatible from the consumer end. Whilest still allowing us to add middleware

Ping @clue

WyriHaximus · 2017-09-04T20:27:51Z

@jsor updated your points and the examples

jsor · 2017-09-04T20:47:30Z

src/MiddlewareRunner.php

+        $that = $this;
+        $cancel = null;
+                      return new Promise\Promise(function ($resolve, $reject) use ($middleware, $request, $middlewares, &$cancel, $that) {

            
+            $cancel = $middleware(


Since we're not working on an interface anymore (but on plain callables), there must be a check if $cancel is actually a promise (or not a Response).

WyriHaximus · 2017-09-06T17:46:45Z

Extracted the Middleware Runner into #215

WyriHaximus · 2017-09-06T20:13:31Z

Extracted the Buffer middleware into #216

clue · 2017-09-07T10:26:48Z

Let me start with saying awesome! 🎉 It's really nice to see how our discussion and our very quick prototype at this year's DPC turned out in this PR!

I'd love to get this feature in ASAP, but this PR contains quite a few changes, so it's really hard to review in its current state. I see you've started splitting this into smaller PRs (#215 and #216), which I think makes perfect sense, so let's get this rolling!

andig · 2017-09-17T10:39:30Z

README.md

+
+#### Callback
+
+A few build in middlewares are shipped with this package, one of them is


should be built-in?

andig · 2017-09-17T10:39:53Z

README.md

+
+#### Buffer
+
+Another build in middleware is `Buffer` which will buffer the incoming 


should be built-in?

This PR is superseded by smaller PR's split off, eg. #216 (which is already merged). This PR should probably closed by @WyriHaximus to avoid confusion.

andig · 2017-10-03T09:39:50Z

@WyriHaximus thanks for all your efforts. Looking at react/http from consumer side I've been wondering why you're not using PSR-15?

WyriHaximus · 2017-10-04T19:51:33Z

@andig One of the reasons is that PSR-15 requires a response from the middleware where we return either a response or a promise (we wrap the response in a promise anyway). Also going with callable's opens up a lot more flexibility letting you use closures just as easily as [$class, 'method'] or $class that has a __invoke. In case you want PSR-15 I've crafted https://github.com/friends-of-reactphp/http-middleware-psr15-adapter so you can use PSR-15 middleware as react/http middleware.

WyriHaximus · 2017-10-04T19:55:48Z

FYI here is an example of a project where I'm using that adapter:

    $middleware = [
        new PSR15Middleware($loop, Uuid::class),
        new PSR15Middleware($loop, ClientIp::class),
        new PSR15Middleware($loop, AccessLog::class, [$logger], function ($accessLog) {
            return $accessLog->context(function (ServerRequestInterface $request, ResponseInterface $response) {
                return [
                    'client-ip' => $request->getAttribute('client-ip'),
                    'request-id' => $request->getHeaderLine('X-Uuid'),
                    'response-time' =>$response->getHeaderLine('X-Response-Time'),
                ];
            });
        }),
        new PSR15Middleware($loop, ResponseTime::class),
        new WithHeadersMiddleware([
            'X-Powered-By' => 'wyrimaps.net-static-maps-server (' . $version . ')',
        ]),
        new WithRandomHeadersMiddleware(Horde::headers(), 1, 2),
        new RequestBodyBufferMiddleware(),
        new CustomRequestBodyParsers(),
        new ClearBodyMiddleware(),
        new PSR15Middleware($loop, Expires::class, [$expires]),
        new PSR15Middleware($loop, Redirect::class, [[
                '/' => 'https://wyrimaps.net/',
                '/favicon.ico' => 'https://wyrimaps.net/favicon.ico',
        ]]),
        new PSR15Middleware($loop, Robots::class, [false]),
        new PSR15Middleware($loop, FastRoute::class, [$dispatcher]),
        new PSR15Middleware($loop, RequestHandler::class),
    ];
    $server = new HttpServer(new MiddlewareRunner($middleware));

WyriHaximus added 7 commits August 16, 2017 08:03

Middleware

5e29f51

Merged in master

6ecd9dc

Merge branch 'master' into middleware

e883ae8

# Conflicts: # composer.json

Updated Server.php documentation

9e318ed

Updated readme code examples

0447be8

Use MiddlewareStack directly in example 13

1eac846

Document middleware in the readme

f7fa17f

WyriHaximus added BC break help wanted new feature labels Aug 19, 2017

WyriHaximus added this to the v0.8.0 milestone Aug 19, 2017

WyriHaximus requested review from jsor and clue August 19, 2017 06:27

Removed PSR15RecoilMiddlewareStack as it is implemented in a for package

f27febb

Removed unused class imports from Server.php

ad7f8bb

jsor reviewed Aug 19, 2017

View reviewed changes

WyriHaximus added 3 commits August 19, 2017 16:16

Removed unused class imports from MiddlewareStack.php

4699ea0

Allow both ResponseInterface and PromiseInterface<ResponseInterface> …

9c3a09a

…as return types MiddlewareInterface::process

Added supported types for Server::__construct InvalidArgumentException

ba9e744

mdrost reviewed Aug 19, 2017

View reviewed changes

WyriHaximus added 3 commits August 19, 2017 17:20

When passing a callable to Server::__construct it is automatically wr…

ece043b

…apped into a middleware stack; restoring old behavior while still supporting middleware

PHP 5.3 compatibility fixes

21aff47

Use fully qualified class names in Server::__construct InvalidArgumen…

8000

567f924

…tException

jsor reviewed Aug 21, 2017

View reviewed changes

WyriHaximus added 2 commits August 21, 2017 15:54

Catch exceptions in MiddlewareStack

1dfb784

Moved exception catching to the Callback middleware and MiddlewareSta…

1052597

…ck; also forward cancels correctly

WyriHaximus added 3 commits August 22, 2017 23:17

self => MiddlewareStack

8cfcdf5

PHP 5.3 compatibility

3fef536

Removed middleware related interfaces and assume all middlewares (and…

b4211aa

… runners/stacks) are callables. As a result the Callback middleware has been removed as it has become obsolete

jsor reviewed Sep 4, 2017

View reviewed changes

Updated examples and renamed the MiddlewareStack to MiddlewareRunner

0f6df46

jsor reviewed Sep 4, 2017

View reviewed changes

WyriHaximus added 2 commits September 5, 2017 16:55

Restored Server.php documentation

7c32bc1

Restored Server.php constructor documentation

4e9d514

jsor mentioned this pull request Sep 6, 2017

Follower cancellation propagation reactphp/promise#99

Merged

WyriHaximus mentioned this pull request Sep 6, 2017

Middleware Runner #215

Merged

WyriHaximus mentioned this pull request Sep 6, 2017

Support buffering request body (RequestBodyBufferMiddleware) #216

Merged

WyriHaximus mentioned this pull request Sep 10, 2017

LimitHandlers Middleware #218

Closed

andig reviewed Sep 17, 2017

View reviewed changes

WyriHaximus closed this Sep 17, 2017


		namespace React\Http;

		use Interop\Http\ServerMiddleware\MiddlewareInterface;


		#### Callback

		A few build in middlewares are shipped with this package, one of them is


		#### Buffer

		Another build in middleware is `Buffer` which will buffer the incoming

Uh oh!

Middleware #213

Middleware #213

Uh oh!

Conversation

Uh oh!

Suggested reading order

Major changes

Included middlewares

Buffer

Callback

LimitHandlers

Body parsing

PSR-15

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!