App - Router Magic [BUG]

[geggleto]

I was going about creating a Provider Object to manage dependencies of certain "modules"/"endpoints" in a project and I came across a bit of an annoyance.

Not really paying attention I simply tried the following:

        $this->router->map(['get'], '/user{id:\d+}', ReadAction::class);
        $this->router->map(['put'], '/user{id:\d+}', UpdateAction::class);
        $this->router->map(['post'], '/user', CreateAction::class);

Which I had assumed would be fine... but if you do this; you end up with an error...

Catchable fatal error: Argument 1 passed to Slim\Handlers\Strategies\RequestResponse::__invoke() must be callable, string given, called in C:\xampp\htdocs\space\vendor\slim\slim\Slim\Route.php on line 325

The following is a fix:

        /** @var $getRoute Route */
        $getRoute = $this->router->map(['get'], '/user{id:\d+}', ReadAction::class);
        $getRoute->setContainer($this->container);

        /** @var $putRoute Route */
        $putRoute = $this->router->map(['put'], '/user{id:\d+}', UpdateAction::class);
        $putRoute->setContainer($this->container);

        /** @var $postRoute Route */
        $postRoute = $this->router->map(['post'], '/user', CreateAction::class);
        $postRoute->setContainer($this->container);

Which leaves me wondering with some questions

1. Why is the container required for each route?
2. Is there not a better way to manage dependencies? There has to be...

I believe it is to grab the instance of CallableResolver, as it might not be used on EVERY request.

[akrabat]

To resolve this, we would need the router to be container aware.

The easiest way to do that is:

1. Add a setContainer() method to Router that's not in the interface
2. Update the DefaultServicesProvider's 'router' closure to call the router's setContainer() but only after doing a method_exists check.
3. Update Router's map() method to call the route's setContainer() method (if it exists) after instantiating the route.
4. Remove the call to $route->setContainer() in the App 's map() method

[designermonkey]

Can we identify what the exact bug is here? Is it that the router/route can't resolve the callable/invokable string without the container?

[akrabat]

The problem is that @geggleto is using the Router's map rather than the App's one. The App's one sets the Container instance onto the route object so that the callable resolver works.

The proposed solution is to move the setting of the container to the route into the Router object rather than the App object.

[designermonkey]

Thanks, I understand now.

This takes me back to something I brought up a while back: we pass the container around too much.

I would be in favour of composing the router to include the resolver, but IMO, passing the container to it is not a good idea.

In the example provided, the code only requires that the invokable string is resolved, not the requirement for a container.

What are your thoughts?

[akrabat]

That would probably work. Want to work up a PR?

[designermonkey]

Ooof, I can try to find time. I know how crap that sounds.