DelegateInterface and RequestInterface

[schnittstabil]

Today, I've tried to implement what @weierophinney has written at https://github.com/http-interop/http-middleware/pull/29#issuecomment-261097279:

You'll also get the fatal error if DelegateInterface invokes a ServerMiddlewareInterface instance with only a RequestInterface. Further, your stack trace will help you know what called the delegate, and thus which middleware is improperly creating a non-server request prior to calling the delegate.

Using the looser RequestInterface on the delegate, which should typically only pass on the given request to the middleware to which it delegates, allows us to have middleware with different signatures at a later point. Restricting to ServerRequestInterface denies that possibility.

At First, a quick reminder. I've suggested at #29:

 interface DelegateInterface
 {
-     public function process(RequestInterface $request);
+     public function process(ServerRequestInterface $request);
 }

About the stack trace

I've tried the following with Equip\Dispatch\MiddlewarePipe and middlewares/geolocation:

$pipe = new MiddlewarePipe([
    new class implements ServerMiddlewareInterface
    {
        public function process(ServerRequestInterface $request, DelegateInterface $delegate) {
            return $delegate->process(new Request()); // dispatch-test.php(26)
        }
    },
    new \Middlewares\Geolocation(),
]);

The resulting error:

PHP Fatal error:  Uncaught TypeError: Argument 1 passed to Middlewares\Geolocation::process() must implement interface Psr\Http\Message\ServerRequestInterface, instance of Zend\Diactoros\Request given, called in /test/vendor/equip/dispatch/src/Delegate.php on line 49 and defined in /test/vendor/middlewares/geolocation/src/Geolocation.php:76
Stack trace:
#0 /test/vendor/equip/dispatch/src/Delegate.php(49): Middlewares\Geolocation->process(Object(Zend\Diactoros\Request), Object(Equip\Dispatch\Delegate))
#1 /test/tests/dispatch-test.php(26): Equip\Dispatch\Delegate->process(Object(Zend\Diactoros\Request))

The error message is misleading, the problem is not that I've called Geolocation::process() with the wrong argument, I've called $delegate->process. Okay, it is easy for us to figure out, the real problem is mentioned at #1 and we may improve the error message, like I did below. But do we need to annoy novice middleware developers? Moreover, a novice may bark up the wrong tree and blame @oscarotero for his great middleware or would ask for help at stackoverflow, mailing lists and chat rooms :unamused:

With process(ServerRequestInterface $request):

PHP Fatal error:  Uncaught TypeError: Argument 1 passed to Equip\Dispatch\Delegate::process() must implement interface Psr\Http\Message\ServerRequestInterface, instance of Zend\Diactoros\Request given, called in /test/tests/dispatch-test.php on line 26 and defined in /test/vendor/equip/dispatch/src/Delegate.php:43
Stack trace:
#0 /test/tests/dispatch-test.php(26): Equip\Dispatch\Delegate->process(Object(Zend\Diactoros\Request))

That would be the right one to me.

About middlewares with different signatures

How would such a client middleware may look like? I can only imagine 3 types @weierophinney may had in mind, they would be similar to:

// 1. same signature
interface ClientMiddlewareInterface extends ServerMiddlewareInterface
{
    public function process(ServerRequestInterface $request, DelegateInterface $delegate);
}

// 2. argument type contravariance
interface ClientMiddlewareInterface extends ServerMiddlewareInterface
{
    public function process(RequestInterface $request, DelegateInterface $delegate);
}

// 3. unrelated 
interface ClientMiddlewareInterface
{
    public function process(RequestInterface $request, ClientDelegateInterface $delegate);
}

The first one would be really strange – how should it work in a client container? The second one is not allowed in PHP – it is allowed in Java and C#, but neither in PHP 5 nor in PHP 7. Thus we end up with the third one.

What does that mean for a server middleware pipe? It cannot typehint to callable at the moment, thus it must use mixed or offer different methods for each type.

What we lose with DelegateInterface::process(ServerRequestInterface)

A server middleware pipe will likely want to wrap the client middleware, and throw an appropriate a error:

class ClientAdapter implements ServerMiddlewareInterface
{
    protected $middleware;

    public function __construct(ClientMiddlewareInterface $middleware)
    {
        $this->middleware = $middleware;
    }

    public function process(ServerRequestInterface $request, DelegateInterface $delegate)
    {
        return $this->middleware->process($request, new class($delegate) implements ClientDelegateInterface {
            protected $delegate;

            public function __construct(DelegateInterface $delegate)
            {
                $this->delegate = $delegate;
            }

            public function process(RequestInterface $request)
            {
                if (! $request instanceof ServerRequestInterface) {
                    throw new \TypeError('Argument 1 passed to process() must implement interface '.ServerRequestInterface::class.', instance of '.get_class($request).' given');
                }

                return $this->delegate->process($request);
            }
        });
    }
}

Ugh, that is ugly. Btw, if both switch to callables it becomes much more readable in my opinion:

class ClientAdapter implements ServerMiddlewareInterface
{
    protected $middleware;

    public function __construct(ClientMiddleware $middleware)
    {
        $this->middleware = $middleware;
    }

    public function process(ServerRequestInterface $request, callable $delegate)
    {
        return $this->middleware->process(
            $request,
            function(RequestInterface $request) use ($delegate) {
                if (! $request instanceof ServerRequestInterface) {
                    throw new \TypeError('Argument 1 must implement interface '.ServerRequestInterface::class.', instance of '.get_class($request).' given');
                }

                return $delegate($request);
            }
        );
    }
}

But I don't want to talk about callables.

This was just a hypothetical ClientMiddleware, if we think about promise-based middlewares (like Guzzle) or generator based middlewares (like my Cormy Bamboo), then we always need adapters. Therefore, I don't see any possible benefit of typehinting to client interfaces in server interfaces.

[schnittstabil]

Side Note: Thanks to @mindplay-dk, another argument wich reduces the need to support client-middlewares can be found at https://github.com/http-interop/http-middleware/issues/37#issuecomment-263861411

[shadowhand]

I'm in favor of changing the DelegateInterface type hint to ServerRequestInterface. It is not our place (in writing a server middleware spec) to make assumptions about what will or will not be useful to async middleware. Most likely the two specs would have different namespaces or the async middleware spec will supersede PSR-15 entirely.

Given that there is no one (to my knowledge) has started work on an async middleware spec, I would prefer that PSR-15 only refer to ServerRequestInterface and never to RequestInterface, as per #20.

[shadowhand]

@schnittstabil can you submit a PR for this please?