[FrameworkBundle] [DI] Automatically add "setLogger" method call. #24973

GaryPEGEOT · 2017-11-15T11:14:33Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Automatically add setLogger method call to any service that implements Psr\Log\LoggerAwareInterface.

Not sure if this belong to FrameworkBundle or MonologBundle.

ro0NL · 2017-11-15T11:57:52Z

enable by autoconfigure: true?

chalasr · 2017-11-15T13:48:40Z

Belongs to monolog-bundle to me.

ogizanagi · 2017-11-15T13:58:19Z

Looks good to me in the FrameworkBundle, as we now have the default logger from HttpKernel registered as logger service if monolog (or any other one registering a logger service) isn't installed.

stloyd · 2017-11-15T14:08:18Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggerAwarePass.php

+            }
+
+            $refl = new \ReflectionClass($definition->getClass());
+            if ($refl->implementsInterface(LoggerAwareInterface::class) && !$definition->hasMethodCall('setLogger')) {


Maybe:

if ($definition->hasMethodCall('setLogger')) { continue; } if (\in_array(LoggerAwareInterface::class, class_implements($definition->getClass()), true)) { }

ro0NL · 2017-11-15T14:09:35Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggerAwarePass.php

+        $reference = new Reference('logger');
+
+        foreach ($container->getDefinitions() as $definition) {
+            if (!class_exists($definition->getClass()) || !$definition->isAutoconfigured()) {


|| $definition->hasMethodCall('setLogger') here?

ro0NL · 2017-11-15T14:09:39Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggerAwarePass.php

+                continue;
+            }
+
+            $refl = new \ReflectionClass($definition->getClass());


$container->getReflectionClass() i guess =/

ro0NL · 2017-11-15T14:10:46Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/LoggerAwarePassTest.php

+class LoggerAwarePassTest extends TestCase
+{
+    /**
+     * @covers \Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\LoggerAwarePass::process()


dont think this is needed really.

chalasr · 2017-11-15T14:14:03Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggerAwarePass.php

+        $reference = new Reference('logger');
+
+        foreach ($container->getDefinitions() as $definition) {
+            if (!class_exists($definition->getClass()) || !$definition->isAutoconfigured()) {


no reason to skip autoconfigured definitions. If we want autoconfigure to leverage this, the compiler pass should only process definitions with a given tag, and that tag+LoggerAwareInterface should be registered for autoconfiguration (see https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L269).

chalasr · 2017-11-15T14:17:35Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggerAwarePass.php

+                continue;
+            }
+
+            $refl = new \ReflectionClass($definition->getClass());


should use $container->getReflectionClass() and be done earlier, replacing the class_exists check by !$refl and probably throwing in case the check passes. See e.g. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php#L55

chalasr · 2017-11-15T14:18:39Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggerAwarePass.php

+
+        foreach ($container->getDefinitions() as $definition) {
+            $class = $definition->getClass();
+            if (!class_exists($class) || !$definition->isAutoconfigured() || $definition->hasMethodCall('setLogger')) {


If we want autoconfigure to leverage this, the compiler pass should only process definitions with a given tag, and that tag+LoggerAwareInterface should be registered for autoconfiguration (see https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L269).

!class_exists should be replaced by !$refl

About autoconfiguration, should I add tag in Framework extension (like logger.aware?) or skip this checking and add the method call anyway ?

nicolas-grekas

Wait, we don't need that compiler pass at all.
If we want to provide the behavior when autoconfiguration is on, we should just call $container->registerForAutoconfiguration(LoggerAwareInterface::class)->addMethodCall('setLogger', array(new Reference(LoggerInterface::class))) in some DI extension.

GaryPEGEOT · 2017-11-15T15:57:41Z

Shouldn't we check that method call ins't already called explicitly ?

chalasr · 2017-11-15T15:59:01Z

@nicolas-grekas Actually I think adding method calls to autoconfigure instanceofs is forbidden https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php#L37, isn't it? Should we remove that restriction?

nicolas-grekas · 2017-11-15T16:17:29Z

Calling a setter twice is not an issue to me so not really a pb.
And yes, we should remove the restriction in place.

chalasr · 2017-11-15T19:39:21Z

@GaryPEGEOT Can you replace the removed test by a new one ensuring that method calls are handled?

ro0NL · 2017-11-15T21:00:05Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -315,6 +317,9 @@ public function load(array $configs, ContainerBuilder $container)
        $container->registerForAutoconfiguration(ObjectInitializerInterface::class)
            ->addTag('validator.initializer');

+        $container->registerForAutoconfiguration(LoggerAwareInterface::class)
+            ->addMethodCall('setLogger', array(new Reference(LoggerInterface::class)));


Calling a setter twice is not an issue to me so not really a pb.

It would be a WTF no? (who's winning here?) Requires to move your definition outside the current file/context at least... not sure that justifies our technical implem, which is correct; we add a method call. But being a setter that messes things up.

Also not sure it justifies a addMethodCallOnce feat., perhaps as 2nd arg?

GaryPEGEOT · 2017-11-16T09:21:58Z

@nicolas-grekas Actually autoconfigured calls seems not to be working out of the box (Or I'm missing something.):

    /**
     * Test that autoconfigured calls are handled gracefully.
     */
    public function testProcessForAutoconfiguredCalls()
    {
        $container = new ContainerBuilder(); $container->registerForAutoconfiguration(LoggerAwareInterface::class)->addMethodCall('setLogger')->addTag('foo');

        $def = $container->register(\get_class($this->createMock(LoggerAwareInterface::class)));
        $this->assertFalse($def->hasTag('foo'), 'Definition shouldn\'t have tags yet.'); // Passes
        $this->assertFalse($def->hasMethodCall('setLogger'), 'Definition shouldn\'t have method call yet.'); // Passes

        (new ResolveInstanceofConditionalsPass())->process($container);
        $this->assertTrue($def->hasTag('foo'), 'Definition should have "foo" tag.'); // Passes
        $this->assertTrue($def->hasMethodCall('setLogger'), 'Definition should have "setLogger" method call.'); // Fail
    }

And I see potential problems for calling set twice, if by example, method is overrided for any reason:

    /**
     * Sets a logger instance on the object.
     *
     * @param LoggerInterface $logger
     * @param string          $channel Which channel to use
     *
     * @return void
     */
    public function setLogger(LoggerInterface $logger, string $channel = null)
    {
        // TODO: Implement setLogger() method.
    }

Here second parameter will be called with default param, so not necessary the expected behavior.

Compiler pass with tagged services seems to be the best option to me.

nicolas-grekas · 2017-11-16T13:31:11Z

Actually autoconfigured calls seems not to be working out of the box (Or I'm missing something.):
And I see potential problems for calling set twice, if by example, method is overrided for any reason

I'm OK to agree with that. That makes me wonder we should make that work in fact. The current exception in place that prevents using setters was put in place exactly because we didn't bother fixing those topic at the time.

But I would definitely prefer merging a PR that fixes that, which would provide high generality to the feature, vs merging a hardcoded adhoc alternative. The compiler pass as is could really well be part of a third party bundle meanwhile if you really want it.

weaverryan · 2017-11-16T15:46:48Z

This makes sense. But this still requires the user to add the setLogger method. A better DX would be to add a new LoggerTrait with ‘@required’ above the setLogger method.

ogizanagi · 2017-11-16T16:03:25Z

But this still requires the user to add the setLogger method

There is already a LoggerAwareTrait part of the psr/log package.

GaryPEGEOT · 2017-11-16T16:21:18Z

I made a separate MR #24996 to allow auto-configured method calls

nicolas-grekas · 2017-11-19T18:33:23Z

Closing in favor of #24996, thanks @GaryPEGEOT for taking over.

Gary PEGEOT added 2 commits November 15, 2017 12:11

[FrameworkBundle] [DI] Automatically add "setLogger" method call.

aa39944

Fix email

1eb32d3

carsonbot added Status: Needs Review FrameworkBundle DependencyInjection Feature labels Nov 15, 2017

Fabbot.io fixes.

0d415d3

nicolas-grekas added this to the 4.1 milestone Nov 15, 2017

Disable for service without autoconfiguration enabled.

b4bc0dc

stloyd suggested changes Nov 15, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 15, 2017

ro0NL reviewed Nov 15, 2017

View reviewed changes

chalasr requested changes Nov 15, 2017

View reviewed changes

chalasr reviewed Nov 15, 2017

View reviewed changes

Gary PEGEOT added 2 commits November 15, 2017 15:19

Small fixes

4d167f8

Use reflection class instead of class_exists

6a3a5bc

nicolas-grekas requested changes Nov 15, 2017

View reviewed changes

ro0NL reviewed Nov 15, 2017

View reviewed changes

GaryPEGEOT force-pushed the feature/logger-aware-di branch from 06502f1 to 35404a9 Compare November 16, 2017 10:13

Add tag for LoggerAwareInterface, add tagged services CP.

35404a9

nicolas-grekas closed this Nov 19, 2017

Uh oh!

[FrameworkBundle] [DI] Automatically add "setLogger" method call. #24973

[FrameworkBundle] [DI] Automatically add "setLogger" method call. #24973

Uh oh!

Conversation

GaryPEGEOT commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Nov 15, 2017

Uh oh!

chalasr commented Nov 15, 2017

Uh oh!

ogizanagi commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stloyd Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

ro0NL Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

ro0NL Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

chalasr Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

chalasr Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

chalasr Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

GaryPEGEOT Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryPEGEOT commented Nov 15, 2017

Uh oh!

chalasr commented Nov 15, 2017

Uh oh!

nicolas-grekas commented Nov 15, 2017

Uh oh!

chalasr commented Nov 15, 2017

Uh oh!

ro0NL Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryPEGEOT commented Nov 16, 2017

Uh oh!

nicolas-grekas commented Nov 16, 2017

Uh oh!

weaverryan commented Nov 16, 2017

Uh oh!

ogizanagi commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GaryPEGEOT commented Nov 16, 2017

Uh oh!

nicolas-grekas commented Nov 19, 2017

Uh oh!

Uh oh!

GaryPEGEOT commented Nov 15, 2017 •

edited

Loading

ogizanagi commented Nov 15, 2017 •

edited

Loading

stloyd Nov 15, 2017 •

edited

Loading

chalasr Nov 15, 2017 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading

ro0NL Nov 15, 2017 •

edited

Loading

ogizanagi commented Nov 16, 2017 •

edited

Loading