4.2 - Middleware Callable Resolver Support

[adriansuter]

See #2779.

We should modify the CallableResolver (and its interface) in order to be able to use it in the MiddlewareDispatcher. There should be 3 methods:

• Resolve a regular callable (For any invocables)
• Resolve a route callable
• Resolve a middleware callable

Currently, in 4.0.x and 4.1.x, the logic of resolving the middleware callable has been hard coded into the dispatcher. This has to be replaced.

Warning: This is a BC! New users of Slim 4 should, if possible, not mess around with the callable resolver.

At the same time, the arguments of the App constructor should be extended by a MiddlewareDispatcherInterface such that users could provide their own dispatcher implementation.

[l0gicgate]

I know we are going against typical versioning practices by doing this but it's too early in the release cycle to wait until the next major version to fix this.

This BC break will be seamless for most users unless you:

• Implemented your own CallableResolverInterface
• You instantiated the app manually instead of using AppFactory

[l0gicgate]

We might be able to implement this functionality without having to change the interface after some thoughts. Since the method CallableResolverInterface::resolve() is not typed, we can pass in a wrapper which will indicate what kind of callable needs to be resolved.

Example of a typed callable (this would be for internal usage only anyway):

<?php
declare(strict_types=1);

namespace Slim;

class TypedCallable {
    public const GenericCallable = 0;
    public const RouteCallable = 1;
    public const MiddlewareCallable = 2;

    /**
     * @var callable|string
     */
    private $callable;

    public function __construct(int $type, $callable) {
        $this->type = $type;
        $this->callable = $callable;
    }

    /**
     * @return int
     */
    public function getType(): int
    {
        return $this->type;
    }

    /**
     * @return callable|string
     */
    public function getCallable()
    {
        return $this->callable;
    }

    /**
     * @param $callable
     * @return TypedCallable
     */
    public static function genericCallable($callable): TypedCallable
    {
        return new static(self::GenericCallable, $callable);
    }

    /**
     * @param $callable
     * @return TypedCallable
     */
    public static function routeCallable($callable): TypedCallable
    {
        return new static(self::RouteCallable, $callable);
    }

    /**
     * @param $callable
     * @return TypedCallable
     */
    public static function middlewareCallable($callable): TypedCallable
    {
        return new static(self::MiddlewareCallable, $callable);
    }
}

Example internal usage resolving a route callable:

$callableResolver = new CallableResolver(...);
$typedCallable = new TypedCallable::routeCallable($this->callable);
$resolved = $callableResolver->resolve($typedCallable);

[adriansuter]

Good idea. That might work and avoids a breaking change.

[adriansuter]

Well, maybe a small BC. If someone implemented a callable resolver and suddenly gets a TypedCallable instance, this would fail. So the user would of course expect the types indicated in the docblock.

[l0gicgate]

That's right. I'm not sure what to do here.

[adriansuter]

Maybe we should leave it as is. The resolver stuff hard coded into the dispatcher.

[adriansuter]

If a user would like to have a custom middleware callable resolver, the user has to implement a MiddlewareDispatcher and pass it to the App. Currently this is not possible, but sooner or later it will, see #2785.

[adriansuter]

Would it make sense to add a new MiddlewareCallableResolverInterface that will be passed to the middleware dispatcher?

[l0gicgate]

@adriansuter it's not a bad idea, but it still doesn't solve the current problem.

Route callables are being resolved where generic callables are being resolved within the current CallableResolver https://github.com/slimphp/Slim/blob/4.x/Slim/CallableResolver.php#L87-L89

There should be a clear logic separation between:

• Generic Callables
• Route Callables
• Middleware callables

Adding MiddlewareCallableResolverInterface adds another parameter to the constructor once we've added MiddlewareDispatcherInterface which I'm not a fan of when it could be combined in one interface.

This is a tough problem to solve without breaking things.

[adriansuter]

Maybe an AdvancedCallableResolverInterface that extends the CallableResolverInterface? The advanced one would add two additional methods for route and middleware callable resolvers.

Then, whenever we need to call the resolve method (for example in the middleware dispatcher), we check if the given callable resolver is advanced and if so we call the appropriate method. Otherwise we do as we did before.

I am tired, not sure if that is even possible, does make sense or if that would not break anything.

[mnapoli]

Just for the record, a BC break should lead to a new major version.

Although here I don't really understand the change:

The current signature of the callable resolver is:

    public function resolve(string|callable $toResolve): callable;

Why would the behavior be different depending on whether the callable is a route or a middleware?

Shouldn't all callables be resolved in the same way?

[mnapoli]

Also that is way too late for that but to add more context, callable resolvers and invokers is something I've worked on standardizing in https://github.com/PHP-DI/Invoker

Maybe there's a new interface we could extract here and push to container-interop or the PHP-FIG (later of course).

[adriansuter]

@mnapoli

Why would the behavior be different depending on whether the callable is a route or a middleware?

As you can pass a string of a class name to the resolver, the resolver needs to know, what method name he has to consider. Currently they are not standardized. In the case of routes, the handler class can for example implement \Psr\Http\Server\RequestHandlerInterface in which the method name is called handle. In the case of middleware, the handler class can implement \Psr\Http\Server\MiddlewareInterface in which the method name is called process.

If someone designs a class that implements both interfaces, then the resolver has to know, which of the methods should be considered to be the callable. Thus we need to inform the resolver about the scenario we are in.

Shouldn't all callables be resolved in the same way?

Ideally, yes.

[l0gicgate]

@mnapoli

Just for the record, a BC break should lead to a new major version.

We're trying to avoid this at all costs obviously. This is what #2787 circumvents

Why would the behavior be different depending on whether the callable is a route or a middleware?

Shouldn't all callables be resolved in the same way?

As @adriansuter stated yes. The problem lies in the fact that when resolving a route, we assume that if it implements RequestHandlerInterface then we must use the handle() method, but that's based on the assumption that the object is solely used for that which is wrong as seen here:
https://github.com/slimphp/Slim/blob/4.x/Slim/CallableResolver.php#L87-L89

The same applies for resolving middleware, currently all the logic is handled within MiddlewareDispatcher but we want to leverage the rich logic of CallableResolver without having to duplicate code while offering more control to the end user over how callables are resolved.

Currently, there is no separation in logic and there should be. Some logic only applies to a specific type of callable and it shouldn't be based on assumptions.

The end goal is being able to resolve disctinct types of callables without making assumptions:

Generic Callables:
For example ErrorRendererInterface uses the __invoke method and is just a regular callable. This means the following shape of a callable resolved can be:

• A named function function MyErrorRenderer(...)
• A class string with deferred instantiation MyErrorRenderer::class that is invokable via __invoke
• A container key $container->get('myErrorRenderer')
• A Slim style callable Class:method

Route Callables:
The following types of callables would be valid:

• A RequestHandlerInterface which we would automatically call the method handle()
• A named function function MyRouteHandler(...)
• A class string with deferred instantiation MyRouteHandler::class that is invokable via __invoke
• A container key $container->get('myRouteHandler')
• A Slim style callable Class:method

Middleware Callables:
The following types of callables would be valid:

• A MiddlewareInterface which we would automatically call the method process()
• A named function function MyCallableMiddleware(...)
• A class string with deferred instantiation MyMiddleware::class that is invokable via __invoke
• A container key $container->get('myMiddleware')
• A Slim style callable Class:method

As you can see, they have a lot of similarities while still having some logic that shouldn't be shared with other types of callables. Hence the need to separate in the 3 proposed methods.

[mnapoli]

But… a callable with a class name has to be a __invoke method. If you want a callable to represent a method, then it should be the array representation [MyRequestHandler::class, 'handle'] or string MyRequestHandler::class . 'handle' (and then on top of that yes I understand that the class name is resolved into an object by the resolver).

If I understand correctly you adopted a convention that MyRequestHandler::class actualy means [MyRequestHandler::class, 'handle'] instead if it's a middleware.

It may be too late, but I want to strongly encourage to reconsider this so that, for the callable resolver, a callable is just a callable. There should be (hopefully) no custom logic to apply that is related to Slim's internals.

The way you could apply the custom logic would be higher up in Slim.

Here is a rough example just to illustrate what I mean (no idea if possible in Slim as it is):

class App
{
    public function add($middleware, ...) {
        // probably more checks to add here
        if (is_string($middleware) && class_exists($middleware)) {
            // I turn it into a valid callable here because I know what method to call
            $middleware = [$middleware, 'handle'];
        }
        $this->middlewares[] = $middleware;
    }

    ...

    private function callMiddleware($middleware) {
        // All callables look the same to me here
        $middleware = $this->callableResolver->resolve($middleware);

        ...
    }
}

The callable resolver should not care what a callable is used for. It's job is just to "resolve callables".

I would also recommend to drop the "Slim-style" callables and instead use classic PHP callable signatures, with DI resolution on top of that. Here is a gist that summarizes it that I wrote some time ago: https://gist.github.com/mnapoli/f8afef0f556010c144e7

Advantages:

• looks familiar because it looks exactly like any valid PHP callable
• easy to integrate with DI containers/callable resolvers (I did the job here and it works with any container out of the box, extremely tested and validated by production usage, we could move that to a separate repository if necessary)
• compatible with the ::class notation which gives us autocompletion and IDE validation

I really wish we had that discussion a few months ago ^^

[l0gicgate]

@mnapoli

If I understand correctly you adopted a convention that MyRequestHandler::class actualy means [MyRequestHandler::class, 'handle'] instead if it's a middleware.

I did not make those decisions, RequestHandler support from within CallableResolver was added in 3.x in this hackish way as a quick fix I'm assuming. I would have voted against that for the reasons stated above.

I would also recommend to drop the "Slim-style" callables and instead use classic PHP callable signatures

I tried to push for that for Slim 4 and it was voted against. The main reason for using this is that we can defer the instantiation of the object while still being able to call the method we want.

Example:

// Invalid as a regular callable
$invalidAsRegularCallable = [MyUninstantiatedClass::class, 'method'];

// Slim enables this via our custom callable pattern
$validSlimCallable = MyUninstantiatedClass::class . ":method";

I really wish we had that discussion a few months ago ^^

Me too!

[mnapoli]

OK thank you that's a little more clear to me.

// Invalid as a regular callable
$invalidAsRegularCallable = [MyUninstantiatedClass::class, 'method'];

// Slim enables this via our custom callable pattern
$validSlimCallable = MyUninstantiatedClass::class . ":method";

I am talking about doing exactly the same thing but making [MyUninstantiatedClass::class, 'method'] valid: the callable resolver would resolve MyUninstantiatedClass::class (e.g. via the container) to turn the invalid-callable into a valid callable.

The main advantage over MyUninstantiatedClass::class . ":method" is that it looks visually similar to all other PHP callables.

Anyway, I understand, this ship has sailed. Thanks for taking the time to explain.

[l0gicgate]

I am talking about doing exactly the same thing but making [MyUninstantiatedClass::class, 'method'] valid: the callable resolver would resolve MyUninstantiatedClass::class (e.g. via the container) to turn the invalid-callable into a valid callable.

The main advantage over MyUninstantiatedClass::class . ":method" is that it looks visually similar to all other PHP callables.

I would have proposed this as a solution initially instead of Slim style callables, unfortunately I wasn't around then.

We should actually support that style as well though. It shouldn't be too hard to do.

[b-hayes]

I ended up here because I was looking to replace the CallableResolver in slim 3 and guessing what im about to say still applies in v4.

I really like the syntax of [MyUninstantiatedClass::class, 'staticMethod'] and it works fine for static calls yes but I want it to work with instance methods without having to do a whole bunch of repetitive code for every route and/or add property tags to a custom container wrapper or some other work around in order to get code assistance form my IDE and have a slimple rout = function setup.

Having [MyUninstantiatedClass::class, 'instanceMethod'] would be ideal as IDE's can recognize this and provide a lot refactoring assistance without us having to do extra code in order to get that assistance.

eg. when type the callable reference even tho the method is a string literal I have the method definition appear with the comments ect, I can auto complete and then ctrl+click through to the definition and means I also see all usages of the method with a single click. Right now we are making a controller for every route path with __invoke() method but I'd like to just set a rout to = a method on a class without having to open function with params and a use($container) statement to access the controller class and execute a method with 0 assistance form the IDE like im used to.

Seems logical to me that this would be supported since just passing the class name on its own will resolve to container instance if it is defined usually if no method is provided.

And I think this would be a not breaking change just supporting this one use case since doing it will only prevent things from breaking when using a instance method statically and nothing can break calling a static method on an instance eg..

class TestInstanceStatic{
    public function instanceMethod(){
        echo __METHOD__," executed \n";//could be fatal as $this doesnt exist when called statically
    }
    public static function staticMethod(){
        echo __METHOD__," executed \n";//nothing the instance has will affect normal operation of a static function
    }
}
$testInstanceStatic = new TestInstanceStatic();
$testInstanceStatic::instanceMethod();
//PHP Deprecated:  Non-static method TestInstanceStatic::instanceMethod() should not be called statically
//TestInstanceStatic::instanceMethod executed
$testInstanceStatic->staticMethod();
//TestInstanceStatic::staticMethod executed```

[mnapoli]

@b-hayes I'm slightly off topic but in the meantime, if you want this feature a workaround is to use this bridge: http://php-di.org/doc/frameworks/slim.html

[juliangut]

Another option is to create your own CallableResolver in which you can transform one notation to another: in https://github.com/juliangut/slim-php-di/blob/master/src/CallableResolver.php#L136 you can see how Slim's string notation is transformed into PHP-DI's, the opposite is also possible

[b-hayes]

@juliangut I ended up doing exactly that after I made the comment. If the class name is defined in the container and the method name exists it will get the instance from there and execute it. Otherwise if the method exists it will try and create an instance and execute it, but obviously only works if the controller construct has no params.

    {
        //note: new logic to make instance methods work
        if (is_array($toResolve) && count($toResolve) === 2) {
            if (!method_exists($toResolve[0], $toResolve[1])) {
                throw new RuntimeException(
                    'Class method ' . $toResolve[0] . $toResolve[1] . ' does not exist!'
                );
            }
            //warning the class is instantiated here
            if ($this->container->has($toResolve[0])) {
                return $resolved = [$this->container->get($toResolve[0]), $toResolve[1]];
            }
            //container doesnt have one lets try making it ourselves!
            return $resolved = [new $toResolve[0]($this->container),$toResolve[1]];
        }
        //the original callable resolver code follows on from here...```