Skip to content
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

Enable custom Logger(s). #5022

Merged
merged 6 commits into from Jan 17, 2022
Merged

Enable custom Logger(s). #5022

merged 6 commits into from Jan 17, 2022

Conversation

weitzman
Copy link
Member

@weitzman weitzman commented Jan 15, 2022

Refs #4422 and implements the suggestion at consolidation/log#20 (comment). DrushLoggerManager is a slightly customized version of the Consolidation/Log/LoggerManager.

The changes in Runtime.php illustrate my guess as to when and how a site would swap a custom Logger for Drush's. I think that we could refactor our front controller (i.e. $runtime->run()) such that a site could replace that controller with one that swaps in own Logger. Is that a promising way to go?

@greg-1-anderson
Copy link
Member

There are a number of options, depending on how much flexibility / complexity we want to bring in.

Robo provides a RelativeNamespaceDiscovery mechanism for finding well-known class names (matching a pattern) via the autoloader tables. An example of this is the Robo plugin class discovery method. One option would be to find specific class names using this discovery method; a site could then hook into Drush behavior by declaring a class that matched the pattern. If we did this, rather than having a plurality of classes discovered this way (over time), I'd recommend we discover a single class which then implements specific interfaces to override behavior. e.g. if we discovered a DrushController class it might override an interface that declared a method for configuring the logger. If the controller implements that interface, then Drush could call the logger configuration method, passing in the logger manager.

An alternate plan wold be to allow sites to simply declare a command file or a hook file via the existing annotated command mechanism, and use a @hook init to ask for the logger manager directly from the Drush logger manager. Doing that has the advantage of not requiring a new hook / extension mechanism, but has the disadvantage that the logger would not be swapped in until very late in the bootstrap. Finding the DrushController could happen pretty early.

I didn't review the Drush initialization code to figure out where this could happen. The place you have selected already has a reference to all of the command files; this is late enough that we could check each command file for a specific interface, e.g. to configure the logger, and not wait for @hook init. I think that finding the DrushController via discovery could happen much earlier. If we want to try to allow this to happen as early as possible, we should probably make this part of Drush 12, and get rid of the secondary autoloader ($loader = $this->preflight->loadSiteAutoloader();) and prohibit Drush from operating on any Drupal site other than the one it shares an autoloader / vendor with.

There is also the question of what to do with log messages that happen before the logger is fully configured. We could ignore this problem, and just let early log messages go to the "wrong" logger, or formalize the wrongness by having a simple "early log" instance that just prints log messages. Alternately, we could have the logger manager cache all of the early logs, and dump them all to the custom logger after the configuration step is done. If we did this, we'd also want the shutdown handler to dump all of the early log messages via print should Drush exit before the logger is configured.

@weitzman
Copy link
Member Author

weitzman commented Jan 16, 2022

I went with @hook init and its working well except the ConsoleLogger that gets swapped in is logging to stdout instead of stderr (see the new test). If someone really does that with Drush, our redispatch commands like updatedb will break. Maybe our recommendation should be to add a logger instead of replacing Drush's?

Lets not worry about the log messages that happen before the swap.

@greg-1-anderson
Copy link
Member

Note that Drush by default does not use the Symfony ConsoleLogger directly, but instead uses the logger from consolidation/log, which replaces it. The Symfony logger only uses stderr for log levels that are "errors"; this is a common misunderstanding vis-a-vis the purpose of the stderr stream.

See https://github.com/symfony/console/blob/5.4/Logger/ConsoleLogger.php#L75

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants