-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A commands logger is ambigious #5850
Comments
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 |
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?The text was updated successfully, but these errors were encountered: