Slim: DeferredCallable cannot accept static closures

[Bilge]

DeferredCallable makes the (invalid) assumption that any Closure can be bound to an instance of an object. Clearly this is false for static closures since they are unbound by definition.

The following code will generate a "Cannot bind an instance to a static closure" warning once the framework begins processing a request.

(new Slim\App)->add(static function () {})->process(...);

Slim should not attempt to bind to static closures.

[shadowhand]

And how do you suggest that DeferredCallable detects whether or not a closure has been defined with static?

[Bilge]

Let me Google that for you.

[shadowhand]

Which is say, it cannot be done without using @ error suppression. In my opinion, that's a non-starter. The documentation should say: "Static closures are not supported as handlers" and call it done.

[l0gicgate]

I think that we need to fix this issue. As @shadowhand said though is there's no "clean" way to detect whether a closure is bindable or not.

However, there's ways around that, we could include a bool on whether we want to autobind or not. Not everyone wants autobound closures, it makes for a really dirty call stack trace and it's annoying when debugging.

What do you guys think of something like:

public function add($middleware, bool $autoBind = true)

[shadowhand]

The other (and better, IMO) option is to only allow PSR-15 objects (instances or class names that implement the interfaces) to be used. That's probably a 👎 from a lot of people but it would do away with a significant amount of extra code that Slim has to execute to deal with closures.

[Bilge]

@l0gicgate Conceptually, that solution is OK for me, but I despise true as a default for any parameter. I would change it to the following, subtly different naming convention.

public function add($middleware, bool $preserveBinding = false)

[l0gicgate]

@Bilge I like that.

@geggleto @akrabat @adriansuter I'd like some feedback on implementing this. The only breaking change I foresee is if someone implemented their own CallableResolverInterface then the signature of the resolve() method would change as it would have an additional parameter to specify whether we want to preserve the existing binding or not.

I'm in favor of implementing it this way since there's no clean way to see if a closure is bindable, it also gives control to the user instead of forcing the auto-binding in the event that it is bindable.

[adriansuter]

I think that's fine.

Another solution would be to add a new method like addNonBound() or so to avoid the breaking change.

Personally I think we can risk this BC.

[akrabat]

Seems reasonable to me. I don't mind defaulting to false in Slim 4. Obviously not in Slim 3.

[l0gicgate]

@akrabat the false default of $preserveBinding would mean that it would autobind just like in Slim 3. You'd need to manually set that to true to turn off the automatic binding.

[akrabat]

@akrabat the false default of $preserveBinding would mean that it would autobind just like in Slim 3. You'd need to manually set that to true to turn off the automatic binding.

Hmm then preserveBinding is a terrible name :) I assumed that by setting to false, I did not want the binding...

I also assumed that @bilge suggested the change to flip the logic and default to not binding unless you explicitly set the second parameter to true

[l0gicgate]

Should it be preserveScope then maybe? Preserve binding seems pretty self explanatory to me though, you want to preserve the original binding of the closure if you set it to true.

[akrabat]

You only want to "preserve the original binding" if you know that Slim 3 defaulted to binding. A new user wouldn't know that.

I'd prefer: $bindContainer personally

[shadowhand]

I don't understand why this is even a thing. If you don't want closure binding, then don't use a closure. This "feature request" is making the Slim API more complicated:

$app->add(static function(){ /* ... */}, false);

When looking at this code the false parameter tells me absolutely nothing. Yes, IDE inspection will hint it for me, but why add this complexity in the first place?

$app->add(WhateverInvokableClassYouWant::class); // no binding will ever happen here

[akrabat]

I am equally good with @shadowhand's view that we don't support static anonymous functions.

[adriansuter]

What are the advantages of using static closures?

[l0gicgate]

@adriansuter the reason why someone would try to use a static closure is probably to control its binding (so it doesn't get autobound) or in case they would try to "optimize" performance while it is an unnoticeable increase.

[shadowhand]

If this feature request is denied, the Slim documentation should make it clear that all closures will be bound to the container and that static closures cannot be used for route groups, route handlers, or middleware.

[adriansuter]

Middleware

Tried to add a static closure as middleware.

$app->add(static function (
    ServerRequestInterface $request,
    RequestHandlerInterface $handler
): \Psr\Http\Message\ResponseInterface {
    echo 'MIDDLEWARE';

    return $handler->handle($request);
});

That worked like a charm.

Tried to add a class name as middleware.

class M implements \Psr\Http\Server\MiddlewareInterface
{
    /**
     * {@inheritdoc}
     */
    public function process(
        ServerRequestInterface $request,
        RequestHandlerInterface $handler
    ): \Psr\Http\Message\ResponseInterface {
        echo 'MIDDLEWARE';

        return $handler->handle($request);
    }
}

$app->add(M::class);

That worked like a charm. As long as there is no constructor, I did not even get the container inside the middleware. So a var_dump($this) inside the process()-method gave

object(M)#60 (0) { } 

Pretty clean 😄.

Route handlers and route groups

Tried to add a static closure as route handler.

$app->get('/',
    static function (ServerRequest $request, Response $response, array $args): \Psr\Http\Message\ResponseInterface {
        $response->write('STATIC ROUTE-HANDLER');

        return $response;
    }
);

This did not work like a charm. Now I got the warning "Cannot bind an instance to a static closure".

Is it correct, that this problem might be for route handlers (and route groups) only? Basically for \Slim\Routing\RouteCollectorProxy?
https://github.com/slimphp/Slim/blob/01f692327ece29bcf930db99bf1c12b84eafa6c9/Slim/Routing/RouteCollectorProxy.php#L181-L183

Solution (kinda hackish)

Right before binding we could set an ignoring error handler, and afterwards restore the old error handler.

        if ($this->container && $callable instanceof Closure) {
            set_error_handler(function () {
                /* ignore errors */
            });
            $boundCallable = $callable->bindTo($this->container);
            restore_error_handler();
            if ($boundCallable) {
                $callable = $boundCallable;
            }
        }

Similar in the \Slim\CallableResolver::resolve()-method.

        if ($this->container instanceof ContainerInterface && $resolved instanceof Closure) {
            set_error_handler(function () {
                /* ignore errors */
            });
            $boundResolved = $resolved->bindTo($this->container);
            restore_error_handler();
            if ($boundResolved) {
                $resolved = $boundResolved;
            }
        }

That way, a static closure would not be bound and the warning would be ignored.

[shadowhand]

/* ignore errors */ ⬅️ 😱

As you made clear, there is no need for static closures. If someone doesn't want binding, use a class.

[adriansuter]

@shadowhand That's why I called it hackish.

[l0gicgate]

I think we're going to label this as "wontfix" as we have in the past. We consider binding the container to a route callable as a feature which some dislike. We can't please everyone unfortunately.

Also @adriansuter I think we might be able to remove the binding statement in RouteCollectorProxy as that logic is duplicated in CallableResolver. The error happens during mapping instead of resolving, it will happen either way though:
https://github.com/slimphp/Slim/blob/01f692327ece29bcf930db99bf1c12b84eafa6c9/Slim/CallableResolver.php#L94-L96

[adriansuter]

I'm fine with that.

[l0gicgate]

Okay cool, let's remove the duplicate binding logic and also we need to document this better as @shadowhand pointed out.

[adriansuter]

I just tried to define a controller class with static methods and then use the class-method-name as string in the route. I thought, that the controller class would not be instantiated in this particular case - but I was wrong. As I had a DI container, the \Slim\CallableResolver asked the container if it knew anything about that class. The container said "of course" and then it created an instance of the controller class. So in the end, the resolved callable was an instance as first element, and a static method name as second element.

Is this the expected behaviour? Should we deny static methods as route handlers? Are there any benefits on allowing static methods?

Note: We would have to modify the callable resolver in order to be able to check for static methods. Something like:

$reflectionMethod = new \ReflectionMethod($class,$method);
if ( $reflectionMethod->isStatic()) {
    return [$class, $method];
}

[l0gicgate]

@adriansuter since static methods cannot be bound I would suggest against it. We should deny static handlers across the board. I think we need to distinctively draw a line here otherwise it gets confusing. I'm not a fan of using reflections to achieve things, it feels forced.

[adriansuter]

@l0gicgate Where do you think that should be mentioned in the docs? In PR https://github.com/slimphp/Slim-Website/pull/390 I added already the alert info about the non-supported static closures.

[l0gicgate]

That’s good enougj @adriansuter I’m closing this as wontfix.