Add a Multichannel DatabaseLogger #12
Conversation
There was a problem hiding this comment.
See my comments in the review.
If you want to run the tests to see if it works you can run the following command:
composer testCurrently, Travis indicates that this commit introduces errors could you check this as well?
(Every pull request/commits are tested by Travis to see if they are valid or not.)
| foreach($channels as $channel){ | ||
| $this->loggers[$channel] = new Logger($channel); | ||
| $logsDir = $databasePath . "/" . Database::LOGS_DIR_NAME; | ||
| $this->pushHandler(new RotatingFileHandler($logsDir . "/".$channel."log", Logger::DEBUG)); |
There was a problem hiding this comment.
Since we are in PHP7 you can make use of the new feature which allows passing variable names inside double-quoted strings:
"$logsDir/$channel.log"I know initially I had set the Logger Level to debug, but could you change it to INFO?
Thank you
There was a problem hiding this comment.
Also since it no longer relies on the Monolog Logger class's parent constructor, it should maybe drop the inheritance of the Monolog logger and instead implement the standard PHP Psr\LoggerInterface.
However, since the signature of the log method from that interface does not support the Channel Loggers, a new method should be added:
DatabaseLogger::logWithChannel(string LogLevel, string $message, array $context = [], LoggerChannel $channel = null)| public function log(string $level, string $message, array $context = [],string $channel = LoggerChannel::__DEFAULT): void | ||
| { | ||
| if (!$this->logger) { | ||
| return; | ||
| } | ||
|
|
||
| Assertion::keyExists($this->logger->loggers, $channel, "Unsupported channel '$channel'"); | ||
| $context['database_root'] = (string)$this->database->getPath(); | ||
| $this->logger->log($level, $message, $context); | ||
| $this->logger->loggers[LoggerChannel::__DEFAULT]->log($level, $message, $context); | ||
| if ($channel === LoggerChannel::__DEFAULT) { | ||
| return; | ||
| } | ||
| // Log in channel specific logger | ||
| $this->logger->loggers[$channel]->log($level, $message, $context); | ||
|
|
There was a problem hiding this comment.
The logging logic/implementation should really not be in the engine, it is not it's responsibility. It shouldn't know how to log, instead, it should only know that it is possible to log by calling the Logger.
The logger should contain the implementation details.
Therefore it would be better to implement the log method in the logToChannel method of the logger I discussed earlier.
Also since LoggerChannel is now a type that handles validation internally, we could get rid of the Assertion call you made.
By doing this log(string $level, string $message, array $context = [], ?LoggerChannel $channel = null)
Unfortunately PHP does not allow custom default argument values for cutom types.
To circumvent this, usually, the following strategy is used:
set a default argument value of null
and then check for null in the method
if(!$channel) $channel = new LoggerChannel(LoggerChannel::__DEFAULT);(again this code should go in the logger, not the engine)
| /** | ||
| * LoggerChannel | ||
| */ | ||
| class LoggerChannel extends BasicEnum |
There was a problem hiding this comment.
Good!
I have done some new devs on the branch to reintroduce queries, it would be great to be able to log to a query channel:
const QUERY = 'query'Good on the __DEFAULT keyword circumvention.
add command and event log