4.x - Support For Slim Callables in MiddlewareDispatcher & Container Closure Autobinding

[l0gicgate]

This PR adds support for Slim callables when adding deferred style middleware:

$app->add('MyClass:customMethod`);

Fix for: #2770 (comment)

This PR also adds support for middleware callable Closure container auto-binding. Now callables are automatically bound to the container as they were in Slim 3.
Fix for: #2770 (comment)

[coveralls]

Coverage remained the same at 100.0% when pulling 65c9325 on l0gicgate:MiddlewareDispatcherCallableResolver into 7ee5627 on slimphp:4.x.

[BusterNeece]

@l0gicgate I appreciate the quick response and the corresponding code.

However, this doesn't actually allow the CallableResolver passed to Slim\App to serve as the handler of the MiddlewareDispatcher as well. It just duplicates that functionality inside the MiddlewareDispatcher but hard-codes it in.

It would be much more flexible for users if the CallableResolver itself was just injected in from Slim\App, which already has it for the routes, and called directly to resolve the callable. That would both allow the "class:method" mechanism used by Slim 3 but would also allow any custom resolution methods that the user introduces.

[adriansuter]

@l0gicgate if we pass the callable resolver to the middleware dispatcher while creating the instance in the App constructor, would that be a BC?

If not, I would pass the callable resolver and use it in the middleware dispatcher, as @SlvrEagle23 suggested. Or am I missing anything?

[Hill-98]

This is great, greatly reducing the work that needs to be done to upgrade 4.x.

[adriansuter]

Of course the resolve() method of the callable resolver needs to be adapted such that in case of an instance of MiddlewareInterface, the default method would be process().

[l0gicgate]

The problem is that we can't use CallableResolver inside of the MiddlewareDispatcher. The logic is ever so slightly different, it's not just callables we are resolving but also MiddlewareInterface.

This PR is just so we can at least add back the ability to use Slim style callables Class:method pattern and bind containers to closures.

Other than that, there isn't much more that can be done without overriding the entire MiddlewareDispatcher component and drop in your own. We may eventually add that to App constructor if it becomes widely requested.

[BusterNeece]

@l0gicgate If you use the CallableResolver and add in a special condition for if the resolved instance is a MiddlewareInstance (changing its default method to 'process', just as is already done for another instance type in the CallableResolver) I don't see in what way this wouldn't be 100% compatible with the existing addCallable functionality used in the MiddlewareDispatcher.

[adriansuter]

@SlvrEagle23 Agree.

@l0gicgate Do you think we need a MiddlewareDispatcherInterface that can be injected into the App constructor?

[l0gicgate]

@adriansuter I suppose we do, that’s the only component at the moment that you can’t drop in.

@SlvrEagle23 the problem with adding a condition for MiddlewareInterface in CallableResolver is that we’re making assumptions that because the class implements the interface it is single purposed and only used for handling middleware (same goes with how RequestHandlerInterface has a condition in there. I highly dislike that. There should be a clear separation between resolving route callables/classes and middleware callables/classes.

We have to find a way to separate those concerns without BC changes (almost impossible) and without assumptions. It’s a hard problem to solve.

[adriansuter]

Could we add an optional argument to the resolve method? Something like a mapping of instances and their corresponding methods. By default the map would be

[RequestHandlerInterface::class => 'handle']

But in the middleware dispatcher, the map would be

[MiddlewareInterface::class => 'process']

[l0gicgate]

@adriansuter we can, but that breaks the interface.

[adriansuter]

Not good.

[adriansuter]

Then I guess your "duplication of code" is actually the only possible solution. Except maybe another new MiddlewareCallableResolver and its interface that would also be injected into the App and then passed to the middleware dispatcher. So users could write their own resolvers.

[BusterNeece]

@adriansuter If you accept the premise that it's unacceptable to add a specific per-interface method overwrite to the CallableResolver (which is already done in this codebase as-is), then that would be the case.

I know the talk about "BC breaks" is referring to the just-released Slim 4, but middleware not using the same CallableResolver as routes is itself a "BC break" from Slim 3, and it's that jump that I suspect will throw the most people off.

I've already worked around the problem, but I feel like the duplication of code that is proposed in this solution is inelegant and completely closed to developer modifications, whereas the alternative both avoids duplication of code and allows for developer customization. I'm not a developer on the Slim project but as a senior PHP developer I would hope any framework I used would prioritize the latter.

[adriansuter]

@SlvrEagle23 If we simply add per-interface conditions, then we would make assumptions. What happens, if a class implements multiple instances - then the callable resolver has to decide, which method should be called.

[l0gicgate]

@SlvrEagle23 then as a senior developer you should also know that separation of concerns is just as important and that further worsening the logic inside CallableResolver is not a solution but just a quick fix which may create other bigger problems down the line.

I addressed this in the PSR-15 PR when we were having the discussions and was shrugged off and now we’re having to deal with it post release, it’s rather frustrating to be honest.

The only real solution is to modify the CallableResolver interface and separate the methods.

There should be 3 methods:

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

[BusterNeece]

@l0gicgate If I had to choose between the two approaches, I'd rather see three interfaces (CallableResolverInterface, RouteCallableResolverInterface and MiddlewareCallableResolverInterface) with standalone implementations inside Slim, but sharing a common resolve function (basically Route and Middleware interfaces would just inherit the parent interface for method definitions) as the current CallableResolver has, so you could, if you wanted, build out a single class that supported all of them.

[l0gicgate]

@SlvrEagle23 separating all 3 now adds 3 more components to the App’s constructor, that seems a bit much, I understand why but sometimes practicality is more important. The constructor is already getting bloated as is and if we have to add MiddlewareDispatcherInterface on top of that, then it becomes even more bloated. I think we can be reasonable and combine all 3 methods in one interface.

[BusterNeece]

@l0gicgate Fair enough. That works just as well in everyday use.

[adriansuter]

@l0gicgate @SlvrEagle23 Agree. Unfortunately a BC. But better sooner than later.

[l0gicgate]

@adriansuter I think we can merge this in the mean time, release 4.1.0. Then we can introduce a BC in 4.2.0, nobody will suffer the BC unless they implemented their own CallableResolver (not that likely).