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

Make LoggerAwareTrait more helpful for integrating projects #36

Closed
wants to merge 2 commits into from

Conversation

davereid
Copy link

@davereid davereid commented Jul 7, 2015

There are a couple of things that I have to do to every project I use with the psr logger aware interface and traits, and end up having to create duplicate code every time. It would be nice if we could improve the usefulness of the aware interface and trait.

  • Add a getLogger() method that defaults to NullLogger().
  • Return static from setLogger() to allow it to be chained.

@davereid davereid changed the title Make LoggerAwareTrait more useful Make LoggerAwareTrait more helpful for integrating projects Jul 7, 2015
@davereid
Copy link
Author

davereid commented Jul 7, 2015

Any thoughts about possibly renaming this interface/trait to not break any backwards compatability? I really like Guzzle's naming of 'HasClientInterface' and 'HasClientTrait' so I'd propose 'HasLoggerInterface' and 'HasLoggerTrait'. Yea or nay?

@davereid
Copy link
Author

davereid commented Jul 7, 2015

Seems like I'm not the only one that finds having setLogger() return static useful: #27

@Crell
Copy link
Collaborator

Crell commented Jul 7, 2015

What purpose does getLogger serve? I can see chaining, but getLogger I don't understand.

@davereid
Copy link
Author

davereid commented Jul 7, 2015

If I use this trait, I still have to defensively code everywhere I want to use the logger:

if (!isset($this->logger)) {
  return false;
}
$this->logger->critical($message);

Whereas with this improved trait I know at least I can use it out of the box and not need to conditionally return every time (and defaulting to NullLogger has the same affect as just returning). Plus it gives a public method that other things can call on my object to log if needed.

$this->getLogger()->critical($message);

@Crell
Copy link
Collaborator

Crell commented Jul 7, 2015

Why not just set the logger to NullLogger in the constructor? That way you know there's always an instance there.

In hindsight, including the traits in the PSR was a mistake. It should have been interface only, and the traits in a separate Util package. The traits are part of the PSR itself, so modifying them is not really kosher.

@davereid
Copy link
Author

davereid commented Jul 7, 2015

I could set in the constructor, yes. We'd still be lacking a way for me to get the existing logger object from something else without the public method though.

@Crell
Copy link
Collaborator

Crell commented Jul 8, 2015

Nothing stops you from adding that method yourself, if appropriate. However, I don't think it's appropriate in the vast majority of cases. Services outside of the service in question should not know or care what it's logger is. If they do, I would consider that a code smell and probably a bug in your code.

@ManInTheBox
Copy link

I think defaulting NullLogger would be nice improvement here. Users of LoggerAwareTrait wouldn't need to check for available logger anymore, but simply call $this->getLogger() as explained above by PR creator. I can understand if addition of getLogger() method would mean BC break, but otherwise this could be really useful. At least please re-evaluate this PR :)

@holtkamp
Copy link

holtkamp commented Feb 2, 2020

Also encountered the "need" for a default getLogger() function and before opening a new PR I found this existing PR. In my projects they are as simple as:

    private function getLogger()
    {
        return $this->logger ?: $this->logger = new NullLogger();
    }

This way, the classes that use the LoggerAwareTrait do not need to check whether a Logger is set before logging messages. This makes it easier to separate concerns by dividing large chunks of functionality in smaller "library" classes.

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