Allow any callable as a handler

[shadowhand]

Callable handlers, not just closures, should be allowed.

Refs #4, #5

[shadowhand]

Added Container::has() check because a string can be a callable.

[shadowhand]

Might as well use existing instanceof checks for CallableHandler.

[oscarotero]

Middlewares\Utils\CallableHandler implements both interfaces (MiddlewareInterface and RequestHandlerInterface) and is callable too (has an __invoke), so I'd move is_callable() to the end, in order to avoid a CallableHandler inside other CallableHandler.

[shadowhand]

cc @luketlancaster

[oscarotero]

Is there an use case for this?

The reason why I didn't allow strings as callables is because you never know whether you want a container entry that doesn't exist (so it must throw a notFoundException) or you want a function name. And due the default container can resolve strings, I'd use strings always as container ids and never as callables.

And the reason of why only Closure is allowed is because they can be created easily from any callable thanks to Closure::fromCallable, that provide a better control, performance and safety.

[shadowhand]

The use case is pretty simple:

class FooController
{
    public function __invoke(ServerRequestInterface $request) { /* ... */ }
}

And the route is defined as:

'foo' => FooController::class,

I would rather not write this for every single route:

'foo' => \Closure::fromCallable(FooController::class),

[shadowhand]

you never know whether you want a container entry that doesn't exist

This is why I added $container->has(...) to the check.

[oscarotero]

The default RequestHandlerContainer should resolve FooController::class, isn't?

This is why I added $container->has(...) to the check.

Let's say that I have a controller called print and my container should return an instance of Controllers\PrintController. If the controller does not exist yet, it still being a valid callable so no error is throw.
And if it's not a valid callable it throws a RuntimeException instead a NotFoundException so, at the end, you will get random results depending of the name of your controllers.

[shadowhand]

If the controller does not exist yet

This argument doesn't make sense. By the time this code runs, the container should know what objects it has available and be able to determine that print means PrintController.

As an aside, many containers are opting to use full class names as the identifier, so it is fairly unlikely that there would be a conflict between $container->has(...) and is_callable(...) checks.

[oscarotero]

What I mean is that $controller->has('print'), can return false (because the class PrintController does not exists, or is misnamed to PrinController, or you forget to add the controller php file before commit, etc...), and the middleware will try to execute the print() function. Or if this happens with a non existing function (let's say $controller->has('listUsers')), you'll get a RuntimeException instead a NotFoundException.

If strings are considered as callables too, there's no way to restrict the requestHandler is resolved only through the container, so if it exists as a global function it will be executed as a fallback. I can consider accept any callable but strings.

Other thing is why you find useful to have a controller with __invoke instead implements the RequestHandlerInterface. In fact, __invoke was suggested but rejected as the method of a RequestHandlerInterface 😃

Anyway, your use case should work perfectly right now, because the default RequestHandlerController should handle this case (as it does in the tests), or please, let me know if this is not happening.

[shadowhand]

Other thing is why you find useful to have a controller with __invoke instead implements the RequestHandlerInterface.

I don't want to import an interface for all my controllers. There's no benefit to doing that, since the controllers are specific to my application and don't need interop.

[shadowhand]

I updated the PR and removed support for callable strings.

[oscarotero]

Middlewares\Utils\CallableHandler implements both interfaces (MiddlewareInterface and RequestHandlerInterface) and is callable too (has an __invoke), so I'd move is_callable() to the end, in order to avoid a CallableHandler inside other CallableHandler.

[oscarotero]

Because this is used only inside if (is_string($requestHandler)), I'd not create the instance in the constructor, but inside that if.

[shadowhand]

I think the consistency is valuable. There's no real performance impact of doing this.

[oscarotero]

@shadowhand Thanks 😃