Feature: Constructor-based Dependency Injection#470
Feature: Constructor-based Dependency Injection#470jamisonbryant wants to merge 7 commits intodereuromark:masterfrom
Conversation
Allow tasks registered in the DI container to be resolved with their constructor dependencies. Processor::loadTask() now checks the container first and falls back to direct instantiation. Io and Logger are injected via new setIo()/setLogger() setters on all paths. Resolves dereuromark#469
Cover three loadTask() paths: container-resolved task, fallback to direct instantiation, and constructor dependency injection with a service. Includes InjectedTask test fixture and updated task count.
Use regex matching instead of exact substring for 'not found' error messages, since macOS shells prefix with 'sh: command not found' while Linux uses 'not found' alone.
Add DI container resolution for task instantiation
Add constructor injection pattern to the DI Container section of the custom tasks docs, alongside the existing ServicesTrait approach.
Remove Io/Logger passthrough from InjectedTask constructor since the Processor handles injection via setters. Update test container registration to match.
Document constructor DI and simplify test fixture
| if ($this->container && $this->container->has($className)) { | ||
| $task = $this->container->get($className); | ||
| } else { | ||
| $task = new $className(); |
There was a problem hiding this comment.
you reverted the above new $className($this->io, $this->logger); - should stay as is.
There was a problem hiding this comment.
I am a bit confused, it was constructor args before:
new $className($this->io, $this->logger);
Why are we now using setters?
$task->setIo($this->io);
$task->setLogger($this->logger);
There was a problem hiding this comment.
even though those args are nullable I agree. It needs to be new $className($this->io, $this->logger); to be BC with what is already present for basically any task out there.
To get true constructor based DI here we'd need to refactor the Task class to do its constructor logic somewhere else (like a new ->initialize() method) and have a "clean" constructor.
This would be a major change.
There was a problem hiding this comment.
Sounds like pulling from the container is fine though:
if ($this->container && $this->container->has($className)) {
$task = $this->container->get($className);
There was a problem hiding this comment.
Sounds like pulling from the container is fine though
I was hoping to add support for the constructor-based DI use case, since that's in line with how core Cake works, but it seems that would be more difficult than I had originally anticipated.
3 participants
Feature: jamisonbryant#1
Docs: jamisonbryant#2