[PSR-15] Provide middleware dispatch examples #996

weierophinney · 2018-01-04T22:56:32Z

Per discussions on #987, on the mailing list, and in the PSR-15 working group, this patch builds on #987 to add a new section 6.3 to the meta document, detailing "Example Interface Interactions".

The section covers the following:

A queue-based request handler that aggregates and dispatches middleware.
A decorator-based request handler for building a layered dispatch mechanism.
Two examples of re-usable middleware to demonstrate de-coupling from the application and the request handler.

This patch incorporates feedback made on #987, but not incorporated there.

weierophinney · 2018-01-04T23:06:35Z

Ping @Crell — I think the new section answers your questions, but would like for you to review and confirm.

Also pinging @shadowhand — do the changes look okay for you? Also, I rebased off of master, which picked up changes merged since you opened #987.

Crell · 2018-01-04T23:20:20Z

proposed/http-handlers/request-handlers-meta.md

+
+- Middleware does not need to know anything about any other middleware or how it
+  is composed in the application.
+- The `QueueRequestHandler` is agnostic of the PSR-7 implementation in use


Missing period on this line.

Crell · 2018-01-04T23:24:01Z

proposed/http-handlers/request-handlers-meta.md

+This system has essentially two request handlers, one that will produce a
+response if no middleware generates one, and one for dispatching the middleware
+layers. (In this example, the `RoutingMiddleware` will likely execute composed
+handlers on a successful route match; see more on that below.)


I don't really see 2 handlers here... I see one handler ($app) that will end up invoked 2-3 times. Am I missing something or is this worded awkwardly?

No, you're correct. The original example I created composed another request handler instead of a request prototype, and delegated to that handler if the last middleware in the queue calls on the handler instead of returning a response. I'm going to refactor the example to to do that, as it also then answers the "request prototype" question you have elsewhere.

Crell · 2018-01-04T23:29:34Z

proposed/http-handlers/request-handlers-meta.md

+    }
+}
+
+$innerHandler = new class ($responsePrototype) implements RequestHandlerInterface {


Nitpick: $responsePrototype isn't defined yet. Is this supposed to be a new Response())->withStatus(500)?

It could be, or a 404 status. I'll update to make this more clear.

Crell · 2018-01-04T23:35:01Z

proposed/http-handlers/request-handlers-meta.md

+Note the use of a "response prototype" in the above example; this approach
+allows the middleware to be agnostic of which PSR-7 implementation is in use,
+and yet still return a response in cases where it determines it should not
+delegate to the handler.Similarly, it is not concerned with how the request


Missing space on this line.

Crell · 2018-01-04T23:38:37Z

proposed/http-handlers/request-handlers-meta.md

+these to work in either situation, we need to write them such that they interact
+appropriately.
+
+Generally speaking, implementors of middleware should follow these guidelines:


I feel like these guidelines should be moved to the spec proper, in some form, as they directly inform how middlewares will interoperate. Eg, when to call the next handler cannot be captured in an interface but is still relevant, and if done "wrong" will lead to a non-interoperable implementation.

@shadowhand What do you think about moving these into the spec proper? If we do, where should they live?

I don't think we should but putting code samples in the spec itself. Having the guidelines at the top of the spec would be good though.

Yes, I was referring to the guidelines, (probably written a bit differently). I agree the code samples should remain in the metadoc.

I really think the spec should not dictate how middlewares should behave (or other implementation details), e.g. when to generate a response or call the delegate.

Maybe that's not what you meant, but the guidelines below are too precise IMO.

I agree with @mnapoli: I don't think the spec document should contain these guidelines:
they are very specific and detailed, and they don't actually describe the middleware themselves, they describe how to compose and dispatch them, which is a non-goal of the spec.

@mnapoli and @stefanotorresi — thanks; I think that's great feedback, and I tend to agree. The guidelines presented are implementation details and good practices, but do not need to be followed in order to comply with the specification. I'll leave them where they are.

Could you make that part clear in the text? Because it reads like a specification (with the "should" keyword).

I'm not sure what other word to user here, TBH, and the fact that it's not ALL CAPS means it does not have the same semantic weight per RFC 2119. Maybe the following?

Implementors of middleware striving for maximum interoperability may want to consider the following guidelines:

Would that work for you, @mnapoli ?

Crell · 2018-01-04T23:39:23Z

proposed/http-handlers/request-handlers-meta.md

+
+        return $corsResult->isCorsScoped()
+            ? $corsResult->prepareResponse($this->responsePrototype)
+            : $response;


It's cool to see someone besides me that uses/likes that particular formatting idiom. 😄

Crell · 2018-01-05T00:05:07Z

proposed/http-handlers/request-handlers-meta.md

@@ -275,6 +276,10 @@ and a request handler and must return a response. The middleware may:
 - Create and return a response without passing the request to the request handler,
  thereby handling the request itself.

+It is expected that a middleware dispatching system will use the request handler
+to delegate response creation to the next available middleware. The last middleware
+would then act as a domain gateway to execute application code.


This paragraph... doesn't quite make sense. I can't wrap my head around what it's saying. Let me try to rephrase and tell me if I'm in the right ballpark:

When delegating from one middleware to the next in a sequence, a dispatching system should use an intermediary request handler as a way to link two middlewares together. The final or innermost middleware acts as a gateway to application code that will, in most cases, be responsible for response generation.

Is that accurate? (Feel free to steal liberally if so.) Should that be in the main spec in some form?

It's correct, except that the "application code that will, in most cases, be responsible for response generation" will likely either be the middleware itself, or the a request handler. I'll see if I can come up with a re-phrase that makes sense.

Crell · 2018-01-05T00:05:33Z

This is a huge improvement, but still some bits aren't as direct as they could be. See commentary above.

mnapoli · 2018-01-05T07:19:14Z

proposed/http-handlers/request-handlers.md

+ *
+ * An HTTP middleware component participates in processing an HTTP message;
+ * for example, by acting on the request, generating the response, or forwarding the
+ * request to a subsequent middleware and possibly acting on its response.


shadowhand · 2018-01-05T15:16:38Z

proposed/http-handlers/request-handlers-meta.md

+
+The two interfaces, `RequestHandlerInterface` and `MiddlewareInterface`, were
+designed to work in conjunction with one another. While a
+`RequestHandlerInterface` can feasibly be used in isolation, it gains more power


This sentence isn't necessary, imo. RequestHandler is perfectly viable without middleware, not the other way around.

shadowhand · 2018-01-05T15:19:54Z

proposed/http-handlers/request-handlers-meta.md

+  a new response by manipulating the one returned (e.g., `return
+  $response->withHeader('X-Foo-Bar', 'baz')`).
+
+The `CorsMiddleware` is one that will exercise all three of these guidelines: if


I am mildly uncomfortable using a highly specific example like this. This assumes that I know what CORS is and what the failure conditions mean. If we're going to use a specific example perhaps something like routing would be more generally understood?

I'll see if I can come up with another example. This was one I liked because it hits all the various ways middleware might produce a response:

on its own, due to a failed pre-condition

by acting as a pass-through to the request handler

by modifying the response returned by the request handler

Let me see if I can come up with an example, and I'll ping you if/when I do.

Came up with an example using authorization instead of CORS; please review and let me know what you think.

shadowhand · 2018-01-05T15:22:02Z

proposed/http-handlers/request-handlers-meta.md

+handler is implemented; it merely uses it to produce a response when
+pre-conditions have been met.
+
+The `RoutingMiddleware` implementation described below follows a similar


Yes this is better, but I think the process should be:

If routing matches, continue processing.

Else return a HTTP Not Found response.

That's actually exactly how we do it in Expressive. 👍

The only issue I see with that is that it complicates the example: I then need to add dispatch middleware and a response prototype or factory. I felt the "map route to handler" approach was sufficiently simple and didn't require extra explanation.

shadowhand · 2018-01-05T15:25:14Z

proposed/http-handlers/request-handlers.md

- * An HTTP middleware component participates in processing an HTTP message,
- * either by acting on the request, generating the response, or forwarding the
+ * An HTTP middleware component participates in processing an HTTP message;
+ * for example, by acting on the request, generating the response, or forwarding the


Why the change of phrasing? "Either" seems like the appropriate wording.

This was based on feedback from @mnapoli, IIRC.

This seems fine. If we want to get super pedantic then "either" is technically only correct when there's 2 options, and in this case there are 3. "For example" seems OK to me.

Or, technically, with the semicolon we could omit that word entirely and it would still be a grammatically valid list of the available options.

`s/either by/for example, by/`, with some additional punctuation.

Added a section to demonstrate both two potential middleware dispatch systems (and thus how middleware and handlers interact), as well as examples of re-usable middleware.

Instead of a "response prototype", we instead compose a fallback handler that is invoked with the request if the last middleware in the application delegates to the handler.

@Crell

Per @Crell, the "decorated request handler" example included an anonymous class that composed a `$responsePrototype`... which was not declared elsewhere in the example. This adds a line to declare it, along with a description of different forms it might take.

Removes two sentences that are unnecessary, and potentially contradictory to the specification.

@shadowhand

As @shadowhand points out, CORS is a domain many specification readers may not be familiar with. As such, refactored it to be an authorization middleware, with the following behavior: - If no authorization is required, it is a pass-through proxy to the handler. - If authorization is required, and the request is invalid, it returns an "unauthorized" response. - Otherwise, it passes the request to the handler, and signs the response returned. This approach hits the same points I was trying to make with the CORS example, but with a domain more people will be familiar with.

@Crell

Updated per verbiage from @Crell, with additional clarifications.

weierophinney · 2018-01-05T18:49:48Z

proposed/http-handlers/request-handlers-meta.md

+system should use an intermediary request handler as a way to link middleware
+together. The final or innermost middleware will act as a gateway to application
+code and generate a response from its results; alternately, the middleware may
+delegate this responsibility to a _request handler_.


@Crell This is the updated text, using your suggestion as a starting point.

Much clearer! Why is the last "request handler" set off, though? Perhaps "to a dedicated request handler"? (Nitpicking here.)

A statement of this sort seems like it belongs in the spec. It's not a "why and background", it's a "no, really, this is how you're supposed to use it". That seems like it should be in the spec proper.

weierophinney · 2018-01-05T18:52:59Z

proposed/http-handlers/request-handlers-meta.md

+  the request to the handler, and sign the response returned based on the request.
+
+```php
+class AuthorizationMiddleware implements MiddlewareInterface


@shadowhand Let me know if this altered example (replacing CorsMiddleware with AuthorizationMiddleware) makes more sense. I definitely want to demonstrate multiple middleware, and particularly different approaches middleware can take both to be re-usable as well as to change the workflow/paths of an application.

shadowhand · 2018-01-05T20:12:24Z

proposed/http-handlers/request-handlers-meta.md

+
+    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler) : ResponseInterface
+    {
+        if (! $authorizationMap->requestRequiresAuthorization($request)) {


How about doesRequireAuthorization instead of requestRequiresAuthorization?

Maybe needsAuthorization to keep it short? (for easiness of readability)

shadowhand · 2018-01-05T20:12:46Z

proposed/http-handlers/request-handlers-meta.md

+        $this->responsePrototype = $responsePrototype;
+    }
+
+    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler) : ResponseInterface


): ResponseInterface as per purposed PSR-12 spec, no space.

https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#45-method-and-function-arguments

shadowhand · 2018-01-05T20:17:18Z

proposed/http-handlers/request-handlers-meta.md

+        ResponseInterface $responsePrototype
+    ) {
+        $this->authorizationMap = $authorizationMap;
+        $this->responsePrototype = $responsePrototype;


I'm not sure why we need a response prototype here. I think it would make more sense of the AuthorizationMap was responsible for generating/manipulating the response. That would simplify the example and keep the focus on the middleware.

Agreed; making that change now.

shadowhand · 2018-01-05T20:20:24Z

proposed/http-handlers/request-handlers-meta.md

+these to work in either situation, we need to write them such that they interact
+appropriately.
+
+Generally speaking, implementors of middleware should follow these guidelines:


I don't think we should but putting code samples in the spec itself. Having the guidelines at the top of the spec would be good though.

Crell · 2018-01-06T01:15:03Z

proposed/http-handlers/request-handlers-meta.md

+    public function __construct(
+        AuthorizationMap $authorizationMap,
+        ResponseInterface $responsePrototype
+    ) {


There's only 2 parameters. No need to go with the harder-to-read multi-line approach. Icky.

When using objects as type, I prefer the multi-line approach. Easier to read and less churn on diffs when parameters change.

I was going to disagree, but when I re-formatted, I realized the line is still shorter than the process() method declaration line. About to commit that now.

Plus, with the change to remove the response prototype, it's now only one argument anyways...

Based on further feedback to php-fig#996.

Removes emphasis from "request handler", based on feedback. Additionally, adds the word "dedicated", also per feedback.

@Crell

… spec Per @Crell, this patch moves the paragraph describing the interactions between middleware and request handlers into the spec, under section 1.2.

Mainly, `): ReturnType` vs `) : ReturnType`.

- `requiresRequestAuthorization()` renamed to `needsAuthorization()`. - Removes response prototype from AuthorizationMiddleware, as it's no longer used. - Reformats AuthorizationMiddleware constructor declaration from multi-line to single line; shorter now, due to only one required argument.

weierophinney · 2018-01-08T16:02:29Z

Requested changes have been made (mostly):

I did not move the "guidelines" bullet points to the spec; as several folks noted, coding to those guidelines will definitely mean you're coding to the spec, but you do not need to follow them to implement the spec.
I removed the emphasis around "request handler", and added the word "dedicated" before them, per @Crell.
I moved that particular paragraph to section 1.2 of the spec, per @Crell.
I updated the preamble to MiddlewareInterface to incorporate the various bits of feedback.
I updated code samples to follow PSR-12.
I made changes to the AuthorizationMiddleware example per feedback received.

@Crell, @shadowhand, @mnapoli and @stefanotorresi — can you give one more review so we can finalize these? I think we can then prepare for a CC vote.

shadowhand

My concerns are addressed.

Crell · 2018-01-08T19:57:19Z

proposed/http-handlers/request-handlers.md

+system should use an intermediary request handler as a way to link middleware
+together. The final or innermost middleware will act as a gateway to application
+code and generate a response from its results; alternately, the middleware may
+delegate this responsibility to a dedicated request handler.


Nitpick: SHOULD and MAY should be capitalized to mean the defined SHOULD and MAY, I think, since this is in the spec proper.

My understanding is they should only be capitalized when they have the semantic meanings as defined in RFC 2119; the question is whether or not those words here have those semantic meanings. I did not originally write this with those meanings in mind.

The problem, of course, is that there are very few ways to re-word this to have the same intended meaning without using those words. Can you propose an alternative? Or do you feel they have those semantics?

If not RFC2119, maybe can could be used for both?

CAN also has semantic meaning via RFC 2119. :)

Been discussing this and the idea of moving the section to the spec document itself heavily today; hopefully we can have resolution shortly.

Hmm, I don't see CAN mentioned in the RFC... hence me thinking it safe.

Would something wordy, like could potentially, do the job without sounding like a 2119 directive?

Crell · 2018-01-08T20:05:06Z

proposed/http-handlers/request-handlers-meta.md

+from the outside-in, passing each request handler "layer" to the next outer one.
+
+```php
+class DecoratedRequestHandler implements RequestHandlerInterface


Nitpick: Technically, this isn't a decoratED handler. It's a decoratING handler. The name should be more descriptive. Sorry for not catching that before.

@Crell

Per @Crell

The meta document clearly details that dispatch systems are a NON-GOAL of the specification, so any guidance we provide around that should be in the meta document, and not the specification itself. I have pushed the paragraph back to the section 6.2 pre-amble, with changes as discussed in Slack.

weierophinney · 2018-01-09T21:03:18Z

At this time, based on discussion with the working group, I've moved the paragraph in question back to the meta document's section 6.2 preamble, with changes as discussed here and elsewhere to remove implications that these are anything more than optional recommendations.

@mnapoli

Based on feedback from @mnapoli.

weierophinney force-pushed the fix/psr-15-meta-corrections branch from 7906137 to 5292adc Compare January 4, 2018 23:05

weierophinney added the PSR-15: HTTP Request Handlers label Jan 4, 2018

Crell reviewed Jan 4, 2018

View reviewed changes

Crell reviewed Jan 5, 2018

View reviewed changes

mnapoli reviewed Jan 5, 2018

View reviewed changes

shadowhand reviewed Jan 5, 2018

View reviewed changes

weierophinney added 3 commits January 5, 2018 12:04

Incorporates feedback from @mnapoli

4faa736

`s/either by/for example, by/`, with some additional punctuation.

Adds review period initiation to voting section

aca41c3

Adds section 6.3, Example Interface Interactions

3fadaa7

Added a section to demonstrate both two potential middleware dispatch systems (and thus how middleware and handlers interact), as well as examples of re-usable middleware.

weierophinney force-pushed the fix/psr-15-meta-corrections branch from 5292adc to 3fadaa7 Compare January 5, 2018 18:04

weierophinney added 6 commits January 5, 2018 12:05

Fixes typographical errors.

56aa94b

Refactors QueueResponseHandler example to use a fallback handler

79863dc

Instead of a "response prototype", we instead compose a fallback handler that is invoked with the request if the last middleware in the application delegates to the handler.

Rephrases opening paragraph of meta section 6.3

274b14c

Removes two sentences that are unnecessary, and potentially contradictory to the specification.

Refactors summary paragraph of meta section 6.2

162ea8b

Updated per verbiage from @Crell, with additional clarifications.

weierophinney commented Jan 5, 2018

View reviewed changes

shadowhand reviewed Jan 5, 2018

View reviewed changes

Crell reviewed Jan 6, 2018

View reviewed changes

weierophinney added 3 commits January 8, 2018 09:47

Updates preamble to MiddlewareInterface

1bfbee4

Based on further feedback to php-fig#996.

Remove unnecessary formatting

952ffe1

Removes emphasis from "request handler", based on feedback. Additionally, adds the word "dedicated", also per feedback.

Moves paragraph describing middleware/request handler interactions to…

a243650

… spec Per @Crell, this patch moves the paragraph describing the interactions between middleware and request handlers into the spec, under section 1.2.

weierophinney added 2 commits January 8, 2018 09:53

Use PSR-12 guidelines for all code samples

cd61148

Mainly, `): ReturnType` vs `) : ReturnType`.

shadowhand approved these changes Jan 8, 2018

View reviewed changes

Crell reviewed Jan 8, 2018

View reviewed changes

s/Decorated/Decorating/ in decoration-based middleware example

a518985

Per @Crell

samdark mentioned this pull request Jan 9, 2018

Fix #15438 PSR middleware yiisoft/yii2#15459

Closed

Rephrases sentence leading into reusable middleware guidelines

4626bf1

Based on feedback from @mnapoli.

weierophinney merged commit 4b417c9 into php-fig:master Jan 11, 2018

weierophinney deleted the fix/psr-15-meta-corrections branch January 11, 2018 20:06

[PSR-15] Provide middleware dispatch examples #996

[PSR-15] Provide middleware dispatch examples #996

Conversation

weierophinney commented Jan 4, 2018 • edited

weierophinney commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanotorresi Jan 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crell Jan 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crell commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Jan 8, 2018

shadowhand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Jan 9, 2018

weierophinney commented Jan 4, 2018 •

edited

stefanotorresi Jan 8, 2018 •

edited

Crell Jan 5, 2018 •

edited