Codeception / module-yii2

Codeception module for Yii2 framework
MIT License
16 stars 36 forks source link

allow to override default Yii2 Logger class via DI container #1

Closed albertborsos closed 4 years ago

albertborsos commented 4 years ago

I have an enhanced Logger class which implements my LoggerInterface. All of my tests are failing because the default Logger class of Codeception is not implements this interface. I made a custom Logger class which extends the Codeception's Logger class and implements my LoggerInterface, but the Logger class is hardcoded.

With this PR it is possible to override in the test application via the configuration of the DI container, with the following way:

return [
     'components' => [
          ...
     ],
     'container' => [
          'definitions' => [
               \Codeception\Lib\Connector\Yii2\Logger::class => \tests\codeception\support\Logger::class,
          ],
     ],
];
albertborsos commented 4 years ago

previous PR: https://github.com/Codeception/Codeception/pull/5771

SamMousa commented 4 years ago

I think that your tests should be failing.

    /**
     * @return Logger message logger
     */
    public static function getLogger()
    {
        if (self::$_logger !== null) {
            return self::$_logger;
        }

        return self::$_logger = static::createObject('yii\log\Logger');
    }
    /**
     * Sets the logger object.
     * @param Logger $logger the logger object.
     */
    public static function setLogger($logger)
    {
        self::$_logger = $logger;
    }

While the code does not use type hinting because Yii2 supports older PHP versions, the PHPDoc clearly indicates that the param of setLogger() must be an instance of Logger. You are obviously free to tighten the return type of getLogger(), in this case to a subclass of Logger that implements LoggerInterface you are responsible for dealing with that when setLogger() is called.

If we merge this PR and allow for custom implementations that do not (properly) override the init function we will reintroduce memory leaks due to the use of register_shutdown_function.

Given that you have basically changed the contract in your code:

class Yii {

    public static function getLogger(): LoggerInterface 
    {
        ...
    }

I'm inclined to recommend actually fixing your code using composition instead of inheritance:


    public static function setLogger(Logger $logger) 
    {
         if ($logger instanceof LoggerInterface) {
            self::$_logger = $logger;
        } else {
            // Deal with a logger that does not implement your interface
        }
    }

I'm not a fan of static instantiation and would gladly replace it. But I feel this is not the way to go about it. I've actually worked on an alternative a while ago here: https://github.com/SamMousa/Codeception/tree/yii2-log-handling

That implementation doesn't replace your logger at all, so no issue there. All it does is add an extra log target for Codeception, which is what we want.

Also, it mocks register_shutdown_function in the yii\log namespace to prevent a memory leak

albertborsos commented 4 years ago

I understand what you said, but in my case it is a different use case. I just extended the default Logger with some helper method. For example created($id, $name, $type) etc.

But now, I decoupled these functions into a new component, so I do not need this feature anymore.

SamMousa commented 4 years ago

Okay, thanks for spending your time making Codeception better! Please go ahead and close this PR.