Correct method of DI with regards to route.thephpleague.com

Question

I received feedback on code of mine that said I was using Dependency Injection incorrectly:

You are using the DI, but you are not utilizing it anywhere except for Request and Response. The following 10 lines will always instantiate the objects, even though you are never using them.

On these lines I have things like

$router->map('GET', '/tokens/{id}', [new APIController($server, $tokenRepository, $logger), 'get']);
$router->map('GET', '/tokens', [new APIController($server, $tokenRepository, $logger), 'list']);
....
$response = $router->dispatch($container->get('request'));

Which according to the documentation, seems to be the correct way to do it. Bootstrap.php:

$container  = new Container;
$logger = new Logger('book');

$tokenRepository = new RedisTokenRepository($predis, $logger);

$container->add(Logger::class);
$container->add(Server::class);
$container->add(TokenController::class)->addArguments([Server::class, $logger]);
$container->add(APIController::class)->addArguments([Server::class, $tokenRepository, $logger]);

$strategy = (new ApplicationStrategy)->setContainer($container);
$router   = (new Router)->setStrategy($strategy);

$router->map('GET', '/', [new Acme\APIController, 'someMethod']);

Controller

class APIController
{
    private $server;
    private $tokenRepository;
    private $logger;

    public function __construct(
        Server $server,
        TokenRepositoryInterface $tokenRepository,
        LoggerInterface $logger
    )
    {
        $this->server = $server;
        $this->tokenRepository = $tokenRepository;
        $this->logger = $logger;
    }

Can anyone explain what I'm doing wrong?

Answer 1

The feedback most likely refers to you actually instantiating the Classes for the route definitions; and you are manually passing the dependencies to them.

$router->map('GET', '/tokens/{id}', [new APIController($server, $tokenRepository, $logger), 'get']);

note the new keyword

with this approach, while parsing the file the system will create the instances of the APIControllers - yes several.

you should either use

$router->map('GET', '/', [APIController::class, 'get']);

or

$router->map('GET', '/', 'APIController::get');

this way the router will only instantiate them once they are needed

It seems your method needs $server, $tokenRepository, $logger, define them as the documentation describes them on the Controller class