Revert Revert "Mark Logger @final to discourage extension"

[lcharette]

This reverts https://github.com/Seldaek/monolog/commit/3cba75ec0926eec177a4a2cd6c977ecddd0fc7c1

The biggest reason I found to extend Logger is related to dependency injection.

Consider this scenario: I have two channels, $debugLogger = new Logger('debug'); and $securityLogger = new Logger('security');, each with their own Handlers, Formatters and/or Processors.

Now some of my class might require $debugLogger, while others requires $securityLogger.

MyClass:

public function __construct(Logger $debugLogger, PHPMailer $phpMailer) {
    // ... 
}

MyOtherClass:

public function __construct(Logger $securityLogger, SomeClass $someClass) { 
    // ... 
}

This might work when class are instantiated manually. However, this won't work when using PHP-DI Autowiring, as it won't be able to determine which Logger to inject.

I can't even distinguish both in my PHP-DI definition:

Logger::class  => function (StreamHandler $handler) {
    $logger = new Logger('debug');
    $logger->pushHandler($handler);

    return $logger;
},

// Doesn't work, can't register two with the same class !
Logger::class  => function (SymfonyMailerHandler $handler) {
    $logger = new Logger('security');
    $logger->pushHandler($handler);

    return $logger;
}

Also consider that while both loggers are the same PHP wise, and can be swapped without raising exception, they might provide very different service! I could make a mistake and inject debug in MyOtherClass and every security log would be passed to the debug handler.

My previous solution was to extend Logger for each use. For example :

class SecurityLogger extends Logger
{
}

class DebugLogger extends Logger
{
}

PHP-DI definition:

DebugLogger::class  => function (StreamHandler $handler) {
    $logger = new DebugLogger('debug');
    $logger->pushHandler($handler);

    return $logger;
},

SecurityLogger::class  => function (SymfonyMailerHandler $handler) {
    $logger = new SecurityLogger('security');
    $logger->pushHandler($handler);

    return $logger;
}

Now MyOtherClass can use SecurityLogger to get the proper channel :

public function __construct(SecurityLogger $securityLogger, SomeClass $someClass) { 
    // ... 
}

Same for MyClass with DebugLogger. Plus DebugLogger can't be injected by mistake in MyOtherClass.

However, having Logger marked as final disable this workaround. So unless there's another solution for this use case, I strongly believe this should not be marked final. I understand the annotation isn't enforced by PHP yet, but it still throws a warning when running PHPStan analysis, and I won't take the change of this annotation being changed to a real final class later.

Reference:

• https://github.com/Seldaek/monolog/pull/1827

[Seldaek]

You could either ignore the warning, or use composition instead of inheritance:

class SecurityLogger extends \Psr\Log\AbstractLogger
{
    public function __construct(private \Monolog\Logger $logger) {}
    public function log($level, string|\Stringable $message, array $context = []): void 
    {
        $this->logger->log($level, $message, $context);
    }
}

[lcharette]

In this case, the constructor would receive a vanilla version of Logger? The channel, handler and such should then be setup in the constructor?

[Seldaek]

You configure your loggers as usual, just define two new services with the monolog Logger instances wrapped inside SecurityLogger and DebugLogger classes, so the autowiring works.

[lcharette]

So something like this?

class SecurityLogger extends \Psr\Log\AbstractLogger
{
    public function __construct(private \Monolog\Logger $logger, private StreamHandler $handler) {
        $this->logger->withName('security');
        $this->logger->pushHandler($handler);
    }
    public function log($level, string|\Stringable $message, array $context = []): void 
    {
        $this->logger->log($level, $message, $context);
    }
}

Is this documented in the doc? If not, I would be happy to contribute to the doc if you're interested.

[Seldaek]

Well it really depends on your DI setup. I'm not familiar with PHP-DI, but IMO I would define the logger and configure it in a separate service, and just inject it explicitly in your SecurityLogger. But if you rather have the config of all handlers in there too sure that works.. Watch out that withName returns a new instance though so your sample above is borked.

In Symfony with the MonologBundle we have special autowiring rules to make it map Logger $securityLogger to the Logger instance of the security channel, so that is not a problem.

It's definitely not in the docs, and maybe it could be, but it seems fairly specific to a given DI implementation so I am not sure if it belongs here or rather in PHP-DI docs..

[lcharette]

I would define the logger and configure it in a separate service, and just inject it explicitly in your SecurityLogger.

I think this would add an unnecessary level of complexity. The securityLogger would then need to accept any logger (or interface), and hope it received the correct one. It's actually back to square one.

[lcharette]

Watch out that withName returns a new instance though so your sample above is borked.

Right. So it should be $this->logger = $this->logger->withName('security'); ?

[Seldaek]

Yes

[lcharette]

Thanks for your input. I linked the commit on our project to this conversation in case it can help someone else in the future.