Slim-Bridge: Custom implementation of CallableResolver breaks deferred middleware

Can I get 1 or 2 confirmation that https://github.com/PHP-DI/Slim-Bridge/pull/71 is good for you?

Just to be clear, please don’t review #71 and nitpick on formatting or other things like that (the PR has been opened for a while and I would understand that the author may not be around to update it).

I’m just trying to make sure that #71 is an acceptable solution. If it is, I can merge and release.

workaround used in our projects:

<?php

namespace MyProject\DI;

use Invoker\CallableResolver as InvokerCallableResolver;
use Psr\Container\ContainerInterface;
use Slim\CallableResolver as SlimCallableResolver;
use Slim\Interfaces\AdvancedCallableResolverInterface;

class CallableResolver implements AdvancedCallableResolverInterface
{
    private $callableResolver;
    private $slimCallableResolver;

    public function __construct(ContainerInterface $container)
    {
        $this->callableResolver = new InvokerCallableResolver($container);
        $this->slimCallableResolver = new SlimCallableResolver($container);
    }

    public function resolve($toResolve): callable
    {
        return $this->callableResolver->resolve($toResolve);
    }

    public function resolveRoute($toResolve): callable
    {
        return $this->resolve($toResolve);
    }

    public function resolveMiddleware($toResolve): callable
    {
        return $this->slimCallableResolver->resolveMiddleware($toResolve);
    }
}

...

use MyProject\DI\CallableResolver;
use Slim\Factory\AppFactory;
use Slim\Interfaces\CallableResolverInterface;

class MyAppFactory {
    public static function createSlimApp(ContainerInterface $container)
    {
        $callableResolver = new CallableResolver($container);
        $container->set(CallableResolverInterface::class, $callableResolver);
        ...
        return AppFactory::createFromContainer($container);
    }
}

Thank you all, and @Rarst!

I’ve tagged https://github.com/PHP-DI/Slim-Bridge/releases/tag/3.2.0

@gravataLonga sometimes that is really problematic. For instance, if a middleware creates an object from data in the request and puts it into the container. Such object creation may not be entirely ideal, but sometimes it is necessary.

The Slim-Bridge fundamentally changes Slim functionality by preventing the usage of middleware as class strings to support invoker features. This is not clearly documented and, in my opinion, not necessary. The only time this is useful is when using anonymous functions as route handlers.

Yes, it’s actual problem

Having figured out what the differences are, this should work with Bridge to add and resolve PSR middleware from a class name (array syntax causes deprecation notices since something tries to call it as static method):

$app->add(Middleware::class . '::process');

A valid PSR15 middleware being added via $app->add(Middleware::class) should just work though just like regular slim and we shouldn’t need to add magic strings as decoration, that’s a code smell and is why it should be avoided IMO.

I fail to understand what the struggle here is.

The struggle is that the feature that works in Slim is broken by Bridge.

That is a concrete problem, regardless of opinions on magic of the feature itself and workarounds for it.

I stumbled upon this issue as well in my current project, and I adapted @kakenbok’s code to a pull request which could hopefully fix this issue.

@dakujem the problem is this library don’t preserver behavior of slim that is descrive here:

https://github.com/PHP-DI/Slim-Bridge/issues/51#issue-496024227

If i have more time will try to explain more later.

@mnapoli sure thing.

CallableResolver is used for two different things in Slim:

1. In routing, to determine how to invoke the target of a route. This is the part that Slim-Bridge cares about.
2. In middleware dispatching, to determine how to call a middleware. This the part that Slim-Bridge has inadvertently broken.

When calling add($middleware), if the middleware is an object, no problem. If the middleware is already callable, no problem. If the middleware is a string, it gets more complicated. In particular, this check for AdvancedCallableResolverInterface allows for specific detection of MiddlewareInterface, by creating an assumption that the given string is a classname, with a process() method.

Implementing AdvancedCallableResolverInterface seems like it would solve this problem.

However, I don’t really understand why CallableResolver is replaced at all… just to provide Invoker capabilities for middleware? I personally don’t need, or want that functionality. The routing strategy in ControllerInvoker is enough for my use cases.

I would also add that we should try to have the bridge not change default slim usage or at least document the issue clearly.

Even if documentation existed, I think that using $container->get(...) on every middleware call is awkward, especially when default slim can just resolve the middleware with MyMiddleware::class by default anyway and is documented as doing so in the framework docs.

This issue tripped me up for some time before finding this thread a while ago and almost made me uninstall the bridge and make something custom which would have been silly now that I know the issue but even still, it feels like this is unexpected behaviour since the bridge should be able to be built and then used like a normal slim app therein.

Just to make it clear, it is very much possible to use middleware on Slim …

// instead of
$slim->add(Middleware::class);
// do this
$slim->add($container->get(Middleware::class));

Don’t always rely on magic.

P.S.: If you want to make the middleware lazy loaded, try this approach:

$slim->add(new GenericMiddleware(function(Request $request, Handler $next) use($container): Response {
    $mw = $container->get(Middleware::class);
    return $mw->process($request, $next);
}));

(see this package or use your own implementation)

Edit: in fact, you don’t even need the GenericMiddleware with Slim, an anonymous function will do.

@mnapoli sure thing.

CallableResolver is used for two different things in Slim:

1. In routing, to determine how to invoke the target of a route. This is the part that Slim-Bridge cares about.
2. In middleware dispatching, to determine how to call a middleware. This the part that Slim-Bridge has inadvertently broken.

When calling add($middleware), if the middleware is an object, no problem. If the middleware is already callable, no problem. If the middleware is a string, it gets more complicated. In particular, this check for AdvancedCallableResolverInterface allows for specific detection of MiddlewareInterface, by creating an assumption that the given string is a classname, with a process() method.

Implementing AdvancedCallableResolverInterface seems like it would solve this problem.

However, I don’t really understand why CallableResolver is replaced at all… just to provide Invoker capabilities for middleware? I personally don’t need, or want that functionality. The routing strategy in ControllerInvoker is enough for my use cases.

@mnapoli There is a big problem - AdvancedCallableResolver will be removed in Slim 5 (see issue). So the merge you have done will soon become obsolete.

And the performance problem I described above still remains.

the PR has been opened for a while and I would understand that the author may not be around to update it

Am around, if any tweaks needed. 😃

The other option is from @solventt which would also work nicely IMO since it addresses topics upstream too.

I’m ok with anything that fixes the issue but I’d also like it to be a considered fix from the maintainer side and not just a “that one will do” choice.

I trust @mnapoli to choose the right PR for the ecosystem and if #71 is it, I’m on board. 🚀

@mnapoli yeah, I think #71 is best option, because BC compatibility.

@gravataLonga there have been at least 3 solutions contributed and proposed but we need a maintainer such as @mnapoli to actually decide on one option that works best for the eco system and merge. Basically just waiting for that.

please do #70.

I really like the simplicity of #70.

PRed a little more backwards compatible take on bringing these things back to parity with Slim in #71, testing/feedback welcomed. 😃

I opened an issue upstream as well, because arguably if the intent is to let a resolver throw an exception and proceed to fallback code then it should handle this case without issue.

The problem is Invoker exception being thrown isn’t caught upstream because it doesn’t extend RuntimeException expected there.

@dakujem sorry, if i sound “arrogant” but i’m not english native. But yeah, i’m agree with you. I prefered transparency and flexibility over “magic”.

I did not mean it disrespectfully.

By “magic” I mean something that makes your dev life easier, but usually comes at cost. Transparency and flexibility are the most frequent tradeoff.