I am wondering why the `LoggerTrait::getLogguer()` says that it can return a null 🤔

Chemaclass

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.

Valerii Trots

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

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

Ievgen Varava

or rather here https://github.com/spryker/log/commit/0ec8cb1335a8a4e304c94c18573b750dda5c4247

Ievgen Varava

no relations to implementation

Chemaclass

@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

Here you got the PR: https://github.com/spryker/log/pull/1

Valerii Trots

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

Nice, thanks for your help! If I need to change or adapt something let me know 🧑‍🍳🏼

Valerii Trots

Thank you for your contribution! 🙂