http-interop / http-middleware

PSR-15 interfaces for HTTP Middleware
MIT License
73 stars 7 forks source link

Rename DelegateInterface::process to RequestHandlerInterface::__invoke #50

Closed schnittstabil closed 7 years ago

schnittstabil commented 7 years ago

I've encountered many problems with the current DelegateInterface and its related contract within my projects. I have thoroughly reviewed all debates about the change I'm proposing with this PR and I do not see any solid technical reason against it. This proposal is about contracts and the big picture, hence, please give it real a chance.

The new versions:

namespace Interop\Http\Middleware;

interface RequestHandlerInterface
{
    /**
     * Process an incoming server request and return the response.
     *
     * @param ServerRequestInterface $request
     *
     * @return ResponseInterface
     */
    public function __invoke(ServerRequestInterface $request);
}

interface ServerMiddlewareInterface
{
    /**
     * Process an incoming server request and return a response, optionally delegating
     * to the next request handler to create the response.
     *
     * @param ServerRequestInterface $request
     * @param RequestHandlerInterface $next
     *
     * @return ResponseInterface
     */
    public function process(ServerRequestInterface $request, RequestHandlerInterface $next);
}

I. What is wrong with DelegateInterface?

1. Middlewares cannot be reused w/o a middleware container.

I'm not the only one struggling with this problem, afaik @mindplay-dk (#37) is suffering from this too. Sometimes we want to call a single middleware, e.g. ```php $csrfMiddleware = new CsrfMiddleware(); $response = $csrfMiddleware->process($request, new class() implements DelegateInterface { public function process(ServerRequestInterface $request) { return new Response(200); } }); ``` While this is technical possible, the contracts prohibit that, they are as follows: ```php interface DelegateInterface { /** * Dispatch the next available middleware and return the response. */ public function process(ServerRequestInterface $request); } interface ServerMiddlewareInterface { /** * …, optionally delegating to the next middleware component to create the response. */ public function process(ServerRequestInterface $request, DelegateInterface $delegate); ``` The `DelegateInterface` instance above does not *dispatch the next available middleware*, the instance does not know if there is any *next available middleware*. @mindplay-dk dissented this point of view at https://github.com/http-interop/http-middleware/pull/29#issuecomment-260746270: > the role of even a simple function consisting of `return new Response()` is still _acting_ as middleware in it's relation to the middleware component that calls it. To be honest, this looks like we are forcing the world to fit the spec, instead of creating a model based on reality: ```php $d = new class implements DelegateInterface {…}; var_dump($d instanceof DelegateInterface); // => true var_dump($d instanceof ServerMiddlewareInterface); // => false ``` Hence, PHP says "no"! The class above is a delegate, not a middleware. Furthermore this point of view conflicts with the idea of *middleware container delegate* (@weierophinney at https://github.com/http-interop/http-middleware/pull/29#issuecomment-260466313): > The delegate is used to manage a stack, queue, collection, whatever, of middleware, but is not itself middleware. The idea when calling it from within middleware is to see if **some other middleware** can produce a response. [emphasis added] Therefore, to be on the safe side, we must use a stack/pipe e.g.: ```php $csrfMiddleware = new CsrfMiddleware(); $pipe = new \Equip\Dispatch\MiddlewarePipe($csrfMiddleware); // Default handler for end of pipe $default = function (RequestInterface $request) { return new Response(); }; $response = $pipe->dispatch($request, $default); ```

2. Current PSR-15 middleware containers already infringe the current contracts.

If we look at the `Equip\Dispatch\MiddlewarePipe` example above, we see a default handler, which is an anonymous function (closure), needed to dispatch the request. The [`MiddlewarePipe::dispatch`](https://github.com/equip/dispatch/blob/master/src/MiddlewarePipe.php#L57-L62) looks like this: ```php public function dispatch(ServerRequestInterface $request, callable $default) { $delegate = new Delegate($this->middleware, $default); // … } ``` Thus the default handler is wrapped into a delegate adapter. But, for the same reasons as in _I. 1._, the `$delegate` cannot fulfill the contracts. Equip Dispatch is by far not the only project doing that, all current PSR-15 frameworks (I'm aware of) wrap anonymous functions (closures) into `DelegateInterface` adapters today and infringe the contract in that way.

3. The DelegateInterface contract is too vague.

According to the phpdoc: > Dispatch the next **available** middleware and return the response. [emphasis added] However, what happens when there is no next middleware *available*? Does `$delegate->process(…)`: 1. Throws an exception? 2. Returns an empty response? 3. Returns `null`? In my opinion the middleware should not care at all. "available middleware" passes the responsibility to care about these questions to the middleware – that is obviously undesirable.

4. The DelegateInterface contract is too concrete.

According to the phpdoc: > **Dispatch** the next available **middleware** and return the response. [emphasis added] Do a middleware need to know that a *middleware* will be dispatched? Beside that this is not true in all cases (see _I. 2._), why should a middleware care? Furthermore, why should a middleware care that it deals with a delegate? From the middleware point of view, it does not matter. The only important information is "**Process a request and return the response.**". That's all, because the `ServerMiddlewareInterface` contract already states: > … **to the next** middleware component[thingy in the queue] to create the response. [emphasis added]

5. Middlewares have to be aware that they are executed within a container.

While it is true in most use cases (except the one in _I. 1._) that a middleware is executed within a container, we do not have one single use-case so far where this is important. Let's face it: almost all middleware frameworks use `callable` and are happy without that information. Hence, the knowledge about: *`$delegate` is a delegate of a container* is useless.

6. For experienced middleware developers the wording of the ServerMiddlewareInterface contract is irritating.

According to the phpdoc: > … optionally **delegating** to the next middleware component to create the response. _delegating_ is a clearly a verb and as a middleware implementer who has used multiple middleware frameworks, I expect a `callable` or _`__invokable`_ thingy if I read `$delegate` afterwards. But, surprisingly *delegate* is used as a noun at `@param DelegateInterface $delegate`. Thus we have this weird situation: ``` $delegate->process(…); | | | \------ the contract uses "delegating" (verb!) for this method | \---------------- the phpdoc uses "delegate" (noun!) for this object ```

7. Middleware delegates cannot be reused.

This one annoys me the most, because I have too many use-cases of this kind. Please note the namespace of the the proposed request handler (i.e. `Interop\Http\Middleware`). Therefore, the request handler is not an arbitrary _request handler_, it is a _**middleware** request handler_. There are many candidates of shareable _delegates_ in the world of middlewares: * Default/final handlers which throw errors. * Default/final handlers which return an empty response. * Default/final handlers which return domain specific responses. * Stacks (see #35) * … They all share the same method signature: `(ServerRequestInterface $request) : ResponseInterface`, but most of them cannot fulfill the `DelegateInterface` contract (see _I. 1._ for details). The `RequestHandlerInterface` name and the new contracts solve that and lead to interop on the _middleware delegate_ level.

II. Why RequestHandlerInterface?

Just because (almost) every PHP developer can instantly grasp the idea – in contrast to FrameInterface and DelegateInterface.

Why RequestHandlerInterface::__invoke?

1. callable does not provide enough type-safety etc.

Honestly, I'm not the right person to explain that [point of view](https://github.com/http-interop/http-middleware/issues/37#issuecomment-265300033). I've implemented sophisticated type-systems (e.g. [Linear types and non-size-increasing polynomial time computation](https://www.tcs.ifi.lmu.de/mitarbeiter/martin-hofmann/publikationen-pdfs/j17-lineartypesnonsizeincerasing.pdf)) and I've created a type-system for a DSL in my master thesis. And I can assure you, I love strictly typing and e.g. [Agda](https://github.com/agda/agda) – in theory. But, PHP is practice, which I professionally use since 2000 (v4.0.2 or so), and it seems that PHP developers cannot sleep well without a more strictly-typed version.

2. Experienced middleware developers expect a callable or at least a ___invokable_.

I hope, at least this is obvious. Almost all middlewares use a `callable` type-hint, using an `__invoke` interface relieve pain of loosing `callable` and writing cumbersome code.

3. Libraries and frameworks already supports this interface

`RequestHandlerInterface::__invoke(ServerRequestInterface $request)` is general enough to reuse it already today with non-PSR-15 compatible libraries. I've already showed that with the `Aura.Router` at https://github.com/http-interop/http-middleware/pull/39#issuecomment-263134031. In contrast, `DelegateInterface::process/dispatch/etc` forces us to write stupid Adapters, e.g. with Slim: ```php $contactStack = new ContactStack(); $app = new Slim\App(); // instead of: $app->get('/', $contactStack); // we have to write a dump adapter function: $app->get('/', function ($request) use ($contactStack) { return $contactStack->process($request); }; ``` The problem with these adapters: they provide zero value, are not type-safe and are annoying to write and test.

Why $next?

It does not make a real difference, the new `ServerMiddlewareInterface` contract: > … optionally delegating to the next request handler to create the response. Thus, we can also use: 1. `$delegate` 2. `$nextResponseHandler` 3. `$nextHandler` 4. … I don't really care, but all double-pass implementations I'm aware of, use `$next`. And I do not want to confuse them unnecessarily.

III. Arguments against ::__invoke

1. RequestHandlerInterface::__invoke does not conflict with ServerMiddlewareInterface::process

The argument is originally about the `callable` type-hint and states the following: > Making delegates callable without making middleware callable would allow people to implement both middleware and delegate interfaces in the same class, which is definitely not correct. Obviously, this applies to `__invoke` interfaces as well. But the inheritance hierarchy and interface were never created to prevent that – The opposite is the case, they provide definitions of methods which objects agree on in order to **cooperate**. If we preventing cooperation by the same name on purpose, then we abuse the type-system in my opinion. Moreover, I gave a realistic example of middlewares which can act as delegates as well at https://github.com/http-interop/http-middleware/pull/41#issuecomment-263969406. Nobody, argued against it, therefore the argument is an untenable assertion in my opion.

2. ::__invoke can be type hinted as callable

The [argument of the current META](https://github.com/http-interop/http-middleware/tree/fd47985af5ce4de141534bbc1f42978686351805#why-doesnt-middleware-use-__invoke) is originally about the `callable` middlewares and states the following: > In addition, classes that define `__invoke` can be type hinted as `callable`, which results in less strict typing. This is generally undesirable, especially when the `__invoke` method uses strict typing. On the first glance, this applies to `RequestHandlerInterface::__invoke` as well. Although, I disagree with the quote, it is not applicable here: with this proposal we do not encourage a `callable` type-hint, e.g.: ```php $next = function(ServerRequestInterface $request) {…}; var_dump($next instanceof RequestHandlerInterface); // => false ``` From the PSR point of view an anonymous function etc. is not a valid `RequestHandlerInterface` instance.

3. ::__invoke conflicts with double-pass (callable $next)

See details at https://github.com/http-interop/http-middleware/pull/39#issuecomment-265266694. The argument is in my opinion really Stratigility specific. It is about `callable` delegates in Stratigility etc. > [Simplified] With a different method name we make migration easier. This debate could already end here, because @weierophinney already stated: > These are all solvable, but they're far easier to solve when the methods *differ*. But okay, @shadowhand claimed I would [ignore arguments](https://github.com/http-interop/http-middleware/issues/43#issuecomment-266212719), lets take a look at [`Zend\Stratigility\Next`](https://github.com/zendframework/zend-stratigility/blob/b12bae3dab179d4ffb8af4c5dbfa66fbfeef3c38/src/Next.php#L24): ```php class Next implements DelegateInterface { public function __invoke(ServerRequestInterface $request, ResponseInterface $response, $err = null) {…} public function process(RequestInterface $request) {…} } ``` Obviously, using the [decorator pattern](https://en.wikipedia.org/wiki/Decorator_pattern) to _add single-pass behavior_ is based on the wrong assumption that we want/need to _add_ something. What we really want to accomplish is this: the legacy implementation should work with the new PSR-15 without modifying source code; and this can be simply done via the [adapter pattern: ](https://en.wikipedia.org/wiki/Adapter_pattern) ```php // simplified: if ($middleware instanceof ServerMiddlewareInterface) { return $middleware->process($request, new PsrAdapter(new Next(…)); } else { return $middleware($request, $response, new Next(…)); } ```

4. ::__invoke cannot be used for other means.

The argument is originally about the middlewares, but also applicable to request handlers: > However, this also prevents any implementing middleware from using `__invoke` for other means. Obviously, the same holds for `dispatch` and `process`, as long as we not choose a uuid or similar we cannot solve that: ```php // I'm pretty sure, this method name is not already taken: $delegate->f892336f82044c759f49c78d834cff3a($request); ```

5. Mocking issues with ::__invoke

> I remember issues when mocking (with prophecy) and trying to test __invokable's response/result That do not really matter to this discussion, but afaik, it is already solved: https://github.com/phpspec/prophecy/issues/205

6. The role the object plays in the application

Thanks to @weierophinney, about having different method names: > This has been listed as undesirable, […] because it makes it more confusing for a consumer to know what role the object plays in the application. `RequestHandlerInterface` makes the situation even worse: Do I want a middleware or a request handler? What are the pros and cons? In my opinion PSR-15 cannot and should not answer this question. It is about personal taste and depends on the concrete framework. For example: * **Framework A:** An action/controller/… is a request handler and we inject a PSR-17 Factory via its constructor. * **Framework B:** An action/controller/… is a middleware and response creation is done via calling `$next`.

All other arguments I've found are about callable type-hints and/or middlewares and cannot be applied to this proposal in my opinion. But maybe I missed one, then please leave a comment.


History:

schnittstabil commented 7 years ago

@shadowhand I hope I've presented my arguments concisely enough this time.

schnittstabil commented 7 years ago

@weierophinney I'm sick and tired about all your technical Stratigility issues. You can easily use the new interfaces in v2.0: https://github.com/schnittstabil/zend-stratigility/tree/request-handler-proposal

jschreuder commented 7 years ago

Not entirely sure if making your arguments collapse makes them more concise. There's some nice symbolism in making them collapse though.

More importantly, do you really think someone with intimate knowledge of PSR-7 and tons of real-world experience implementing it shouldn't raise technical issues when he sees them? Saying you're "sick and tired" of hearing arguments that go against your position probably isn't your best strategy either.

schnittstabil commented 7 years ago

More importantly, do you really think someone with intimate knowledge …shouldn't raise technical issues when he sees them?

No, and I do not want to stifle these arguments. I'm not tired of interop, migration, technical arguments at all.

But I'm tired of these long arguments like https://github.com/http-interop/http-middleware/pull/39#issuecomment-265266694, https://github.com/http-interop/http-middleware/issues/44#issuecomment-265292327, https://github.com/http-interop/http-middleware/issues/43#issuecomment-265283111 and https://github.com/http-interop/http-middleware/pull/29#issuecomment-260498459 which only about Stratigility and its migration issues.

Stratigility supports callable middlewares, and all of these Stratigility issues are because it supports callable. But if I proof that MiddlewareInterface::__invoke leads to better migration for some middleware frameworks at #43, then @weierophinney returns:

For what it's worth, … it is up to the middleware framework to decide whether or not it accepts callable middleware.

Or in summary: If @weierophinney have to change his code, then it is a tremendous problem.

schnittstabil commented 7 years ago

Not entirely sure if making your arguments collapse makes them more concise. There's some nice symbolism in making them collapse though.

Hahah, true! But @weierophinney wanted to see a list at https://github.com/http-interop/http-middleware/issues/37#issuecomment-265300033:

Having a concrete list allows everyone involved to more objectively evaluate the pros and cons of each approach.

weierophinney commented 7 years ago

@schnittstabil —

tl;dr I mostly agree with the changes proposed, with a few caveats:


First off, you're making a ton of assumptions about me and what I think, and comments such as "I'm sick and tired about all your technical Stratigility issues" or "if @weierophinney have to change his code, then it is a tremendous problem" (before I've even had a chance to comment, no less!) certainly make me less inclined to review your proposal objectively. I'd appreciate it if you'd assume I'll review with an open mind; I promise to do likewise with regards to you.

For what it's worth, I'm not worried about changing code I maintain. I'm more worried about end users needing to change their code. That will have to happen at some point, particularly since we're migrating from double-pass to single-pass. If we can come up with a specification that makes it possible for their code to exist in both paradigms, that will make me hugely happy. That is my primary goal in reviewing the specification and proposed changes to it.

As an author of a middleware library, I also have to consider migration of that library to the new specification. Yes, we can break signatures in new major versions. However, lengthy experience has shown me that doing so often results in developers either sticking with the previous major version, or switching to something else entirely. If I can make forwards-compatible changes in a new minor release, I can ease migration for end-users. Again, I'm not worried about changing my code, but making it effortless or requiring minimal effort for end users to migrate to the new specification. I'm also thinking about this in regards not just to my own users, but to existing Slim users, or Relay users, or Lumen users.

(And yes, some of this can be handled via support in implementing projects; we've already done similarly with the CallableMiddlewareWrapper in Stratigility. I just want to make sure that we don't do anything that cannot be accommodated.)

Overall, I think you make some excellent points in your proposed changes, though I do not agree with all of them. Below is some point-by-point commentary... (and no, I'm not going to collapse any of it).

What is wrong with DelegateInterface?

  1. Middlewares cannot be reused w/o a middleware container.

    Your argument appears to be with the verbiage in the contract, versus the actual signature of the interface.

    The verbiage indicates "dispatch the next available middleware and return the response," which, based on what I understand of your argument, you take as meaning the delegate must compose a stack/queue/collection of middleware.

    The signature only indicates that the method accepts a request and must return a response.

    As far as I'm concerned, the signature trumps the verbiage; an implementation only needs to ensure it follows the signature. However, if your problem is with the verbiage, I completely back changing it.

    You propose "Process an incoming server request and return the response." May I suggest:

    Process an incoming server request and return the response.
    
    How a request is processed is undefined, but could include:
    
    - Generating a default response.
    - Processing additional middleware collected in a queue or stack.
    
    Regardless of how the request is processed, this method MUST return a
    response.
  2. Current PSR-15 middleware containers already infringe the current contracts.

    I'm not following the argument you make here: how is wrapping a callable into a DelegateInterface decorator infringing the contract, exactly? How is the delegate not fulfilling the contract? You're asserting these points, but I'm missing something somewhere along the way.

    Speaking from the point of view of Stratigility, the "default handler" (which it calls a "final handler", per its roots in Connect/ExpressJs), is essentially a middleware that guaranteed to return a response, and which is called either to create the response or once no more middleware exists with which to process the request. In essence, in PSR-15 terms, it would be more akin to a delegate, per the revised contract verbiage I propose above.

    As such, I'd propose that existing projects rewrite such default/final handlers to work as delegates. Would that address your point here? Does this actually conflict with either the existing or proposed signatures?

  3. The DelegateInterface contract is too vague, and

  4. The DelegateInterface contract is too concrete.

    This is where you started to lose me. You cannot argue the contract is both too vague and too concrete.

    Regardless, as noted in my responses to both points 1 and 3, I think:

    • Renaming the interface, and
    • Changing the description of the interface

    resolve these points you make.

  5. Middlewares have to be aware that they are executed within a container.

    I really didn't understand this point at all.

    Middleware only need be aware that they are passed a delegate (or request handler) which they can call upon to receive a response. This essentially means that middleware do not need to generate a response. This situation occurs:

    • if they decide that they are ultimately unable to handle the request.
    • if they only wish to manipulate the response generated by another layer.

    Essentially, each middleware decides where it may fit within a larger application, but does so without any knowledge of the other layers.

    So, yes, middleware are typically aware that they are executed as part of a layered application, but not what layers exist or what layer they occupy within the application. The delegate or request handler is how the middleware delegates generation of a response from the current request if it is unable to do so itself. How the delegate/request handler does that is not something it needs to concern itself with.

    So, going back to the original assertion: I'm not sure that this is a problem per se. If it is, can you provide me more information, please?

  6. For experienced middleware developers the wording of the ServerMiddlewareInterface contract is irritating.

    This is clearly your opinion, and not necessarily based on empirical evidence. I had no issue with the wording as it was before. (And no, I did not author it.)

    Yes, delegating is a verb. Delegate is a noun, meaning a representative.

    In the current iteration, the DelegateInterface represents another handler capable of processing the request.

    I don't see this as misleading or irritating; it's a clear description of its purpose in the system.

    That does not mean, however, that I disagree with changing the name or the verbiage surrounding the interface, only that I disagree with this particular assertion.

  7. Middleware delegates cannot be reused.

    You assert that each of the listed potential shareable delegates cannot fulfill the DelegateInterface contract, referring to point 1; I assume you mean in terms of the verbiage, and not the signature, as each of these can fulfill the signature just fine. Again, if we change the verbiage to either remove the references to a middleware stack/queue/container/whatnot, or make that reference a suggestion, or just follow the signature in the first place this argument goes away entirely.

Why RequestHandlerInterface?

  1. Why RequestHandlerInterface::__invoke?

    The only argument I have against your points is the one that's already in the metadoc: having a different name than the one in the ServerMiddlewareInterface allows a class to define itself in such a way that it fulfills both contracts. This has been listed as undesirable, in part because it breaks the SRP, in part because it makes it more confusing for a consumer to know what role the object plays in the application.

    You bring this up more in "Arguments against ::__invoke", and I'll make more points on this later.

  2. Why $next?

    I think $next may suffer from the same issues you point out above in I.5 ("Middlewares have to be aware that they are executed within a container"); naming it $next implies a queue or a stack, versus a generic object capable of handling a request.

    As such, if we go with the proposed interface name change, $requestHandler would likely be most correct in that regard; that's also an argument for a discrete method name other than __invoke(), as $requestHandler is a noun; $requestHandler->process() == noun + verb. (If @shadowhand agrees to __invoke() in the signature, we'll need to come up with a good verb for the variable name; again, however, I think $next implies a stack or queue, which introduces some of the same problems you bring up earlier.)

Arguments against __invoke

  1. RequestHandlerInterface::__invoke does not conflict with ServerMiddlewareInterface::process.

    First, in this argument, you bring up a comment you made and the fact that nobody argued against it, which makes assertions against your point untenable. This is a horrible argument; lack of response does not imply agreement. To be quite honest, you've been so prolific in your comments that it has been almost impossible to keep up, much less respond to every comment you make. I've had to cherry pick a ton, particularly as I've had several times in the past couple months where I've had no ability to respond for a week or two at a time.

    The whole argument made in the current iteration of the proposal, and by you in this proposal, is that the interfaces created are intended to cooperate.

    The assertion made in the metadoc, as well as on list and in various discussions, is that the ability to create a class that acts as both middleware and delegate breaks the SRP and makes the role of that class within the system less clear.

Regarding your other points, I have no arguments. As I've mentioned before, having a method name other than __invoke() makes adapting existing callable $next implementations to PSR-15 easier, but it can be done regardless; I can live with whatever decision is made. The main point I have about __invoke() is around whether or not the specification wants to allow classes that act as both middleware and delegates/request handlers. That is up to @shadowhand as editor of the specification to decide.

Other notes

As noted in #47, we have naming issues in the spec currently, and the proposed name of RequestHandlerInterface is not helping any, as it is specifically handling a server request, and not any request. As such, I'd argue for naming it ServerRequestHandlerInterface (though ServerMiddlewareInterface could still name the argument $requestHandler). Additionally, if that interface implements __invoke(), we'll need to figure out a good verb for the variable name as used in ServerMiddlewareInterface.

schnittstabil commented 7 years ago

@weierophinney Thanks for acting professionally. My intention was to keep the focus on all (possible) middleware frameworks, not on Stratigility alone. I've (re)read the code of Stratigility, Middleman and Equip\Dispatch and moreover I have reread all comments around here just to make sure that this is doable, which gave me that unfortunate impression – but getting personal was not fair by me to you.

As far as I'm concerned, the signature trumps the verbiage;

To me the verbiage and the signature together define the contract, which I have to rely on and which give me assurance. I have no concerns with a more verbose verbiage, but I believe doing too much does not help either. Thus, the only problem with your suggestion I see is the MUST return a response part, as I'm aware of final handlers which throw an exception to signal a misconfigured stack.

how is wrapping a callable into a DelegateInterface decorator infringing the contract, exactly?

Almost all of my points are about the verbiage. (I hope) every time I've used the word contract, I write about the verbiage. In this context the callable does not dispatch the next available middleware and therefore do not fulfill the contract.

You cannot argue the contract is both too vague and too concrete.

That was a trick to gain attention :wink: And I believe I can: in my opinion some parts are too vague (what does next available mean to the current middleware?) and some parts are too concrete (why do I have to know that a middleware will be dispatched?). – But, yes, these points would be resolved.

  1. Middlewares have to be aware that they are executed within a container.

I really didn't understand this point at all. Middleware only need be aware that they are passed a delegate (or request handler) which they can call upon to receive a response.

That is exactly what I wanted to express. But currently, the server middleware contract includes: optionally delegating to the next middleware component – Hence, as middleware implementer I must assume that this is anyhow important to me.

  1. For experienced middleware developers the wording of the ServerMiddlewareInterface contract is irritating.

This is clearly your opinion, and not necessarily based on empirical evidence. I had no issue with the wording as it was before.

Absolutely, I should have rethought that argument before posting, but I would ask you to bear in mind that I'm (obviously) not a native speaker, and the change of meaning form delegate (verb) to delegate (noun) is not such obvious to me. Especially because the delegating method is called process.

  1. Middleware delegates cannot be reused.

I assume you mean in terms of the verbiage

Exactly.

nobody argued against it, which makes assertions against your point untenable. This is a horrible argument; lack of response does not imply agreement.

I fully agree, but now I know that I did not changed your point of view by my example. Sorry, if I tricked you to give me that information. Anyway, I believe we just disagree about what responsibility in SRP means and I do not believe we will find an agreement on this. In any case, I refuse to believe that PHP developers are so stupid and implementing both interfaces just because this package provides two interfaces. Therefore I do not think we must protect them from themselves.

we'll need to come up with a good verb for the variable name;

I do not like $next either. In an OO world it should be a verb. But to me it does not necessarily imply a stack or queue – but I'm neither a native speaker nor a linguist/etymologist.

I'm also fine with ServerRequestHandlerInterface, a bit long but more precise.

mindplay-dk commented 7 years ago

Just a note regarding RequestHandlerInterface - besides the method-name debate (which I'm not getting involved in again) very early in the process, I proposed this interface be named Controller or Dispatchable, and suggested it be an entirely separate PSR.

Why? Because "takes a request and produces a response" is an extremely generic thing - it's the simplest definition of a controller you can have, and it isn't by any means middleware-specific.

I'm still of the opinion that this interface would be much better off in a separate PSR, at which point the interface and method-name becomes a non-discussion in the context of middleware.

I think at least part of the the problem that ignites the discussion about the interface and method-name is simply the fact that it's not really middleware-specific at all, and so it's naturally difficult to come up with suitable names for use in a middleware-standard.

Just my two cents.

mindplay-dk commented 7 years ago

You cannot argue the contract is both too vague and too concrete.

This is kind of the subject I'm getting at with my last comment - basically any name or definition you can come up with is going to be either too vague in a middleware-context, or too specific outside of a middleware-context.

I believe that's a symptom of this interface being misplaced as belonging explicitly in a middleware-context.

If the interface is defined as part of the middleware-spec, it's role can't, in my opinion, be defined as anything more general than a delegate - it's role, if it belongs to a middleware-specification, is for that purpose and that purpose only, otherwise it's out of scope.

On the other hand, if you define it as a more general interface (which I believe it is) then it should have a very general description like request-handler, controller or dispatchable - but then it doesn't belong in a middleware-spec, because then it's not middleware-specific.

It seems you want it to be very generic (too vague) but you want it to also fit into a proposal about middleware (too specific) - just my opinion, but it seems to me you can't have your cake and eat it too ;-)

shadowhand commented 7 years ago

I have not had time to review this and the comments. Give me a couple more days. 😄

DaGhostman commented 7 years ago

Honestly I think that this kind of discussions went on for pretty long and it is starting on to repeat itself (the naming of everything was discussed for quite some time in the past).

With that being said, I will ignore the naming part, since it is not that much important at the moment. Why is the (currently named) DelegateInterface so problematic, it has to return a response, that is clear and honestly I don't see anything wrong for a possible implementation to have a constructor signature of:

public function __construct(array $middleware, Psr\Http\Message\ResponseInterface $baseResponse) // ...

And when there is no other middleware, just return whatever was passed as $baseResponse and there problem solved (this is assuming it should not throw an exception, nor it be allowed to return null), even this sits better with me as I consider new to be a code smell (yeah classes have to be instantiated, but not inside the controller or some random middleware), also keep in mind I am not implying a specific signature to the constructor, that was an example.

As @schnittstabil has said and I really think it is a bit out of scope, about someone changing their code, well, at the end of the day there will be someone changing something of their own since there is no way to make everyone happy. I will refer again to the PSR7 discussions, where many people ware against it, but it passed as it is just because that was more logical and once you get used to it, it really seems the logical way + makes more sense. Everyone can migrate when they bump to their next major release so the discussion about projects which use callable is really off the table, all of those projects ware pre-PSR15 so it is normal for them not to comply, heck there are major frameworks which do not use PSR7, but I don't see people asking for a change in the interfaces to meet some project's preferences.


Lets assume people would expect a callable/__invoke approach to the problem. How does this sit with PHP and PHP only? As said in the meta, it allows for mixing the interfaces, callable is too broad in the context of PHP (arrays, static, functions, __invoke) which is bad, even for the fact that some devs will prefer arrays with class-method entries since they use a single "controller" with multiple actions, others will stick with functions because its simple/come from other languages' background/find it prettier and when you inherit a project, although it complies with the standard, there is no way of knowing what exactly you'll find and makes PSR15 not-so-standard-standard.

On Stratigility breakage, I see a fair point about it since it is relatively recent and is like a 'reboot' to all other frameworks so it makes sense to not break it as much as possible, unlike other older frameworks (Symfony, Laravel, etc, I am not pointing fingers) they decided to ignore PSR7 and add support for it with more code, instead of just including adopting it since most have had a major release since PSR7 got accepted

So to sum it up, lets not think about projects specifically, but rather focus on the greater good - future projects/versions of frameworks, what is rational and what actually makes sense and get things moving. The sooner it passes the sooner will the adoption start, just my 2c..

schnittstabil commented 7 years ago

@DaGhostman It might appear that I'm only bikeshedding. Obviously, there is no technical difference between the DelegateInterface name and the RequestHandlerInterface name. But I'm talking about contracts, similar to the meaning as in _design by contract_.

My intention was to present all arguments for and against this proposal, as far as objectively possible, which necessarily includes some repetitions. The list got a bit long, but as @weierophinney mentioned, a concrete list allows everyone involved to more objectively evaluate the pros and cons of each approach.

I've thoroughly reviewed all callable/__invoke arguments and we always ended in debates about callable/__invoke middlewares. In my opinion, your arguments going into the same direction. Hence, please note: I neither propose ServerMiddlewareInterface::__invoke nor callable middlewares nor callable delegates/request-handlers. See also III 2..

Everyone can migrate when they bump to their next major release so the discussion about projects which use callable is really off the table, all of those projects ware pre-PSR15 so it is normal for them not to comply.

I believe you think that my arguments targets only middleware/micro frameworks etc. The Slim example was one which shows that with this proposal the migration to PSR-15 becomes more softly. It is also applicable to Stratigility and to post-PSR-15 projects.

Moreover, it seems you believe that projects like Aura.Router will implement the PSR-15 standard. In my opinion that is an erroneous assumption. At first that would violate SRP on the project level. And secondly, it means that the project becomes less useful/attractive for non-PSR-15 frameworks. The final result will be, that someone else will create an aura-router-middleware package, which is just a dump process-adapter and which will sooner or later suffer from versioning problems which naturally accompanies that approach.

weierophinney commented 7 years ago

I'm still of the opinion that this interface would be much better off in a separate PSR, at which point the interface and method-name becomes a non-discussion in the context of middleware.

We had this same situation with PSR-7 with regards to both streams and URIs. The problem with separating those out into another PSR means that we then push back both work on and acceptance of this standard by a matter of several months. When we considered that fact for PSR-7, the consensus was that while it would be ideal, it's also a change that can be made in the future: get the original specification complete, then work on the foundational ones, and later do a specification to supercede the original that depends on the new foundation specs. I'd argue the same here; the longer we delay this, the less likely we'll get interested implementors.

mindplay-dk commented 7 years ago

We had this same situation with PSR-7 with regards to both streams and URIs ... later do a specification to supercede the original that depends on the new foundation specs

Unfortunately, as we've seen with PSR-7, that never actually happens - streams and URIs are covered well enough for their purposes in PSR-7, and the effect of this is that no one cares enough to do it.

I've needed to use both streams and URIs outside of a PSR-7 context, didn't want the dependency on an HTTP abstraction to get it, and ended up making or using something that wasn't interoperable.

I'm sure you've been there?

The same thing will happen with this interface and PSR-15.

The problem is that, once these things are established, it is extremely difficult to convince anyone of the value of making a breaking change.

If we commit to something, and it succeeds community-wide, for the next decade or so, you can essentially count on it being immutable as far as any breaking changes go.

The problem with separating those out into another PSR means that we then push back both work on and acceptance of this standard by a matter of several months

I believe that's true for things like URIs and streams because they're complex.

This interface is very, very simple: takes a Request, returns a Response. There really isn't much to debate or even bikeshed about - PSR-7 has already standardized the difficult parts.

I get it, nobody wants to delay this, nobody wants to do more work, I'm just not sure we're helping ourselves in the long run...

weierophinney commented 7 years ago

If we commit to something, and it succeeds community-wide, for the next decade or so

I disagree there. PSR-0 had only a 3 or 4 year run before it was deprecated by PSR-4. None of the standards later than PSR-2 are older than 3 years. I don't think we can make that assumption at this time.

I believe that's true for things like URIs and streams because they're complex.

This interface is very, very simple: takes a Request, returns a Response. There really isn't much to debate

I'm not worried about bikeshedding and debate — the process alone, however, generally eats up a minimum of 2 months, and, realistically, I don't see it moving through in less than 3, particularly as any proposal we want to raise at this point also needs to go through a WG and have at least 2 implementations. Sure, it's relatively simple; the process duration is what's painful here, particularly as it's already been over a year that this proposal has been in the works.

Again, I think this is up to @shadowhand to decide. I'm okay with either direction, but wanted to point out the precedent of other existing specifications as well, and the rationale.

schnittstabil commented 7 years ago

I believe there is always enough space for debates :worried: Some questions I believe I can foresee:

To me, the __invoke signature and the (Server)RequestHandlerInterface name/contract have proofed its usefulness in the realm of server-side middlewares (e.g. Stacks/Dispatcher, pre-PSR-15 middleware frameworks and non-PSR-15 libraries), which justifies the existence in the Interop\Http\Middleware namespace.

Hence, lets assume a possible new PSR interface will exactly look like the RequestHandlerInterface:

namespace Interop\Http\Foo;

interface FooInterface
{
    public function __invoke(ServerRequestInterface $request);
}

From the technical point of view that would not be a problem, the interfaces would be compatible:

class Bar implements FooInterface, RequestHandlerInterface
{
    public function __invoke(ServerRequestInterface $request) {…}
}
mindplay-dk commented 7 years ago

PSR-0 had only a 3 or 4 year run before it was deprecated by PSR-4.

That was not really disruptive in any way - it wasn't really even a breaking change, at least not in code terms, mostly just required people to reorganize their source-files a bit.

Specifications that include interfaces are much harder to change, since those interfaces are not just specifications, they're dependencies. And this isn't JavaScript, where you can just load two libraries using different versions of a dependency and run them side-by-side.

Both PSR-0 and PSR-4 are very simple. This is not. (and I realize the interfaces themselves are fairly simple, but there is a lot of inherent complexity due to the interplay with PSR-7.)

Once this thing is in the water, it'll be much, much harder to change course.

Not only that, but the motivation will be extremely hard to find - either the difference has to be really, really substantial, or the change has to have a relatively minimal impact, otherwise people will be strongly inclined to just stick with the devil you know.

Anyways, that's just my opinion. If I was to follow my own inclinations, I'd actually like to go all the way back to PSR-7 and fix that first, but, like I said, I fully understand why you want to move ahead with this.

schnittstabil commented 7 years ago

@mindplay-dk I fully agree, a PSR for the (Server)RequestHandlerInterface (and/or similar) would be awesome. But the PSR-process does not necessarily result in an interface which fits the needs of server-side middlewares best; hence, I'm on the fence about your suggestion.

shadowhand commented 7 years ago

I see no reason to adopt this change, which boils down to two points:

  1. Changing the delegate process() method to __invoke(), which I have already stated I will not do before the PSR goes to review. This argument can be made again during the review process and I will debate it there.
  2. Changing DelegateInterface to ServerRequestHandlerInterface, which is a new idea. I do not think this is useful because:
    • it is much more wordy
    • it implies the delegate is going to "handle" the request, which it is not (middleware does)
    • the interface doesn't need ServerRequest in the name, because it is already in the namespace

The name "delegate" is a perfectly acceptable name for what this interface does:

Delegates processing of the (server) request to the next available (server) middleware.

I see nothing else in here that hasn't already been discussed elsewhere.

mindplay-dk commented 7 years ago

Given that nobody wants to start a separate PSR, I agree with closing this and leaving as-is - so this interface belongs to the middleware domain, in which the name DelegateInterface is more accurate than e.g. RequestHandlerInterface which doesn't describe the role of this interface in the middleware domain.

Sorry, @schnittstabil - I'd be on your side on this one, if the interface belonged to a separate specification. Since it belongs to the middleware-domain, it needs to be named according to it's role in relation to middleware, which is a delegate.

schnittstabil commented 7 years ago

it implies the delegate is going to "handle" the request, which it is not (middleware does)

Of course it does, from etymonline.com:

handle (v.) […], "touch with the hands, hold in the hands, […]" also "to deal with, treat, manhandle," […]

The exact opposite is the case, process does not fit well, from etymonline.com:

process (v1,and n.) […]sense of "continuous series of actions meant to accomplish some result" (the main modern sense)

Due to the fact, that the delegate does not perform actions against the request itself; but instead hand the request over to the next middleware; handle fits much better imo.


The name "delegate" is a perfectly acceptable name for what this interface does:

Delegates processing of the (server) request to the next available (server) middleware.

@shadowhand you've never ever provided any evidence that the name and/or the contract gives us any benefit. On the other hand I've presented a list of concrete issues, why both is not perfectly acceptable. And I give you another example (see here):

class Stack implements DelegateInterface
{
    protected $index;
    protected $core;
    protected $middlewares;

    public function __construct(DelegateInterface $core, MiddlewareInterface ...$middlewares)
    {
        $this->core = $core;
        $this->middlewares = $middlewares;
        $this->index = count($middlewares);
    }

    public function process(ServerRequestInterface $request)
    {
        if ($this->index === 0) {
            return $this->core->process($request);
        }

        --$this->index;

        try {
            $topMiddleware = $this->middlewares[$this->index];

            return $topMiddleware->process($request, $this);
        } finally {
            ++$this->index;
        }
    }
}

It is the most efficient (non-thread-safe) stack implementation I'm aware of. Hence, @shadowhand please show me:

  1. Where is the Delegate? (noun; the representative of the container)
  2. Why is this information useful for a middleware implementer?
shadowhand commented 7 years ago

@schnittstabil You are choosing to make the stack be nest-able, which makes it less obvious where the delegate is, because the stack also acts as the delegate. (There are also a number of issues with your example that are not relevant to the discussion.)

The MiddlewarePipe from equip/dispatch is what I would consider to be "standard" usage of delegates and much more clearly shows the separation of concerns.

mindplay-dk commented 7 years ago

the delegate does not perform actions against the request itself

@schnittstabil you're trying to describe how it's implemented, not what it does - the desired action and effective result of calling process() is the request gets processed; yes, a delegate happens to delegate the work to a different component, but the method is correctly named after the intent to process the request, everything else is details.

You're quoting the noun definition of "process" - this being an action, the word "process" refers to the verb, the definition of which is:

perform a series of mechanical or chemical operations on (something) in order to change or preserve it

In the abstract, we're performing a serious of operations on a request in order to turn it into a response.

It's not named after "a process" or "the process", but from the action and intent "to process".

schnittstabil commented 7 years ago

@mindplay-dk Of course, handle a request and process a request are both valid expressions and can usually used interchangeably; and yes, I've quoted the noun definition, but only because the verb definition is referring to it – thus this was not by mistake.

But I do not understand your argumentation. As far as I can see, your (new) definition and your explanation of it supports mine: As middlewares perform operations on requests to turn it into responses, MiddlewareInterface::process is named correctly – no doubt. But as delegates do not perform operations on requests, DelegateInterface::handle would be much more accurate.

Taking this into account and summing it up, I claim:

  1. If I process a request, then this implies that I also handle that request.
  2. But if I handle a request, then this does not necessarily implies that I (by myself) also process that request.
shadowhand commented 7 years ago

If it was only about the technicality of the method name, then handle or next or pass would all be fine. However, the part of the reason it is process and not __invoke is:

https://github.com/http-interop/http-middleware/blob/master/README.md#why-does-the-delegate-conflict-with-middleware

In my opinion this is bike shedding and I'm not interested in continuing to engage with it.

schnittstabil commented 7 years ago

@shadowhand You've started that discussion by claiming:

it implies the delegate is going to "handle" the request, which it is not (middleware does)

As far as I can see, etymologists and dictionaries proof you wrong, and moreover handle works for both – that's all.