minor #27252 [HttpKernel] Make TraceableValueResolver $stopwatch mandatory (ogizanagi)

fabpot · fabpot · commit 906a05c43feb · 2018-05-14T08:40:41.000+02:00
This PR was merged into the 4.1 branch. Discussion ---------- [HttpKernel] Make TraceableValueResolver $stopwatch mandatory | Q | A | ------------- | --- | Branch? | 4.1  | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | #26833 (comment)  | License | MIT | Doc PR | N/A I understand why this was suggested in #26833 (comment), but as stated by @iltar , I don't think it makes sense to register a traceable resolver instantiating a Stopwatch itself, as there is no way to fetch it and wouldn't be a shared instance, probably defeating the feature and registering a useless decorator. Instead, let's make the stopwatch mandatory and make the service id to use in the pass configurable. Commits ------- 585ae7c [HttpKernel] Make TraceableValueResolver $stopwatch mandatory
diff --git a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/TraceableValueResolver.php b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/TraceableValueResolver.php
@@ -26,10 +26,10 @@ final class TraceableValueResolver implements ArgumentValueResolverInterface
     private $inner;
     private $stopwatch;
 
-    public function __construct(ArgumentValueResolverInterface $inner, ?Stopwatch $stopwatch = null)
+    public function __construct(ArgumentValueResolverInterface $inner, Stopwatch $stopwatch)
     {
         $this->inner = $inner;
-        $this->stopwatch = $stopwatch ?? new Stopwatch();
+        $this->stopwatch = $stopwatch;
     }
 
     /**
diff --git a/src/Symfony/Component/HttpKernel/DependencyInjection/ControllerArgumentValueResolverPass.php b/src/Symfony/Component/HttpKernel/DependencyInjection/ControllerArgumentValueResolverPass.php
@@ -15,7 +15,6 @@
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
 use Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
-use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\HttpKernel\Controller\ArgumentResolver\TraceableValueResolver;
 use Symfony\Component\Stopwatch\Stopwatch;
@@ -31,11 +30,13 @@ class ControllerArgumentValueResolverPass implements CompilerPassInterface
 
     private $argumentResolverService;
     private $argumentValueResolverTag;
+    private $traceableResolverStopwatch;
 
-    public function __construct(string $argumentResolverService = 'argument_resolver', string $argumentValueResolverTag = 'controller.argument_value_resolver')
+    public function __construct(string $argumentResolverService = 'argument_resolver', string $argumentValueResolverTag = 'controller.argument_value_resolver', string $traceableResolverStopwatch = 'debug.stopwatch')
     {
         $this->argumentResolverService = $argumentResolverService;
         $this->argumentValueResolverTag = $argumentValueResolverTag;
+        $this->traceableResolverStopwatch = $traceableResolverStopwatch;
     }
 
     public function process(ContainerBuilder $container)
@@ -46,12 +47,12 @@ public function process(ContainerBuilder $container)
 
         $resolvers = $this->findAndSortTaggedServices($this->argumentValueResolverTag, $container);
 
-        if ($container->getParameter('kernel.debug') && class_exists(Stopwatch::class)) {
+        if ($container->getParameter('kernel.debug') && class_exists(Stopwatch::class) && $container->has($this->traceableResolverStopwatch)) {
             foreach ($resolvers as $resolverReference) {
                 $id = (string) $resolverReference;
                 $container->register("debug.$id", TraceableValueResolver::class)
                     ->setDecoratedService($id)
-                    ->setArguments(array(new Reference("debug.$id.inner"), new Reference('debug.stopwatch', ContainerInterface::NULL_ON_INVALID_REFERENCE)));
+                    ->setArguments(array(new Reference("debug.$id.inner"), new Reference($this->traceableResolverStopwatch)));
             }
         }
 
diff --git a/src/Symfony/Component/HttpKernel/Tests/DependencyInjection/ControllerArgumentValueResolverPassTest.php b/src/Symfony/Component/HttpKernel/Tests/DependencyInjection/ControllerArgumentValueResolverPassTest.php
@@ -17,6 +17,7 @@
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\HttpKernel\Controller\ArgumentResolver;
 use Symfony\Component\HttpKernel\DependencyInjection\ControllerArgumentValueResolverPass;
+use Symfony\Component\Stopwatch\Stopwatch;
 
 class ControllerArgumentValueResolverPassTest extends TestCase
 {
@@ -52,7 +53,7 @@ public function testServicesAreOrderedAccordingToPriority()
         $this->assertFalse($container->hasDefinition('n3.traceable'));
     }
 
-    public function testInDebug()
+    public function testInDebugWithStopWatchDefinition()
     {
         $services = array(
             'n3' => array(array()),
@@ -68,6 +69,7 @@ public function testInDebug()
 
         $definition = new Definition(ArgumentResolver::class, array(null, array()));
         $container = new ContainerBuilder();
+        $container->register('debug.stopwatch', Stopwatch::class);
         $container->setDefinition('argument_resolver', $definition);
 
         foreach ($services as $id => list($tag)) {
@@ -88,6 +90,24 @@ public function testInDebug()
         $this->assertTrue($container->hasDefinition('n3'));
     }
 
+    public function testInDebugWithouStopWatchDefinition()
+    {
+        $expected = array(new Reference('n1'));
+
+        $definition = new Definition(ArgumentResolver::class, array(null, array()));
+        $container = new ContainerBuilder();
+        $container->register('n1')->addTag('controller.argument_value_resolver');
+        $container->setDefinition('argument_resolver', $definition);
+
+        $container->setParameter('kernel.debug', true);
+
+        (new ControllerArgumentValueResolverPass())->process($container);
+        $this->assertEquals($expected, $definition->getArgument(1)->getValues());
+
+        $this->assertFalse($container->hasDefinition('debug.n1'));
+        $this->assertTrue($container->hasDefinition('n1'));
+    }
+
     public function testReturningEmptyArrayWhenNoService()
     {
         $definition = new Definition(ArgumentResolver::class, array(null, array()));

Original file line number	Diff line number	Diff line change
`@@ -26,10 +26,10 @@ final class TraceableValueResolver implements ArgumentValueResolverInterface`
`26`	`26`	`private $inner;`
`27`	`27`	`private $stopwatch;`
`28`	`28`
`29`		`- public function __construct(ArgumentValueResolverInterface $inner, ?Stopwatch $stopwatch = null)`
	`29`	`+ public function __construct(ArgumentValueResolverInterface $inner, Stopwatch $stopwatch)`
`30`	`30`	`{`
`31`	`31`	`$this->inner = $inner;`
`32`		`- $this->stopwatch = $stopwatch ?? new Stopwatch();`
	`32`	`+ $this->stopwatch = $stopwatch;`
`33`	`33`	`}`
`34`	`34`
`35`	`35`	`/**`