alexdebril / rss-atom-bundle

RSS and Atom Bundle for Symfony
MIT License
139 stars 49 forks source link

Issue With Logger #127

Closed bshoemaker136 closed 7 years ago

bshoemaker136 commented 7 years ago

In your extension class you have this:

$this->setDefinition($container, 'logger', 'Psr\Log\NullLogger');

Isn't that overriding the typical Monolog logger that has been provided by Symfony? Is there a reason to override that?

alexdebril commented 7 years ago

Hi,

It's not overriding the default logger because setDefinition checks if the definition exists or has an alias before :

    protected function setDefinition(ContainerBuilder $container, $serviceName, $className)
    {
        if ( ! $container->hasDefinition($serviceName) && ! $container->hasAlias($serviceName)) {
            $container->setDefinition($serviceName, new Definition($className));
        }

        return $this;
    }

It creates a logger instance if your dependency injection doesn't have one, which is the case during the unit tests execution.

Do you experience an issue with that ?

matteogatti commented 7 years ago

After the upgrade form version 2.x to 3.0.4 the service logger was substituted from Symfony\Bridge\Monolog\Logger to Psr\Log\NullLogger. Strangely $container->getDefinitions() method gets an empty array and the setDefinition doesn't work properly. Maybe setDefinition needs some extra control because as the documentation say:

In the load() method, all services and parameters related to this extension will be loaded. This method doesn't get the actual container instance, but a copy. This container only has the parameters from the actual container. After loading the services and parameters, the copy will be merged into the actual container, to ensure all services and parameters are also added to the actual container.

DonCallisto commented 7 years ago

Same here. It seems that hasDefinition and hasAlias will return both false (maybe container isn't processed yet?) and, so, definition gets and override.

DonCallisto commented 7 years ago

I've open a PR for this issue --> https://github.com/alexdebril/rss-atom-bundle/pull/128 Seems to work in my local application.

alexdebril commented 7 years ago

Thank you @DonCallisto , it works fine with the demo application. :+1:

DonCallisto commented 7 years ago

@alexdebril you're welcome. Maybe that's perfect for a patch version like 3.0.5. What do you think?

alexdebril commented 7 years ago

I just did it : https://github.com/alexdebril/rss-atom-bundle/releases/tag/v3.0.5

DonCallisto commented 7 years ago

Thank you!