Type-checking

[mindplay-dk]

I'm porting middleman to PSR-15, and I think we still have a type issue.

How do you check if a given value is middleware?

Currently, I have to do this:

if ($value instanceof MiddlewareInterface || $value instanceof ServerMiddlewareInterface)

That's ugly.

I mean, strictly speaking, server-middleware is a specialized type of middleware - but our interfaces do not express this type-relationship, so you have to write two type-checks to find out if a value is middleware.

This indicates a pretty serious problem to me, communication-wise, semantically, etc. - if a type-check like that is necessary to check for what is actually a single pseudo-type, we're not using the type-system correctly.

Also, this is highly inconsistent with PSR-7, where ServerRequestInterface extends RequestInterface - it would be natural to expect a symmetry of these types, which means that ServerMiddlewareInterface should extend MiddlewareInterface.

Now, this may not be ideal, but given the constraints of the type-system, I think that the following is technically the only correct way to model this type relationship with the PHP type system:

interface MiddlewareInterface {
    public function process(RequestInterface $request, DelegateInterface $delegate);
}

interface ServerMiddlewareInterface extends MiddlewareInterface {
    public function processServer(ServerRequestInterface $request, DelegateInterface $delegate);
}

We can quibble about the method-names, but I don't think there's any good way to get around this - server middleware is a specialization of middleware, and it should be modeled that way.

And yes, this means that server-middleware must implement both methods. This is not necessarily a bad thing - while many implementations of process will simply return $this->processServer($request, $next), it removes the need to type-check, as this call will simply fail for server-only middleware:

class MyMiddleware implements ServerMiddlewareInterface {
    public function process(RequestInterface $request, DelegateInterface $delegate) {
        return $this->processServer($request, $delegate); // may error!
    }

    public function processServer(ServerRequestInterface $request, DelegateInterface $delegate) {
        // ...
    }
}

Note that the marker interface does not solve this problem - if we have both types extend an empty interface, as we did previously, this still requires the exact same run-time type-checks; only now, if you also type-hint against the marker-interface, that makes the type-relationship even more illogical:

function run(CommonMiddlewareInterface $middleware) {
    if ($middleware instanceof MiddlewareInterface || $middleware instanceof ServerMiddlewareInterface) {
        // ...
    } else throw new RuntimeException(...);
}

As you can see, in practice, the marker interface changes nothing - and also, it still fails to provide symmetry with PSR-7, it just does so in a different way.

The problem is, what we have here is a generic type relationship, and there is no way to express that with the PHP type-system.

I'm beginning to think we'd be better off with less type-constraints and a simpler middleware signature, e.g. something closer to the de-facto standard.

I mean, we're struggling for type-safety here, and I am hugely in favor of type-safety, but it really seem like the PHP type-system cannot express this type-relationship, and whatever work-arounds we come up with, one way or the other, will inevitably be wrong. I think we're trying to do something that can't be done with PHP. Maybe we'd be better off accepting the fact that PHP is a dynamic language?

We could of course still type-hint statically with php-doc, but I'm no longer convinced we're going to get anywhere (good) by warping the PHP type-system concepts trying to bend it to our will.

It sucks, but, I tried to work with this for just a couple of hours, and, I already hate it. It obstructs rather than enables me. I never hated working with the de-facto standard and it never caused much trouble, even if it's very dynamic.

As a footnote, I also found that the existence of DelegateInterface creates obstruction - as I predicted in a comment earlier, it requires me to wrap callables in a meaningless object, which, internally, isn't type-safe anyhow:

class Delegate implements DelegateInterface
{
    /**
     * @var callable
     */
    private $callable;

    /**
     * @param callable $callable
     */
    public function __construct($callable)
    {
        $this->callable = $callable;
    }

    public function next(RequestInterface $request)
    {
        return call_user_func($this->callable, $request);
    }
}

There's no way around doing this, and I expect every middleware stack will need to do the same. (My first impulse was to have a delegate implementation that takes a reference to the stack, and an index - but without a callable somewhere in between, what you end up with in the middleware dispatcher, is immediate recursive creation of the entire middleware stack, which is not what you want, so there's no way around the callable, and therefore no way around the meaningless wrapper, as far as I can figure...)

[mindplay-dk]

Oh, and there's one more uncomfortable asymmetry, with server-middleware receiving a DelegateInterface, which would accept a non-server-request - and, again, my concern here is not so much type-safety, but the fact that the type-hints are misleading: the interface signature implies you can pass a non-server-request, when in fact you can not.

My concern here is human comprehension. We make these trade-offs to get more type-safety, but we get only partial type-safety due to limitations of the type-system, so we're not getting what we aim for in the first place - and the resulting PHP types do not accurately represent the true type constraints, which is misleading.

[mindplay-dk]

You can see a preliminary port of middleman to PSR-15 0.1 here.

It's not very gratifying or meaningful work - basically two new meaningless wrapper-classes for partial type-safety, and a bunch of added run-time if/else type-checks.

Note that I already previously ported middleman to lambda-style, so this work starts from an already-lambda-style branch - that work was definitely worthwhile, it actually simplified the code in some ways, and also has substantially simplified the client code in which I'm using it.

This change, on the other hand, only seems to complicate things, and will complicate the client code as well, with no clear gain. This doesn't seem worthwhile...

[shadowhand]

How do you check if a given value is middleware?

This type check will be made simpler by union types.

I think that the following is technically the only correct way

And also a nightmare to use. Having for fork in the dispatching based on the type of middleware and in the delegate is just moving the instanceof check to a different place, it doesn't actually reduce the code required or make it easier to understand in any meaningful way.

It's also technically incorrect, in so far as the only reasonable implementation of ServerMiddlewareInterface::process() will be:

throw new RuntimeException('This middleware can only be used with server requests.');

I also found that the existence of DelegateInterface creates obstruction - as I predicted in a comment earlier, it requires me to wrap callables in a meaningless object

Why would you use a callable? I expect the "standard" implementation of delegate to be something like the Tari implementation, where the delegate is an immutable object that takes the stack and clones itself as the it increments through the stack.

[shadowhand]

Oops, I submitted that last comment too early. I was going to add:

I think you are struggling to provide exceptions where we should be allowing the PHP type system to provide runtime safety. To wit: don't do so much type checking yourself. Assume that the user has set up the stack correctly (within reason) and if the user does something wrong, allow a fatal error to happen. I can think of no scenario where the middleware stack would be so dynamic that it would be necessary to catch an exception during middleware dispatching.

Due you have use cases where the middleware stack would actually be reconfigured and/or dynamically modified based on user input? If not, then we can assume that once the middleware stack is configured in development, the same stack will also be used in production and testing one will ensure that other is sane.

[mindplay-dk]

Why would you use a callable? I expect the "standard" implementation of delegate to be something like the Tari implementation, where the delegate is an immutable object that takes the stack and clones itself as the it increments through the stack

It avoids using closures only by using using new class instead, and deferring the creation of the next middleware on the stack that way.

I'm sure a lot of people are not yet ready to commit to PHP 7 to the point of completely excluding PHP 5.

If either a closure or class is not involved, the entire stack of middleware would be constructed recursively at the first call, all the way down to the bottom component, e.g. (typically) a 404 error page generator, which for most requests would never actually be executed. (Like e.g. Relay, my implementation uses the concept of a resolver to defer the creation of middleware components until they're needed.)

I think you are struggling to provide exceptions where we should be allowing the PHP type system to provide runtime safety

My point is the PHP type system can't provide run-time safety, only partially, because the type system does not formally support the kind of type-relationship we're dealing with. It's a generic type relationship - the only correct declaration would be something like:

interface DelegateInterface<TRequest : RequestInterface>
{
    public function next(TRequest $request) : ResponseInterface;
}

interface MiddlewareInterface<TRequest : RequestInterface>
{
    public function dispatch(TRequest $request, DelegateInterface<TRequest>) : ResponseInterface;
}

You could then implement either generic or server-middleware as e.g.:

class MyMiddleware implements MiddlewareInterface<RequestInterface>
{
    public function dispatch(RequestInterface $request, DelegateInterface<RequestInterface> $delegate) : ResponseInterface {
        // ...
        return $delegate($next);
    }
}

class MyServerMiddleware implements MiddlewareInterface<ServerRequestInterface>
{
    public function dispatch(ServerRequestInterface $request, DelegateInterface<ServerRequestInterface> $delegate) : ResponseInterface {
        // ...
        return $delegate($next);
    }
}

Generic syntax, to my knowledge, is the only way to correctly express a generic type relationship - if there's a way to correctly substitute that using a simpler type-system, I haven't heard about it, and none of the ideas we've come up with so far correctly express the actual type-constraints.

In my current opinion, we were fine with the total lack of type-safety in the de-facto standard - it was never a big problem for anyone; we (hopefully) write tests anyway. If we could have full, correct type-declarations, I'd be all for it, but as it stands, it doesn't look like we can - and the incorrect (or at least partially incorrect, still too wide) type-declarations are of no real practical value and lead to useless layers of complexity adding no actual safety or value, just mental and computational overhead.

That's my current opinion. If someone can propose something that is actually correct, and doesn't create a burden with no pay-off, I am of course more than eager to change my mind back again :-)

For starters, we could simply nix the ServerMiddlewareInterface, since the relationship between it and MiddlewareInterface can't be practically realized, and the relationship with RequestInterface and ServerRequestInterface can't be made symmetrical and can't correctly align with DelegateInterface.

Without a ServerMiddlewareInterface, at least what we do have won't be incorrect - it won't be complete either, of course, but it doesn't seem like the PHP type system is capable of that.

Better then, in my opinion, to accept the limitations of the type-system instead of trying to shoehorn it in.

[mindplay-dk]

Come to think of it, there is a way to express the relationship of the server middleware interface with the generic middleware interface - sort of.

Check it:

interface MiddlewareInterface
{
    public function process(RequestInterface $request, DelegateInterface $next);
}

interface ServerMiddlewareInterface extends MiddlewareInterface
{}

ServerMiddlewareInterface is now a marker interface, but it correctly inherits from MiddlewareInterface.

Thus both being correctly type-checkable as e.g. instanceof MiddlewareInterface - even if the signature for the inherited process() method in ServerMiddlewareInterface is too wide, but, as shown, you'll need the run-time type-check either way, so.

Functions type-hinting for ServerMiddlewareInterface will correctly refuse an instance that only implements MiddlewareInterface, so there's a plus.

That'll work, and it isn't strictly wrong, merely incomplete.

If one day we have a php-doc annotation to declare generics, as is now experimantally supported by etsy/phan, we'd be able to type-hint those these interfaces correctly for static analysis.

What do you think?

[shadowhand]

This suggestion does not provide type safety at the execution level, since the following would explode with an undefined method error:

$attr = $request->getAttribute('something');

The reason for ServerRequestInterface type hint is to provide runtime safety without throwing harder-to-debug errors somewhere within the body of the middleware, which may or may not be reached depending on how body forks.

[shadowhand]

I'm sure a lot of people are not yet ready to commit to PHP 7 to the point of completely excluding PHP 5.

While Tari is PHP 7, that doesn't mean the implementation it proposes is strictly PHP 7+. One could implement the concept by using a concrete class.

[mindplay-dk]

The reason for ServerRequestInterface type hint is to provide runtime safety without throwing harder-to-debug errors somewhere within the body of the middleware, which may or may not be reached depending on how body forks

I completely understand that - this was my line of argumentation when I first pushed for this layering of the interface.

The problem is, this comes at the cost of correct inheritance. The relationship between the two middleware types is non-existent, which is simply wrong.

Given that there is no way to type this correctly, I would prefer a lacking type-declaration over a wrong one.

That, or simply quit struggling and deal with reality - simply reduce to one middleware interface, as it is in most implementations today. Everyone is already used to doing the run-time type-check in middleware to make the assertion about server-requests.

It's not a problem.

Having type-relationships that are simply wrong is a much bigger problem, in my opinion.

I think it makes far more sense to simply say, "this is middleware - we're not going to specify or specialize for certain kinds of middleware, it's all just middleware", and then let developers deal with special constraints at run-time, just the same as they do today.

The correct interface, is this:

interface MiddlewareInterface<TRequest : RequestInterface>
{
    public function dispatch(TRequest $request, DelegateInterface<TRequest>) : ResponseInterface;
}

And it's closest realizable relative in PHP, is this:

interface MiddlewareInterface
{
    public function dispatch(RequestInterface $request, DelegateInterface) : ResponseInterface;
}

Coming up with a whole different set of types to try to get closer to the correct interface, simply doesn't make any sense.

Reality is, we can't model this with PHP, and in my opinion, we need to stop trying.

One could implement the concept by using a concrete class.

One would have to, that's the point I'm trying to make.

The problem is, wrapper classes like this one have zero value - they only provide superficial type-safety, the net effect of which is like a really complicated, expensive php-doc tag.

[weierophinney]

Everyone is already used to doing the run-time type-check in middleware to make the assertion about server-requests.

It's not a problem.

Where are you seeing this? Every middleware I see is typehinting on either RequestInterface (typically client-side middleware) or ServerRequestInterface (server-side middleware). This provides type safety. I have not seen any server-side middleware type hinting against RequestInterface unless it is using only methods defined in only that interface. In fact, if I have to do run-time type checking vs using the argument type hint, I consider that problematic, as it means I'm not programming to the interface, but rather a superset or an implementation.

I understand the arguments you're making, and agree we cannot model this perfectly in PHP yet, but I'd still prefer type safety over runtime checks any day.

[mindplay-dk]

Every middleware I see is typehinting on either RequestInterface (typically client-side middleware) or ServerRequestInterface (server-side middleware). This provides type safety

No, it does not. Even less so than a single middleware interface. The only type-safety we have today is callable, which makes zero assertions about anything.

What you have today is not type-safety, it's a run-time type-check declared using, yes, a type-hint, but the effect of that is merely a run-time type-check - the only type-safety, at the time when you insert the middleware, is callable.

I think that even a simple, generic middleware-interface, which you have to implement, which at least asserts that you're working with PSR-7 models, is a huge improvement over the status quo.

I understand the arguments you're making, and agree we cannot model this perfectly in PHP yet

Yes, yet - that's why it's really important that what we can and do model with this PSR at least be correct, so that we can incrementally improve these interfaces in the future.

If we model it wrong now, we won't be able to incrementally fix it in the future - fixing what we did wrong will be a BC break with serious consequences; as opposed to simply tightening the type-constraints that people are already meeting, which would be a simple transition.

I'd still prefer type safety over runtime checks any day.

As would I - but in this case, it's only possible by creating a model that is incorrect.

If we model it correctly, there's a good chance you'll be able to do offline inspections against generic php-doc tags with etsy/phan pretty soon, likely others will follow, and if we have generics in php someday, we'll be able to improve type-safety then.

I'm sorry that PHP does not provide the type-safety we both want. It's not ideal, but I don't believe we're helping anyone by abusing the type feature we do have.

Obviously, I too wish that we could do better, but it seems we're at the mercy of the type-system we're dealt with here.

[shadowhand]

https://github.com/http-interop/http-middleware/issues/19#issuecomment-244808533 proposes:

interface MiddlewareInterface
{
    public function process(RequestInterface $request, DelegateInterface $delegate);
}

interface ServerMiddlewareInterface extends MiddlewareInterface {}

I think this is the best we could do. I will update the current interfaces accordingly.

[weierophinney]

I've been playing with this, and I still disagree wholeheartedly with having ServerMiddlewareInterface not use ServerRequestInterface as the typehint.

There are a number of reasons why:

• IDEs do not offer completion for the various methods defined in ServerRequestInterface but not in RequestInterface.
• Server middleware that requires the ServerRequestInterface now needs to do an additional check within the middleware body to ensure it has a usable request instance: php if (! $request instanceof ServerRequestInterface) { throw new InvalidArgumentException(); }

The various points made by @mindplay-dk all center around the fact that PHP doesn't allow us to model this properly (by allowing an extending interface to narrow the scope of an argument and/or allowing union types). I get that. However, we have to work with the language we have, not the language that could be (or might never be). We can always create a replacement PSR later to account for such language changes; distasteful, yes, but pragmatic.

There are very few times where the middleware interfaces themselves will be typehinted against. These will typically be middleware dispatchers (e.g., Slim\App, Zend\Stratigility\MiddlewarePipe, when adding middleware to dispatch) or the actual delegate implementations. It will be up to the projects implementing dispatchers whether or not they will support either or both; supporting both is often a simple conditional or multiple methods for attaching middleware; documenting what is supported is all that's required.

For server-side middleware dispatchers, one option is to type-hint only on ServerMiddlewareInterface, and then require that any non-server middleware be decorated:

class ServerMiddlewareDecorator implements ServerMiddlewareInterface
{
    private $middleware;

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

    public function process(ServerRequestInterface $request, DelegateInterface $delegate)
    {
        return $this->middleware->process($request, $delegate);
    }
}

The bigger problem is DelegateInterface when inside a server-side stack. In the above, it's entirely possible for middleware to pass a completely new request instance into the next layer; if that new instance is not a ServerRequestInterface, deeper layers will have issues. This is bad behavior on the part of the middleware regardless, and PHP will catch the problem early, due to the types. Having a ServerDelegateInterface only means the issue might be caught slightly earlier, when calling $delegate->next(). Either way, I'm willing to let PHP handle the type error in this situation.

However, the problems noted above — IDE completion, requiring manual type checking in server-side middleware — will remain if we have ServerMiddlewareInterface extend a MiddlewareInterface that type-hints on RequestInterface, and these make the spec unwieldy.

How they are currently defined (as of ad3f0cc8) makes a ton of sense to me, and gives me type safety and usability, both from an implementer and consumer perspective. Please, let's leave this as-is.

[mindplay-dk]

The problem with having two completely separate interfaces is it explicitly designates "middleware" and "server middleware" as being two completely unrelated things - but in reality, "server middleware" is a more specific kind of "middleware", just like a "server request" is a more specific kind of "request".

To make matters worse, as proposed, "middleware" and "server middleware" have conflicting method-signatures - so it's not even possible to implement both.

I'm sorry, but I find that completely unacceptable. Reality is that we're forced to choose between wrong or incomplete - given that there is no right way to do this, surely being incomplete must be better than being just plain wrong?

In my opinion, the only way we can have both, would be to avoid the method-name collision, e.g. by qualifying the method-names so they don't collide:

interface MiddlewareInterface
{
    public function processRequest(RequestInterface $request, DelegateInterface $delegate);
}

interface ServerMiddlewareInterface extends MiddlewareInterface
{
    public function processServerRequest(ServerRequestInterface $request, ServerDelegateInterface $delegate);
}

This isn't wrong, it's just slightly more boilerplatey, having to implement two methods instead of one.

It is however correct, as well as complete - without generics, there is not other way to declare this completely and correctly.

I can't suggest anything else, because the language doesn't offer anything else.

Favoring brevity over correctness, to the point that you're willing to knowingly model things incorrectly would be, frankly, the death of this PSR as far as I'm concerned. I wouldn't want any part in it.

[shadowhand]

That suggestion implies that calling ServerMiddlewareInterface::processRequest() is perfectly viable, and it isn't.

[shadowhand]

I agree with all of @weierophinney's points. As such, I think the current implementation (two explicit interfaces) is the best we can do for usability and type safety. Union types will make the type hint issue go away and using separate methods will work in the interim.

[oscarotero]

Hello. Just to clarify:

Let's say I want to create a middleware to remove the trailing slash of the url. If I whant to use it as a client (for example, to make request to an API), I have to use the TrailingSlashMiddleware but to use it in the server site, I should create a ServerTrailingSlashMiddleware, or create a ServerMiddlewareWrapper to allow to execute client middlewares as server middlewares. Am I right?

There's no a ServerDelegateInterface, so, tecnically a ServerMiddlewareInterface could pass a RequestInterface to the next middleware. Is this correct?

[shadowhand]

@oscarotero you only need to implement ServerMiddlewareInterface if your middleware requires methods that are defined in ServerRequestInterface. Mixing client and server middleware in a stack perfectly acceptable, so long as the dispatched request is a server request.

Technically speaking, a badly written middleware could replace the request with a different request type. In the real world, this should never happen because a middleware should only modify (using with* methods) the existing request, not replace it.

[oscarotero]

Ok, thanks for the clarification

[mindplay-dk]

You guys are fighting the limitations of the language with fire.

I don't like where this is going.

On the upside, hopefully this PSR will help promote the idea of lambda-style middleware, and eliminate the use of double-pass - which should make it fully compatible with custom third-party interfaces implementing __invoke() or using callable, to which all of our middleware (at work) has already been ported; which simplified a number of oddities surrounding controllers and dispatch in our projects already.

The downside is that this PSR effectively introduces two new middleware abstractions - implementing support for this in a dispatcher will essentially be like supporting two additional types of middleware.

Oh, well.

[mindplay-dk]

I took another pass at adding PSR-15 support to mindplay/middleman, and this time it turned out relatively not horrible.

I'm basically treating the PSR-15 interfaces as just two more types of support middleware with no special status, e.g. retaining equal support for callable and any object implementing __invoke(), and somehow this turns out a lot cleaner.

One thing still bothers me, and that's the existence of the Delegate class which exists solely to satisfy a type-check - it's a shim class wrapping a callable, and it's doing nothing of any practical value.

(I tried various "tricks" to see if I could get around this by actually moving the responsibility of resolving middleware into the DelegateInterface implementation, but that only seemed to create a kind of artificial/meaningless separation of concerns that weren't really separable.)

But okay, if that's the one side-effect of formally standardizing middleware, I guess it'll do.

@http-interop/http-middleware-contributors anyone else have implementations of ^0.1 compatible dispatchers for comparison?