Callable type hints about route

[philipobenito]

With all my testing, these situations should be fine, however, I will look in to it and ensure they work before the release of 2.0.0

—
Sent from Mailbox

On Sat, Sep 19, 2015 at 5:52 PM, Bernardo [email protected]
wrote:

May I suggest the removal of the callable type hints in the development branch?
When I provide [ SomeClass::class, 'methodName' ] as a handler (implying that SomeClass be resolved from the DI container), multiple warnings are raised saying that methodName should not be invoked as a static method (because it isn't static). I can work around the warnings by prepending the @ to the map statement, but I don't think that's a decent solution.

Worse even, when I provide 'SomeClass::methodName' as handler (which should be legal), a fatal error is raised.

Reply to this email directly or view it on GitHub:
#71

[philipobenito]

Is everything left as default for strategies etc? Trying to determine whether the warnings/errors are being triggered in Route or Container as there is a unit test for each of those callable types for the RequestResponseStrategy

[hannesvdvreken]

What if... you'd pass some kind of object that is a callable.

class ContainerCallable
{
    private $container;

    public function __construct(ImmutableContainerInterface $container)
    {
        $this->container = $container;
    }

    public function withInvokable($invokable)
    {
         $that = clone($this);
         $that->invokable = $invokable;
         return $that;
    }

    public function __invoke()
    {
        return $this->container->call($this->invokable);
    }
}


$callable = new ContainerCallable($container);

$router->get('/foo', $callable->withInvokable([SomeClass::class, 'methodName']));

Just exploring the use of decoration here.

[philipobenito]

Well, is_callable should actually return true on those types of callables even if they technically are not, running tests with E_ALL and they all pass. This is why I'm wondering if it's the ParamStrategy and it's failing in the container from some sort of oversight on my part?

I'll investigate more this evening. But these tests https://github.com/thephpleague/route/blob/develop/tests/Strategy/RequestResponseStrategyTest.php cover them, and pass on all supported versions. https://travis-ci.org/thephpleague/route/builds/79831704.

[philipobenito]

For a quick and dirty example.

<?php

class Something
{
    public function action()
    {
        return 'Hello, World!';
    }
}

$callable1 = [Something::class, 'action'];
$callable2 = 'Something::action';

var_dump(is_callable($callable1)); // true
var_dump(is_callable($callable2)); // true

function test(callable $callable) {
    return is_callable($callable);
}

var_dump(test($callable1)); // true 
var_dump(test($callable2)); // true

Valid across PHP5.5.0 to PHP7.0. Would also be valid in PHP5.4 without the ::class constant.

https://3v4l.org/gJroH

[philipobenito]

Oh wait, different results when I click the results link. My bad on strict standards, callable type hint !== is_callable. I'll fix this.

[thebigb]

Thanks, that's exactly what I meant.

You might also want to consider the third test I've added:

https://3v4l.org/hubg8

This simulates a scenario where non-class-name names are used in a DI container.

Removing the callable type hint should be sufficient to fix that, but you might want to incorporate it in to the unit tests.

[philipobenito]

As the callable type hint and is_callable are not the same (baffled as to why) I'll be removing the type hint throughout the tree and swapping for an is_callable check until the point where a solid callable is built within the dispatcher. That should cover all bases.

Thanks for picking up on this, it's annoying as my environment has been ignoring E_ALL for some reason that I can't figure out.

[thebigb]

Great!

Would it also be possible to remove the type hint from the StrategyInterface?

It would allow $controller to be null. This could be useful for dynamic routing strategies, where the controller is determined by the dispatcher based on other information from $this->request or $vars.

I can work around this by using a dummy callable, but it's not the most elegant solution.

[philipobenito]

I'd prefer it to stay in as by that point a callable has been built and it would require more checks in the default strategies to ensure it can be invoked if it is not there.