Calling Developers!
We are reenergizing our code contribution process! Learn More

What are the Slack Archives?

It’s a history of our time together in the Slack Community! There’s a ton of knowledge in here, so feel free to search through the archives for a possible answer to your question.

Because this space is not active, you won’t be able to create a new post or comment here. If you have a question or want to start a discussion about something, head over to our categories and pick one to post in! You can always refer back to a post from Slack Archives if needed; just copy the link to use it as a reference..

I am wondering why the `LoggerTrait::getLogguer()` says that it can return a null πŸ€”

Options
Chemaclass
Chemaclass Tech Lead Spryker Solution Partner Posts: 213 πŸ§‘πŸ»β€πŸš€ - Cadet
edited April 2021 in Slack General

I am wondering why the LoggerTrait::getLogguer() says that it can return a null πŸ€”
https://github.com/spryker/log/blob/master/src/Spryker/Shared/Log/LoggerTrait.php#L17

/**
 * @return \Psr\Log\LoggerInterface|null πŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆ
 */
protected function getLogger(?LoggerConfigInterface $loggerConfig = null)
{
    return LoggerFactory::getInstance($loggerConfig);
}

when inside the LoggerFactory you are actually creating a logger in case it doesn’t exist (see: createInstanceIfNotExists)
https://github.com/spryker/log/blob/ad13572ac0b671bebfd64a41216719e52e7cf703/src/Spryker/Shared/Log/LoggerFactory.php#L33

/**
 * @return \Psr\Log\LoggerInterface|null πŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆ
 */
public static function getInstance(?LoggerConfigInterface $loggerConfig = null)
{
    if ($loggerConfig === null) {
        if (!static::$loggerConfig) {
            static::$loggerConfig = static::createLoggerConfig();
        }

        $loggerConfig = static::$loggerConfig;
    }

    return static::createInstanceIfNotExists($loggerConfig); πŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆ
}

And that function explicitly says (in its PHPDoc) that it wont return a null:
https://github.com/spryker/log/blob/ad13572ac0b671bebfd64a41216719e52e7cf703/src/Spryker/Shared/Log/LoggerFactory.php#L51

/**
 * @return \Psr\Log\LoggerInterface πŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆπŸ‘ˆ
 */
protected static function createInstanceIfNotExists(LoggerConfigInterface $loggerConfig)
{
    $channelName = $loggerConfig->getChannelName();

    if (!isset(static::$loggers[$channelName])) {
        $logger = new MonologLogger($channelName, $loggerConfig->getHandlers(), $loggerConfig->getProcessors());

        static::$loggers[$channelName] = $logger;
    }

    return static::$loggers[$channelName];
}

========================

Why I am saying this is because PHPStan level 8 is actually complaining about this, and it does makes sense that it complains about it, because the types signature are not correct. Am I missing something here or is it just wrong?

I wanted to create a PR to fix this but the repo is [READ ONLY] Subtree split of the Log module.… so I guess I can’t.

Comments

  • Valerii Trots
    Valerii Trots SRE @ Spryker Sprykee Posts: 1,654 ✨ - Novice
    Options

    Hi @U015S0C0V29, hopefully someone from our developers will take a look into this. I just wanted to say that you are free to create an external PR to that repo. It's just that we will need to import it to our internal mono repo where Spryker develops. And from there it will be released back as a new version. You will remain as the author\who to blame for the change though. πŸ™‚

  • Ievgen Varava
    Ievgen Varava Sprykee Posts: 154 πŸ§‘πŸ»β€πŸš€ - Cadet
    Options

    it's just an interface that was changed at some point https://github.com/spryker/log/commit/568bf8e82d0bcc6644bc5856a2ca4b232c472157

  • Ievgen Varava
    Ievgen Varava Sprykee Posts: 154 πŸ§‘πŸ»β€πŸš€ - Cadet
    Options
  • Ievgen Varava
    Ievgen Varava Sprykee Posts: 154 πŸ§‘πŸ»β€πŸš€ - Cadet
    Options

    no relations to implementation

  • Chemaclass
    Chemaclass Tech Lead Spryker Solution Partner Posts: 213 πŸ§‘πŸ»β€πŸš€ - Cadet
    Options

    @valerii.trots that’s good news, I didn’t know that! Thanks! πŸ™

    @ULSLLNL7K I thought something like that too. I think it’s just not updated to the actual/current state. Right now it clearly not returning a null ever, so I think it makes sense to remove the nullable there in the signature πŸ™‚

  • Chemaclass
    Chemaclass Tech Lead Spryker Solution Partner Posts: 213 πŸ§‘πŸ»β€πŸš€ - Cadet
    Options
  • Valerii Trots
    Valerii Trots SRE @ Spryker Sprykee Posts: 1,654 ✨ - Novice
    Options

    Thanks! I'll proceed with it as soon as I have time.
    I'll need to create an internal ticket for our developers, then I'll import it through our release app, then it will stay in the queue to be reviewed, tested and released. πŸ‘

  • Chemaclass
    Chemaclass Tech Lead Spryker Solution Partner Posts: 213 πŸ§‘πŸ»β€πŸš€ - Cadet
    Options

    Nice, thanks for your help! If I need to change or adapt something let me know πŸ§‘β€πŸ³πŸΌ

  • Valerii Trots
    Valerii Trots SRE @ Spryker Sprykee Posts: 1,654 ✨ - Novice
    Options

    Thank you for your contribution! πŸ™‚