http-interop / http-middleware

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

__invoke and parameter type widening #64

Closed schnittstabil closed 7 years ago

schnittstabil commented 7 years ago

PHP 7.2 will allow us to do:

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

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

class M implements MiddlewareInterface
{
    /*
     * removed $delegate type-hinting!
     */
    function process(ServerRequestInterface $request, $delegate)
    {
    }
}

$m = new M();
var_dump($m instanceof M); // => true
$m->process(new ServerRequest(), $m); // no error or warning 

tl;tr: How can we prevent that?


I love PHP because of that. But according to the current README.md:

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.

and the explanations of @shadowhand at https://github.com/http-interop/http-middleware/pull/55#issuecomment-271609382 and his clarification at https://3v4l.org/4Ncgj:

// with an __invoke interface:
interface DelegateInterface
{
    public function __invoke(ServerRequestInterface $request);
}

// this becomes possible, but is bad
public function process(ServerRequestInterface $request, callable $delegate);

// without __invoke, only this is possible, which is good
public function process(ServerRequestInterface $request, DelegateInterface $delegate);

I therefore conclude that @shadowhand also wants to prevent MiddlewareInterface implementers to remove the DelegateInterface type declaration entirely, simply because:

process interfaceno type declaration holds as well (in the imaginary type semilattice)

Hence: How can we prevent class M above?

schnittstabil commented 7 years ago

@DaGhostman at https://github.com/php-fig/fig-standards/pull/867#issuecomment-276822997 you have asked:

why are you opening pretty much the same issue with different wording

Simply because it took me months to figure out what @shadowhand exactly wants to accomplish, in this case it is preventing contravariance of method argument types at MiddlewareInterface::…(…, <?> $d).

Please reread README.md:

Why doesn't middleware use __invoke?

… 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.

The header is about MiddlewareInterface::__invoke, not about DelegateInterface::__invoke! FFS, is it any surprise that it took so long and so many github issues to figure that out?

And I hope you now understand the issues I want to have solved, a comprehensible PSR-15 based on facts, surveys, science and logical sound reasoning!.

schnittstabil commented 7 years ago

:tada: PHP RFC: Parameter Type Widening got landed yesterday :tada:

This means, in PHP 7.2 and upwards we cannot prevent above by the type declarations in our interfaces. If @shadowhand still wants to prevent that usage, as far as I can see, it MUST be done via the metadoc or the interface comments.

kelunik commented 7 years ago

There's no reason to prevent that. If the interface declares a type, the implementing class can only drop it, but MUST at least accept the declared type, otherwise it violates the LSP.

schnittstabil commented 7 years ago

There's no reason to prevent that.

I couldn't agree more, but I've stopped to convince the editor by means of arguments, reasons and examples.

If he still wants to prevent that, I want to support him to clarify the reasons behind his decision.

shadowhand commented 7 years ago

One problem we'll end up with if we do change the expected method to __invoke is incompatibility with callable middleware. It can be overcome by introspection (instanceof) but not via duck typing.

I'm still very much on the fence about this. On one hand, it makes this a bit cleaner:

$response = $middleware($request);

On the other hand, it makes things more complicated:

// cannot use $this->middleware($request);
$middleware = $this->middleware;
$response = $middleware($request);

I lean towards the latter as being a more significant concern.

shadowhand commented 7 years ago

Neither request handler or middleware will use __invoke. See #71.