[FrameworkBundle] Deprecate auto-injection of the container in AbstractController instances

nicolas-grekas · nicolas-grekas · commit 38277b7935eb · 2018-06-04T12:56:02.000+02:00
diff --git a/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md b/src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
@@ -5,6 +5,7 @@ CHANGELOG
 -----
 
  * Allowed configuring taggable cache pools via a new `framework.cache.pools.tags` option (bool|service-id)
+ * Deprecated auto-injection of the container in AbstractController instances, register them as service subscribers instead
 
 4.1.0
 -----
diff --git a/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php b/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php
@@ -51,16 +51,22 @@ protected function createController($controller)
      */
     protected function instantiateController($class)
     {
-        return $this->configureController(parent::instantiateController($class));
+        return $this->configureController(parent::instantiateController($class), $class);
     }
 
-    private function configureController($controller)
+    private function configureController($controller, string $class)
     {
         if ($controller instanceof ContainerAwareInterface) {
             $controller->setContainer($this->container);
         }
-        if ($controller instanceof AbstractController && null !== $previousContainer = $controller->setContainer($this->container)) {
-            $controller->setContainer($previousContainer);
+        if ($controller instanceof AbstractController) {
+            if (null === $previousContainer = $controller->setContainer($this->container);
+                @trigger_error(sprintf('Auto-injection of the container for "%s" is deprecated since Symfony 4.2. Configure it as a service instead.', $class), E_USER_DEPRECATED);
+                // To be uncommented on Symfony 5:
+                //throw new \LogicException(sprintf('"%s" has no container set, did you forget to define it as a service subscriber?', $class));
+            } else {
+                $controller->setContainer($previousContainer);
+            }
         }
 
         return $controller;
diff --git a/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerResolverTest.php b/src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerResolverTest.php
@@ -92,6 +92,10 @@ class_exists(AbstractControllerTest::class);
         $this->assertSame($container, $controller->getContainer());
     }
 
+    /**
+     * @group legacy
+     * @expectedDeprecation Auto-injection of the container for "Symfony\Bundle\FrameworkBundle\Tests\Controller\TestAbstractController" is deprecated since Symfony 4.2. Configure it as a service instead.
+     */
     public function testAbstractControllerGetsContainerWhenNotSet()
     {
         class_exists(AbstractControllerTest::class);
@@ -110,6 +114,10 @@ class_exists(AbstractControllerTest::class);
         $this->assertSame($container, $controller->setContainer($container));
     }
 
+    /**
+     * @group legacy
+     * @expectedDeprecation Auto-injection of the container for "Symfony\Bundle\FrameworkBundle\Tests\Controller\DummyController" is deprecated since Symfony 4.2. Configure it as a service instead.
+     */
     public function testAbstractControllerServiceWithFcqnIdGetsContainerWhenNotSet()
     {
         class_exists(AbstractControllerTest::class);

Original file line number	Diff line number	Diff line change
`@@ -51,16 +51,22 @@ protected function createController($controller)`
`51`	`51`	`*/`
`52`	`52`	`protected function instantiateController($class)`
`53`	`53`	`{`
`54`		`- return $this->configureController(parent::instantiateController($class));`
	`54`	`+ return $this->configureController(parent::instantiateController($class), $class);`
`55`	`55`	`}`
`56`	`56`
`57`		`- private function configureController($controller)`
	`57`	`+ private function configureController($controller, string $class)`
`58`	`58`	`{`
`59`	`59`	`if ($controller instanceof ContainerAwareInterface) {`
`60`	`60`	`$controller->setContainer($this->container);`
`61`	`61`	`}`
`62`		`- if ($controller instanceof AbstractController && null !== $previousContainer = $controller->setContainer($this->container)) {`
`63`		`- $controller->setContainer($previousContainer);`
	`62`	`+ if ($controller instanceof AbstractController) {`
	`63`	`+ if (null === $previousContainer = $controller->setContainer($this->container);`
	`64`	`+ @trigger_error(sprintf('Auto-injection of the container for "%s" is deprecated since Symfony 4.2. Configure it as a service instead.', $class), E_USER_DEPRECATED);`
	`65`	`+ // To be uncommented on Symfony 5:`
	`66`	`+ //throw new \LogicException(sprintf('"%s" has no container set, did you forget to define it as a service subscriber?', $class));`
	`67`	`+ } else {`
	`68`	`+ $controller->setContainer($previousContainer);`
	`69`	`+ }`
`64`	`70`	`}`
`65`	`71`
`66`	`72`	`return $controller;`
Original file line number	Diff line number	Diff line change
`@@ -92,6 +92,10 @@ class_exists(AbstractControllerTest::class);`
`92`	`92`	`$this->assertSame($container, $controller->getContainer());`
`93`	`93`	`}`
`94`	`94`
	`95`	`+ /**`
	`96`	`+ * @group legacy`
	`97`	`+ * @expectedDeprecation Auto-injection of the container for "Symfony\Bundle\FrameworkBundle\Tests\Controller\TestAbstractController" is deprecated since Symfony 4.2. Configure it as a service instead.`
	`98`	`+ */`
`95`	`99`	`public function testAbstractControllerGetsContainerWhenNotSet()`
`96`	`100`	`{`
`97`	`101`	`class_exists(AbstractControllerTest::class);`
`@@ -110,6 +114,10 @@ class_exists(AbstractControllerTest::class);`
`110`	`114`	`$this->assertSame($container, $controller->setContainer($container));`
`111`	`115`	`}`
`112`	`116`
	`117`	`+ /**`
	`118`	`+ * @group legacy`
	`119`	`+ * @expectedDeprecation Auto-injection of the container for "Symfony\Bundle\FrameworkBundle\Tests\Controller\DummyController" is deprecated since Symfony 4.2. Configure it as a service instead.`
	`120`	`+ */`
`113`	`121`	`public function testAbstractControllerServiceWithFcqnIdGetsContainerWhenNotSet()`
`114`	`122`	`{`
`115`	`123`	`class_exists(AbstractControllerTest::class);`