Middleware Runner #215

WyriHaximus · 2017-09-06T17:40:47Z

Middleware Runner extracted from #213

jsor · 2017-09-06T17:59:21Z

README.md

@@ -136,6 +137,78 @@ $server->on('error', function (Exception $e) {
 Note that the request object can also emit an error.
 Check out [request](#request) for more details.

+### Middleware
+
+Middleware can ba added to the server using [`MiddlewareRunner`](src/MiddlewareRunner.php)


Typo: ba -> be

jsor · 2017-09-06T18:02:17Z

README.md

+### Middleware
+
+Middleware can ba added to the server using [`MiddlewareRunner`](src/MiddlewareRunner.php)
+instead of the the `callable`. A middleware is expected adjure to the following rules:


the ~~the~~

adjure -> to adhere (?)

jsor · 2017-09-06T18:04:14Z

README.md

+));
+```
+
+#### Buffer


The Buffer and LimitHandlers are not part of this PR and the sections should probably removed.

Yes correct, a PR for Buffer is coming soon and LimitHandlers after that. Then two more PR's will follow, one for the readme section and one for examples

jsor · 2017-09-06T18:12:33Z

README.md

+
+* It is a `callable`.
+* It accepts `ServerRequestInterface` as first argument and `callable` as second argument.
+* It either returns a promise resolving to a `ResponseInterface`; or it calls the second argument


It is actually allowed to return a ResponseInterface directly (not only a Promise resolving to a ResponseInterface).

How about this?

It is a callable.

It accepts ServerRequestInterface as first argument and optional callable as second argument.

It returns a ResponseInterface (or a PromiseInterface resolving to a ResponseInterface)

It calls $next($request) to continue processing the next middleware function or returns explicitly to abort the chain

It returns a ResponseInterface (or a PromiseInterface resolving to a ResponseInterface)

I'd still write just promise because you can return any thenable which can be consumed by react/promise.

Valid point, just link to the promise repo and maybe provide a small example below? 👍

Suggestion:

It returns a ResponseInterface (or any promise which can be consumed by Promise\resolve() resolving to a ResponseInterface)

No objection, just wanted to point out that if probably makes sense to keep this in line with the first paragraph here: https://github.com/reactphp/http#response

Considering the other chapters, doesn't it make more sense to have this chapter below the response chapter? Middleware behave transparently to the Server, so most of what is described in "Response" still applies to middleware and the Server alike.

Makes sense 👍

jsor · 2017-09-06T18:13:13Z

src/MiddlewareRunner.php

+                    $middlewareCollection
+                )
+            );
+            Promise\resolve($cancel)->then($resolve, $reject);


This can be shortened to $resolve($cancel).

clue

Awesome, would love to get this feature in! 🎉

Only have some minor remarks besides the small oversights @jsor already mentioned 👍

clue · 2017-09-07T09:34:19Z

README.md

+
+```php
+$server = new Server(new MiddlewareRunner(
+    new Response(404), // Default response if no middleware returns a promise


Not sure about this "default response" parameter and what its use case would be. I feel like "no middleware returns a promise" would be a violation of this API and should result in an internal server error?

I would love to get this feature out, so I'd vote for removing this parameter for now and consider re-introducing this less prominently at a later time if we see a good use case? 👍

What would you suggest the runner does when it doesn't have any middleware to run? Throw an exception?

Actually that would make sense as it's turned into a 500 because it is an internal server error \o/

clue · 2017-09-07T10:20:21Z

README.md

+
+* It is a `callable`.
+* It accepts `ServerRequestInterface` as first argument and `callable` as second argument.
+* It either returns a promise resolving to a `ResponseInterface`; or it calls the second argument


How about this?

It is a callable.

It accepts ServerRequestInterface as first argument and optional callable as second argument.

It returns a ResponseInterface (or a PromiseInterface resolving to a ResponseInterface)

It calls $next($request) to continue processing the next middleware function or returns explicitly to abort the chain

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

src/MiddlewareRunner.php

+    private $middleware = array();
+
+    /**
+     * @param ResponseInterface $response


See above 👍

clue · 2017-09-07T10:22:21Z

src/MiddlewareRunner.php

+                    $defaultResponse,
+                    $middlewareCollection
+                )
+            );


I'm okay with this recursive implementation for now, but this is certainly something we would want to look into again in the future 👍

Yup, it works for now and I'll revisit it in the future 👍

WyriHaximus · 2017-09-07T19:52:41Z

Rebased and squashed the commits

davidwdan · 2017-09-07T20:59:38Z

README.md

+    function (ServerRequestInterface $request, callable $next) {
+        $request = $request->withHeader('Request-Time', time());
+        return $next($request);
+    }


Missing a comma:)

clue

I'd really love to get this in, we can still (should) update the documentation to include the motivation for middleware in a follow-up PR 👍 🎉

clue · 2017-09-08T06:39:28Z

README.md

+### Middleware
+
+Middleware can be added to the server using [`MiddlewareRunner`](src/MiddlewareRunner.php)
+instead of the `callable`. A middleware is expected to adjure the following rules:


@jsor suggested adhere instead?

@clue whoops thought I fixed that one, fixed it, squashed it, and pushed it again 🎉

WyriHaximus added easy pick help wanted new feature labels Sep 6, 2017

WyriHaximus added this to the v0.8.0 milestone Sep 6, 2017

WyriHaximus requested review from jsor and clue September 6, 2017 17:40

WyriHaximus mentioned this pull request Sep 6, 2017

Middleware #213

Closed

jsor reviewed Sep 6, 2017

View reviewed changes

WyriHaximus mentioned this pull request Sep 6, 2017

Support buffering request body (RequestBodyBufferMiddleware) #216

Merged

clue reviewed Sep 7, 2017

View reviewed changes

WyriHaximus added a commit to WyriHaximus/http that referenced this pull request Sep 7, 2017

Use shorter notation as suggested by @jsor at reactphp#215 (comment)

71add86

jsor approved these changes Sep 7, 2017

View reviewed changes

WyriHaximus force-pushed the middleware-runner branch from c40aa7d to 865facd Compare September 7, 2017 19:51

davidwdan reviewed Sep 7, 2017

View reviewed changes

WyriHaximus force-pushed the middleware-runner branch 2 times, most recently from 0036e0b to e12aee6 Compare September 8, 2017 05:48

clue approved these changes Sep 8, 2017

View reviewed changes

clue removed the easy pick label Sep 8, 2017

Middleware Runner

1c82840

WyriHaximus force-pushed the middleware-runner branch from 5a0ba25 to 1c82840 Compare September 8, 2017 06:46

WyriHaximus merged commit 2d57b8c into reactphp:master Sep 8, 2017

This was referenced Sep 8, 2017

HTTP middleware support #179

Closed

Rename Server to StreamingServer #197

Closed

This was referenced Sep 10, 2017

LimitHandlers Middleware #218

Closed

Support parsing urlencoded request body (RequestBodyParserMiddleware) #220

Merged

jsor mentioned this pull request Sep 14, 2017

[RFC] Middleware, where do they belong #221

Closed

clue mentioned this pull request Dec 10, 2017

Simplify using middleware request handlers #277

Merged

Uh oh!

Middleware Runner #215

Middleware Runner #215

Uh oh!

Conversation

WyriHaximus commented Sep 6, 2017

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!

jsor Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

clue left a comment

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!

WyriHaximus commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clue left a comment

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!

jsor Sep 6, 2017 •

edited

Loading