4.x - Deferred callable as string without resolver

[adriansuter]

Dear reader

In DeferredCallable we can pass a string as callable. But the callable resolver is allowed to be null and therefore the __invoke will fail. Shouldn't we throw a RuntimeException if that happens? If yes, already in the constructor or later in the invocation?

[mapogolions]

I briefly looked - https://github.com/slimphp/Slim/blob/4.x/Slim/DeferredCallable.php#L40. It seems correct. Please, give a counterexample.

function add($a, $b) {
    return $a + $b;
}
$callable = new DeferredCallable('add');
echo $callable(10, 20) . PHP_EOL;

[adriansuter]

For example if the callable string is in Slim notation ClassName:method.

class Calculator
{
    function add($a, $b)
    {
        return $a + $b;
    }
}

$callable = new DeferredCallable('Calculator:add', new \Slim\CallableResolver());
echo $callable(10, 20).PHP_EOL;

$callable = new DeferredCallable('Calculator:add');
try {
    echo $callable(10, 20).PHP_EOL;
} catch (Error $e) {
    echo $e->getMessage().PHP_EOL;
}

This would output

30
Call to undefined function Calculator:add()

The first one is working as it has a callable resolver.

Although. I am not sure if this would ever happen.

[l0gicgate]

Shouldn't we throw a RuntimeException if that happens? If yes, already in the constructor or later in the invocation?

@adriansuter yes, let's throw a RuntimeException as soon as possible.

[adriansuter]

@l0gicgate Sure. Is this class actually used somewhere? I haven't found any references.

[l0gicgate]

@adriansuter it is not anymore. I think we can remove it altogether. It was an oversight when we merged the PSR-15 PR I believe.

[adriansuter]

If we remove that, could that bug someone - for example someone who used it already somewhere? Chances are small, but still.

[adriansuter]

Actually it is used:

Slim/tests/Routing/RouteTest.php

Lines 388 to 403 in 335b5fd

	public function testControllerMethodAsStringResolvesWithoutContainer()
	{
	$callableResolver = new CallableResolver();
	$responseFactory = $this->getResponseFactory();
	$deferred = new DeferredCallable('\Slim\Tests\Mocks\CallableTest:toCall', $callableResolver);
	$route = new Route(['GET'], '/', $deferred, $responseFactory, $callableResolver);
	CallableTest::$CalledCount = 0;
	$request = $this->createServerRequest('/');
	$response = $route->run($request);
	$this->assertInstanceOf(ResponseInterface::class, $response);
	$this->assertEquals(1, CallableTest::$CalledCount);
	}

Slim/tests/Routing/RouteTest.php

Lines 405 to 452 in 335b5fd

	public function testControllerMethodAsStringResolvesWithContainer()
	{
	$self = $this;
	$responseProphecy = $this->prophesize(ResponseInterface::class);
	$callableResolverProphecy = $this->prophesize(CallableResolverInterface::class);
	$callable = 'CallableTest:toCall';
	$deferred = new DeferredCallable($callable, $callableResolverProphecy->reveal());
	$callableResolverProphecy
	->resolve($callable)
	->willReturn(function (
	ServerRequestInterface $request,
	ResponseInterface $response
	) use (
	$self,
	$responseProphecy
	) {
	$self->assertSame($responseProphecy->reveal(), $response);
	return $response;
	})
	->shouldBeCalledOnce();
	$callableResolverProphecy
	->resolve($deferred)
	->willReturn($deferred)
	->shouldBeCalledOnce();
	$responseFactoryProphecy = $this->prophesize(ResponseFactoryInterface::class);
	$responseFactoryProphecy
	->createResponse()
	->willReturn($responseProphecy->reveal());
	$route = new Route(
	['GET'],
	'/',
	$deferred,
	$responseFactoryProphecy->reveal(),
	$callableResolverProphecy->reveal()
	);
	$request = $this->createServerRequest('/');
	$response = $route->run($request);
	$this->assertInstanceOf(ResponseInterface::class, $response);
	$this->assertEquals(1, CallableTest::$CalledCount);
	}

[l0gicgate]

@adriansuter we should either move it into the test mocks, or delete it an not use it at all. It shouldn't be in there and for people using it, why? you can already defer any route/middleware callable.

[mapogolions]

@adriansuter and @l0gicgate I created a draft PR. It shows how the DeferredCallable class can help us.

[adriansuter]

Seems to be a nice idea. What do you think @l0gicgate?

[l0gicgate]

If we use DeferredCallable again, then we make MiddlewareDispatcher depend on CallableResolverInterface which will make it harder to decouple that interface as proposed in #2785

I don't think we need it anymore. Change my mind 😂

[adriansuter]

Right. So I will close #2807, as soon as I can as it is not needed anymore.

[BusterNeece]

@l0gicgate Agh, this is what I get for being on effectively 4.x-dev-master...I just pulled this update down and found this was gone, and while I'm not 100% surprised, I actually was using it in my code. :(

Almost certainly this is a case of XKCD 1172, and I don't blame you guys for getting rid of it for Slim itself, but it was a nice little utility class to have to fulfill the same purpose in other places that don't take advantage of Slim's CallableResolver (in my case, Symfony's EventDispatcher).

[l0gicgate]

Sorry @SlvrEagle23, it should be fairly easy to re-implement in your code base though if you need it though.