8000 [DX] Null Object pattern for 'LoggerInterface $logger' · Issue #15594 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DX] Null Object pattern for 'LoggerInterface $logger' #15594

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.

Sign up for GitHub

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

Jump to bottom
Closed
AlexDpy opened this issue Aug 23, 2015 · 6 comments
Closed

[DX] Null Object pattern for 'LoggerInterface $logger' #15594

AlexDpy opened this issue Aug 23, 2015 · 6 comments

Comments

@AlexDpy
Copy link
Contributor
AlexDpy commented Aug 23, 2015

I noticed that most of the time in Symfony, when a LoggerInterface is injected into a service and when it can be null, we do not use the null-object pattern.

Psr\Log has a NullLogger that can be used in this case. It says :

/**
 * This Logger can be used to avoid conditional log calls
 *
 * Logging should always be optional, and if no logger is provided to your
 * library creating a NullLogger instance to have something to throw logs at
 * is a good way to avoid littering your code with `if ($this->logger) { }`
 * blocks.
 */

https://github.com/php-fig/log/blob/master/Psr/Log/NullLogger.php

If found 60 occurences of "if (null !== $this->logger)" in Symfony. Many of these could be avoided and the code could be clearer.

Before:

    protected $logger;

    public function __construct(LoggerInterface $logger = null)
    {
        $this->logger = $logger;
    }

    public function foo()
    {
        if (null !== $this->logger) {
            $this->logger->info('foo');
        }
    }

    public function bar()
    {
        if (null !== $this->logger) {
            $this->logger->info('bar');
        }
    }

After:

    protected $logger;

    public function __construct(LoggerInterface $logger = null)
    {
        $this->logger = null === $logger ? new NullLogger() : $logger;
    }

    public function foo()
    {
        $this->logger->info('foo');
    }

    public function bar()
    {
        $this->logger->info('bar');
    }

If you think it's a good idea, i can provide a PR.
@dosten
Copy link
Contributor
dosten commented Aug 24, 2015

This idea was proposed in a PR, but rejected for performance reasons. (see #14682)

@javiereguiluz
Copy link
Member

@dosten I don't think that performance degrades too much by using this pattern. I've added a comment in the related PR to ask Symfony deciders to reconsider it.

@linaori
Copy link
Contributor
linaori commented Aug 24, 2015

I would recommend the following syntax:

$this->logger = $logger ?: new NullLogger();

@nicolas-grekas
Copy link
Member

I don't think this should be labelled DX at all: DX does not apply to the internal implementation, only to the interface we provide.
👎 here: our internal implementation has to be as fast as possible. We don't know how people use our code.

@fabpot
Copy link
Member
fabpot commented Aug 24, 2015

As it does not bring any "value", I'm closing this ticket.

@pilot911
Copy link

using null object is good solution... strange last comment from fabpot

@chalasr chalasr mentioned this issue Jun 28, 2017
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @javiereguiluz @nicolas-grekas @dosten @linaori @pilot911 @AlexDpy
0