Allow command classes to set their own logger channel #5963
Conversation
|
Is there a way that I could test this change in a phpunit test run in gitlab_templates pipeline? Happy to help investigate, but not sure how to do it. |
|
@jonathan1055 not sure TBH. I haven't ever php unit tested a module in a drush context. Not sure if that's a thing. |
|
The contrib module provides drush commands, and I have a phpunit test that checks the commands work as expected. I have had to disable that test group for a while. It is still working correctly on DrupalCI phpunit test, but not in gitlab pipelines. |
src/Commands/DrushCommands.php
Outdated
| * | ||
| * @param LoggerInterface $logger | ||
| */ | ||
| public function setLoggerIfEmpty(LoggerInterface $logger): void |
There was a problem hiding this comment.
Is this used by anything other than the service manager? If not, I think it would be better to just move the condition to inflect, and omit the extra trivial function in the base class.
There was a problem hiding this comment.
We can't do that, the logger variable is protected and can't be accessed from the service manager.
There was a problem hiding this comment.
what about making logger() public and using it here? maybe only on 13.x branch due to BC.
if (!$object->logger()) {
$object->setLogger($container->get('logger'))
}
There was a problem hiding this comment.
You can tell whether the logger variable is set via
drush/src/Commands/DrushCommands.php
Line 84 in 95e0acf
There was a problem hiding this comment.
That method is protected, so we can't call it from outside, i.e. from the service manager.
src/Runtime/ServiceManager.php
Outdated
| @@ -452,7 +452,7 @@ public function inflect(DrushContainer $container, $object): void | |||
| $object->setConfig($container->get('config')); | |||
| } | |||
| if ($object instanceof LoggerAwareInterface) { | |||
There was a problem hiding this comment.
OK, I guess we can't check $object->logger() here as I suggested unless we make logger() public. Did not consider impact of that, I think that method is defined in another project.
There was a problem hiding this comment.
That method is in Drush so we can change it but only for Drush 13 due to BC. I'll open a new PR for that, and fixup this one for Drush 12.
There was a problem hiding this comment.
I went the other way around. The Drush 12.x PR is at #5980. This PR will be for 13.x
src/Runtime/ServiceManager.php
Outdated
| @@ -452,7 +452,7 @@ public function inflect(DrushContainer $container, $object): void | |||
| $object->setConfig($container->get('config')); | |||
| } | |||
| if ($object instanceof LoggerAwareInterface) { | |||
| $object->setLogger($container->get('logger')); | |||
| $object->setLoggerIfEmpty($container->get('logger')); | |||
There was a problem hiding this comment.
setLoggerIfEmpty() is not a method of LoggerAwareInterface, so there is a mismatch between the instanceof above and the usage here.
weitzman
requested review from
davereid,
Sutharsan,
pfrenssen,
damiankloip,
DieterHolvoet,
claudiu-cristea and
bircher
as code owners
|
Now this is a small, helpful change for 13.x. Thanks everyone. |
Following this thread on Drupal Slack the ServiceManager inflection overrides the logger channel of a Drush command, if that came with its own channel to e.g. log to the Drupal channel e.g. in watchdog.
With this PR, the service manager would only set its logger if the command had not defined their own before.