Open eiriksm opened 7 months ago
I didn't dive into the code, but I just found the method name "logger" confusing as I expect that it should at least send the message to the loggerchanel so I can find it into my defined log target (file, dblog, syslog, ...).
See this PR and especially these lines https://github.com/drush-ops/drush/pull/5022/files#diff-99d5f4f635c30767edca2077f00a804bec2c283565da05f4a864e9d45295d690R37-R54
I dont think this is documented anywhere yet.
I think the most basic level of documentation would be to change the signature of LoggerAwareTrait
so that it only takes DrushLoggerManager
and never LoggerInterface
, because it kinda works, and kinda doesn't as-is, leading to broken code.
Describe the bug The command class uses the LoggerAwareTrait from the psr/log package (plus the interface DrushLoggerManager). This trait has the method
setLogger
which accepts aLoggerInterface
instance. So far, this is very straight forward and just what I expect.However, many commands you try to run will try to get the logger via the
::logger
method. This method is required to either return aDrushLoggerManager
ornull
.This means that if you try to set a logger on a drush command class that is not a
DrushLoggerManager
(for example, one of the psr/log compatible loggers from monolog) you will get a type error (example taken from setting a monolog logger as the logger):This is a bit confusing, even if understandable (for example, seems we need the
::success
method). On the one hand, we are implementing the "contract" specified inDrushLoggerManager
but for someone following this contract, it will not work.To Reproduce
Expected behavior It would run the queue, and log to my logger.
Actual behavior
Workaround I guess I could somehow extend DrushLoggerManager for the purpose of logging the output. Or run drush within a subprocess and fetch the output.
System Configuration
12.4.3.0
)Additional information Not sure if this is fixable in a good way. But maybe we should not use the
::setLogger
method from the psr/log package to avoid the confusion?