Practical Discussion about callable Delegates

[mindplay-dk]

To put issue #37 in a more practical context, I have started this discussion branch, along with a matching middleman discussion-branch which has been updated to match those changes.

Per the commit-log so far, I have made two changes:

in ServerMiddlewareInterface, allow callable as $delegate in middleware-signature, using a static type-hint of callable. In DelegateInterface, using the method-name __invoke() then becomes necessary, so this has been changed.

since the removal of MiddlewareInterface, we decided to use a type-hint of RequestInterface in DelegateInterface, for future compatibility with non-server-middleware - I think there is no longer any reason to exclude MiddlewareInterface from the standard. This will also align better with the de-facto standard.

Per the test-suite, this all works as expected - it's simpler than previously, and inspections in Php Storm seem to make sense.

The only thing I would do differently, if it was possible, is have ServerMiddlewareInterface extend MiddlewareInterface, but we've explored that, and it's not really possible. Having to select and implement either one or the other appears to make sense though - your middleware will effectively support either RequestInterface|ServerRequestInterface for client/server-middleware, or ServerRequestInterface for server-only middleware.

[schnittstabil]

To be honest, I think you're making the same mistake as I've made the other day. It looks like you want to discuss two different things here: callable and a revival of MiddlewareInterface – Would you mind to divide those? I think they are mostly independent.

[mindplay-dk]

It looks like you want to discuss two different things here: callable and a revival of MiddlewareInterface – Would you mind to divide those?

The thing is that the removal of MiddlewareInterface was merged and incorrectly tagged as minor - I did not downgrade middleman to remove support for non-server-middleware, because that's not only a breaking change, but the removal of a feature. I don't wish to do that now, just to have to introduce it again later. I don't have faith that this will be a permanent decision, which is why I locked my version constraint to the previous release.

Let's keep the discussion to callable delegates, and if we can reach a conclusion on that, we can take the discussion about non-server-middleware after - I'll adjust the PR as needed then.

[atanvarno69]

Ignoring non-server-middleware entirely, this breaks down into two changes:

On callable delegates, i.e. the method name in DelegateInterface being __invoke():

• I am not sure particularly what concrete benefit it has over the current process(), but have no particular objection. I am equally happy to write $response = $delegate->process($request); as $response = $delegate($request);.

• Note that with DelegateInterface using __invoke() it is possible for a class to implement both DelegateInterface and ServerMiddlewareInterface. I am fine with this, but from reading the meta I understand there was deliberate decision to prevent this, which this change would undo. I am also fine with that, but think the meta should be updated as well, see later.

On ServerMiddlewareInterface's method type hinting agaisnt callable:

• I am strongly against this. A good summary why this is bad is provided by @DaGhostman here, third paragraph onwards and more generally throughout the thread. Briefly: type hinting callable isn't exactly SOLID, which is odd for something targeted OO frameworks (functional programming is all well and good for me where it supplements OO principles not undermines them) and other reasons.

• Not having ServerMiddlewareInterface type hinted in code outside of docblocks looks and feels strange to me. While I accept the interface is not rendered entirely useless and redundant, I do think it reduces the usefulness of having an interface. Again, it looks and feels strange, not better.

• From the current meta:

  Why isn't the delegate a callable?

  Using an interface type hint improves runtime safety and IDE support.

  In addition, it allows compatibility with existing middleware that already defines an __invoke method.

  See "discussion of FrameInterface" in relevant links for additional information.

  • I take it we no longer care about (the admitadly while we live with PHP 5.6 support) perceived runtime safety or IDE support?

  • Likewise, existing middleware that already defines an __invoke method?

  • Will a rebuttal of the conclusions from the referenced discussion be included in the inevitable new meta document?

Ideally, I would like to see an updated meta included in the PR, summarising succinctly the rationale for both the changes, (I think I have most of it from reading various issue and PR threads, but a summary without the back-and-forth would be really useful, especially for someone just now jumping into this) particularly the sections #why-does-the-delegate-conflict-with-middleware and #why-isnt-the-delegate-a-callable, but also any other bits that are impacted.

Finally, a general question for my own curiosity not directly tied to the PR at hand: will the updated meta only contain the final position or will it summarise the whole decision history (i.e. including the previous -- that is current as I write this -- position and why it was changed)?

[schnittstabil]

As already mentioned at https://github.com/http-interop/http-middleware/issues/37#issuecomment-263899376: PSR is an abbreviation of PHP Standards Recommendation. And if we stick to prior art, e.g. PSR-7, then we see that it is full of key words like 'SHOULD', 'SHOULD NOT', 'RECOMMENDED' and 'OPTIONAL' (not every time capitalized).

Furthermore, if we take a look at the current PSR-16 (Simple Cache) Draft, then we see that they type-hint against mixed etc.

One example:

/**
 * …
 * @param null|int|DateInterval $ttl   Optional. The TTL value of this item. If no value is sent and
 *                                     the driver supports TTL then the library may set a default value
 *                                     for it or let the driver take care of that.
 */
public function set($key, $value, $ttl = null);

We can also take a look at the accepted PSR-6 (Caching Interface), they also use union types, e.g @return array|\Traversable.

Thus, I'm open for callable – it is obviously better than mixed, and we may 'RECOMMENDED' or whatever…

[schnittstabil]

I am fine with this, but from reading the meta I understand there was deliberate decision to prevent this, which this change would undo.

The recursion argument is totally flawed:

If a middleware was used as a delegate the entire middleware stack would end up recursive, instead of piped.

One must really mess his setup to get in an endless recursive loop, and only with help of a bugged middleware container, moreover you cannot prevent that by naming them the same – Furthermore, it is okay to reuse an middleware – it might me not very useful today, but why should we prevent that? In my Cormy OnionTest I reuse a middleware – okay it utilizes Generator, but it shows that I do not end up in endless recursion – I cannot present you a PSR-15 test, so I hope it is okay …

Stupid me: In my StackTest and PipeTest I reuse middlewares, both show that I do not end up in endless recursion.

[schnittstabil]

Why isn't the delegate a callable? Using an interface type hint improves runtime safety and IDE support.

As far as I can see, that is the only valid argument against callable. The mentioned PHP-FIG discussion of FrameInterface considers PHP7 return types etc. – which we cannot enforced by this PSR.

As far as I know, the backwards compatibility and __invoke argument is about middlewares, and not delegates – furthermore I think it is flawed too.

Thus I think, the only practical benefit of the interface over callable are the enforcement of the amount of parameters and there type.

[DaGhostman]

@schnittstabil

We can also take a look at the accepted PSR-6 (Caching Interface), they also use union types, e.g @return array|\Traversable.

That being true, it is not the same as the situation with this PSR, since there they are both traversable that typehint will not affect the usage of the result, whereas callable is broader than that as I mentioned in the other PR thread. In PSR6 the type-hint is against the return type, which is fine, since the developer using the implementation has to care about what is being returned + it can safely be ignored since a PHP7 implementation might use the return types and do : \Traversable, but in any way the implementation does not have to care about what it receives.

---

@mindplay-dk Honestly I am kinda happy with the approach the delegate being a callable, but @atanvarno69 is making a good point, more specifically:

Note that with DelegateInterface using __invoke() it is possible for a class to implement both DelegateInterface and ServerMiddlewareInterface

Which is applicable to MiddlewareInterface as well, if we actually return it.

---

Has something like All about middleware been discuessed, more specifically the 3rd approach in section But What About Dependency Inversion???, which will obviously make this interface dependant on PSR-17?

[schnittstabil]

Note that with DelegateInterface using __invoke() it is possible for a class to implement both DelegateInterface and ServerMiddlewareInterface

Personally, I have no problem with that – to me it would be a feature. Let me explain why.

If we think of a middleware pipe, then we have to decide what the final handler should do. With PSR-17 in mind, it would be reasonable that the final handler will throw a miss-configured pipe exception. Thus if you forget to add at least one action/controller/whatever middleware which creates a virgin response then the error bubbles up and the top most middleware can handle it (e.g. 500 and logging etc.)

Of course, that is only an implementation detail, nothing which should end in the PSR. But I hope it shows, that there are valid cases where middlewares effectively ignore the delegate.

To me those can act as delegates as well. Even if we stick to the current delegate contract:

class ActionMiddleware implements ServerMiddlewareInterface
{
    /**
     * Dispatch the next available middleware and return the response.
     *
     * @param RequestInterface $request
     *
     * @return ResponseInterface
     */
    public function __invoke(RequestInterface $request)
    {
        return $this->dispatch($request);
    }

    public function process(RequestInterface $request, callable $delegate)
    {
        return $this->dispatch($request);
    }

    public function dispatch(RequestInterface $request)
    {
        // … use a PSR-17 factory …
       return $response;
    }
}

[schnittstabil]

About the All about middleware idea:

class MyMiddleware implements Middleware {
    public function handle(RequestInterface $request, DelegateInterface $delegate) {
        return $delegate->factory()->createResponse(404);
    }
}

Personally, I see no need for that. Moreover I think it is bad idea. For example, what happens if middleware A needs a different (domain specific) factory than middleware B?

I think it would be bad, if developers start to create 2 container to achieve that: one for the A and one for the B and then stack those container in some way or another – I believe most of them will use constructor injection instead, manually or with a DI framework – at least I hope so.

[schnittstabil]

To me, the type-hint examples shows that the PSR-16 and PSR-6 guys believe that supporting multiple types is more useful than preserve type-safety:

// @param null|int|DateInterval
// @return array|\Traversable.

I do not believe that was an easy decision, because neither PHP nor a MessDetector will help consumers. I agree, a concrete PHP-7 implementation may type-hint its return type. But the middleware process interface look like this:

public function process(RequestInterface $request, DelegateInterface $delegate);

And as we cannot type-hint the return value of DelegateInterface, it does not help middleware implementers if a concrete container provides a return type-hint.

[atanvarno69]

And as we cannot type-hint the return value of DelegateInterface, it does not help middleware implementers if a concrete container provides a return type-hint.

While this is true currently, it won't be in future. I noted here:

However, in just over two years PHP 5.6 will no longer be supported. At which point, this PSR could (should?) be updated (to version 2.0.0) or superseded (by a PSR with a new number - I am not sure which way FIG would do this) to use return types.

I think this spec should be written with that update in mind: it will be possible to have the type safety of return types, so we should not produce a spec now that will preclude a backward compatible revision later. Type safety is fantastic and we can have it, just not yet. Type hinting callable means we can never have it.

[schnittstabil]

@atanvarno69 Granted, we should keep future type safety in mind, but who knows what the future holds? The only thing I know is, that the namespace of PSR-51 will differ from the PSR-15 one.

An example, PHP RFC: Generic Types and Functions (Draft):

Generic Closures TODO describe callable<T, …> type-hints and/or generic Closure<T, …> and/or Function<T, …> types

I do not believe it will be accepted – but who knows?

[DaGhostman]

@schnittstabil you are correct, we have no idea what will happen in a while, but I think you will agree that at the moment, we do not have a standard about this at all and this PSR has been quite controversial - imagine the consequences if we don't get it right, what will happen when we have to introduce a BC brake (as atanvarno69 points out about using callable). Also if this gets to a point where the majority of the community and major frameworks are happy with it, there might not be a need for a change, every modern implementation at the time will type-hint and these interfaces will still hold.

[schnittstabil]

we do not have a standard about this at all

As far as I can see, the only standard was made by industry and most (if not all) of them use some kind of function interface, probably because:

1. they do not care about type-safety (e.g. JavaScript, but also Slim etc.) or
2. they can provide return type-safety (e.g. Java: Function<T,R>)

Unfortunately, we are caught between those two stools.

– Of course, I'm talking about our kind of middleware concept, there are several other ideas around.

[schnittstabil]

I just fiddled a bit around, and reviewed our @param possibilities (PSR-5: PHPDoc).

I know, Closure is not a collection-type, and I do not feel very well with the following:

interface ServerMiddlewareInterface
{
    /**
     * …
     * @param \Closure<RequestInterface> $delegate
     * …
     */
    public function process(ServerRequestInterface $request, \Closure $delegate);
}

But can somebody give me a hint, why the following does not work:

$delgate = function (ServerRequestInterface $request):ResponseInterface {
    return new \Zend\Diactoros\Response();
};

var_dump($delgate instanceof \Closure);
// => true

$response = $unicornMiddleware(new \Zend\Diactoros\ServerRequest(), $delgate);
// => Uncaught Error: Function name must be a string in …

I bring this up here, because maybe \Closure<RequestInterface> is already supported by QA or IDE tools, or maybe they will/want to support this anyway. Furthermore, with PHP7 return type-hints, PSR-5 is already a bit outdated…

[mindplay-dk]

First, let me clarify something.

You may get the impression that these are two independent changes, which is false.

Per the commit-log:

in ServerMiddlewareInterface, allow callable as $delegate in middleware-signature, using a static type-hint of callable.

That's one thing, but it's inseparable from the other change:

in DelegateInterface, using the method-name __invoke() then becomes necessary, so this has been changed.

Consider this example, per the current state of the PSR:

class DoNothingMiddleware implements ServerMiddlewareInterface
{
    public function __invoke(ServerRequestInterface $request, DelegateInterface $delegate)
    {
        return $delegate->process($request);
    }
}

Now, if we change the $delegate doc-block type-hint to callable|DelegateInterface, without changing the method-name to __invoke(), you'd have to remove the static type-hint entirely, which leads to:

class DoNothingMiddleware implements ServerMiddlewareInterface
{
    public function __invoke(ServerRequestInterface $request, delegate)
    {
        return $delegate instanceof DelegateInterface
            ? $delegate->process($request)
            : $delegate($request); // assume callable
    }
}

Burdening every middleware implementation with manual run-time type-checking is error-prone and, in my opinion, unacceptable.

Change the method-name to __invoke() and you can type-hint as callable|DelegateInterface in the doc-block, and statically as callable, because that makes an implementation of DelegateInterface also a callable:

class DoNothingMiddleware implements ServerMiddlewareInterface
{
    public function __invoke(ServerRequestInterface $request, callable $delegate)
    {
        return $delegate($request);
    }
}

This does give you static type-checking via doc-blocks.

It does not give you run-time type-checking for arguments passed to the delegate, but this is easily addressed by mandating middleware-stacks perform the type-check - that's no great burden, and besides, every middleware-stack will likely support different things; middleman, for one, will continue to support callable middleware, some form of non-server-middleware (if this is ultimately removed from the PSR) as well as (since you guys talk of inversion of control) integration with container-interop, e.g. "middleware" that is just strings, e.g. component names that get resolved by a DI container while traversing the stack.

But it seems we can ping-pong on this issue forever and get nowhere closer to a decision.

I think we've laid all the pros and cons on the table.

How do we close this issue?

[atanvarno69]

How do we close this issue?

The sponsors ultimately have the responsibility for the PSR, and selling it to FIG. Since this is a controversial issue, they should be the ones to decide since they are the ones who will ultimately have to defend/sell the decision.

[mindplay-dk]

Since this is a controversial issue, they should be the ones to decide since they are the ones who will ultimately have to defend/sell the decision.

Yeah, good point.

Well, people use callable today with de-facto standard PSR-7 middleware, so will it be easier to justify a change on this point, or keeping it as-is?

[schnittstabil]

Thanks to @mindplay-dk, we should remember that frameworks will likely support multiple middleware interfaces, e.g. a very strictly type-hinted:

interface Delegate7Interface
{
    public function __invoke(ServerRequestInterface $request):Response;
}

interface Middleware7Interface
{
    public function process(ServerRequestInterface $request, Delegate7Interface $delegate):Response;
}

Thus their audience benefits from PHP7 typing, but as they go with __invoke they can easily use their delegates with callable delegate Psr-15 middlewares – If we stick to the interface process they must write a proxy to support Psr-15.

@atanvarno69 I hope that this lowers your resistance against callable because of your future concerns.

[schnittstabil]

@atanvarno69 and @DaGhostman: I believe we must agree to disagree on how good code should look like.

But I hope the following is something which we can all live with:

interface ServerMiddlewareInterface
{
    /**
     * …
     *
     * @param callable|DelegateInterface $delegate  If a callable is given, its signature MUST be
     *                                              compatible with DelegateInterface::__invoke.
     *                                              Using DelegateInterface instances is
     *                                              RECOMMENDED.
     * …
     */
    public function process(ServerRequestInterface $request, callable $delegate);
}

About MUST and RECOMMENDED:

1. MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification.

2. SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course.

Of course, the wording is not 100% perefect, but:

// because of MUST, the following produces a good stack trace:
class M42 implements ServerMiddlewareInterface
{
    public function process(ServerRequestInterface $request, callable $delegate)
    {
        return $delegate(42); // <-- FATAL ERROR in this line
    }
}

// because of MUST, the following is out of the spec – the behavior is not defined:
$middleware($request, function ($request) {
    return new Response();
});

// because of RECOMMENDED, the following is okay, but we do not encourage these:
$middleware($request, function (RequestInterface $request) {
    return new Response();
});
$middleware($request, function (RequestInterface $request):ResponseInterface {
    return new Response();
});

// because of RECOMMENDED we instead encourage these:
$middleware($request, new class implements DelegateInterface {
   function __invoke(RequestInterface $request)
   {
      return new Response();
   }
});
$middleware($request, new class implements DelegateInterface {
   function __invoke(RequestInterface $request):ResponseInterface
   {
      return new Response();
   }
});

[shadowhand]

I am rejecting this PR for the same reason I rejected #37. Refer my closing comments there.