Container not called properly for "invokable" controllers

[Arcesilas]

In Route, line 97:

        if (is_string($callable) && method_exists($callable, '__invoke')) {
            $callable = $this->resolveClass($container, $callable);
        }

When resolving a class, the container may return a concrete which is quite different from the abstract. For example, I use a ControllerFactory which has a default namespace. When the Route::getCallable() method passes PageController to the factory, it automatically adds the default namespace and then resolves PageController to App\Http\Controllers\PageController and returns the appropriate object.

If Route considers the class specified does hold an __invoke method, then resolution by the container is useless. Actually, the container should be asked to resolve the class and then Route should check if the object has an __invoke method. Or maybe am I missing something... ?

P.S.: sorry for the first version of the issue, I thought I had opened a new tab in my browser, and an empty issue was published when I hit enter...

[philipobenito]

This is an interesting one, container sees any callable as a callable that should resolve the concrete but I see how that might cause problems in this situation. It's going to require a bit of thought but I'm quite busy this week so will recreate the issue and give it some attention next week.

[Arcesilas]

A simple workaround is to specify a method, so it's still usable. Thanks for your attention and your quick reply

[Arcesilas]

Hi,

If you ever have a little time to have a look at this issue... :)

I've just tried to add this before line 97 (before the sample I quoted in original post):

        if (is_string($callable) && $container->has($callable) && method_exists($c = $container->get($callable), '__invoke')) {
            $callable = [$c, '__invoke'];
        }

That's ugly, it's just to try, and it seems to work as expected: the controller is resolved before checking it has an __invoke method.

[Arcesilas]

Another use case which fails (event with the small monkey patch I've just mentioned): if the alias is a string which resolves to a callable array...

For example:

$container->add('defaultController', [PageController::class, 'showPage']);

$router->addPatternMatcher('path', '[^?]+');
$router->get('/{path:path}', 'defaultController');

Obviously $container->get('defaultController') resolves to [PageController::class, 'showPage'] which is a valid callable and could be used as a controller. Yet, it will not be resolved by the container.

Shouldn't we check in the first place if the container holds the alias if it does not contain ::?

Maybe my case is very specific and very rare. What I want to do is allow a plugin to define the controller used for a given route, via the container. I use a custom DefinitionAggregate which allows definitions to be replaced. Anyway, I think the route should always try to resolve a string from the container before considering it's a class name.

[philipobenito]

@Arcesilas sorry it's taken me so long to properly look at this, I'm currently trying to solve it. I don't think it's fixable in Route, but I've been playing around with having a wrapper in Container, that would not consider a class with __invoke as a factory, and just resolve the class.

So it would be something like this.

<?php

$container->add(ClassWithInvoke::class, new CallableClass(ClassWithInvoke::class));

$router->get('/path', ClassWithInvoke::class);

Does that make sense and would that solve your problem?

[philipobenito]

As for the resolving of aliases, I'd prefer to keep Route without any knowledge of possible aliases in a container, but will look at whether it is something I can do cleanly, no promises though :-)

Edit: actually, allowing aliases is tidier :-)

[philipobenito]

@Arcesilas so it turns out I'd already solved this, I'm sorry I didn't realise this before.

This will already work:

<?php

use League\Container\Argument\ClassName;

$container->add(ClassWithInvoke::class, new ClassName(ClassWithInvoke::class));

$router->get('/path', ClassWithInvoke::class);

I'll allow aliases to be resolved in the release I have coming out this week.

I'll close this for now, if it still doesn't solve your issue I can re-open, but as I understand it, that solves it.

[Arcesilas]

Hi. It seems the problem is still present.

Before throwing the Exception, the getcallable() method should ask the container (if any defined) if it handles the requested alias.

As I said in the original post, when using a delegate container as controller factory which automatically prepends a namespace, the controller is not a valid callable.

Maybe something like:

        if (null !== $container && $container->has($callable)) {
            $callable = $this->resolveClass($container, $callable);
        }

        if (is_string($callable) && method_exists($callable, '__invoke')) {
            $callable = $this->resolveClass($container, $callable);
        }

We first ask the container. If it handles the alias, get the concrete. Then, check the __invoke() method exists. It may seem a little redundant, but the container may return a string when requesting an alias...

Not fully tested, but works for me. I will take some time in the next few days to check if tests still pass.