bug symfony#23573 [Config] Make ClassExistenceResource throw on invalid parents (nicolas-grekas)

fabpot · fabpot · commit 547185fe7a76 · 2017-07-19T06:18:00.000+02:00
This PR was merged into the 3.3 branch. Discussion ---------- [Config] Make ClassExistenceResource throw on invalid parents | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#23564 | License | MIT | Doc PR | - Let's throw a more specific exception when a parent class/interface/trait is missing. Fine tunes symfony#23041 Commits ------- 53b0190 [Config] Make ClassExistenceResource throw on invalid parents
diff --git a/src/Symfony/Component/Config/Resource/ClassExistenceResource.php b/src/Symfony/Component/Config/Resource/ClassExistenceResource.php
@@ -25,6 +25,7 @@ class ClassExistenceResource implements SelfCheckingResourceInterface, \Serializ
     private $exists;
 
     private static $autoloadLevel = 0;
+    private static $autoloadedClass;
     private static $existsCache = array();
 
     /**
@@ -57,6 +58,8 @@ public function getResource()
 
     /**
      * {@inheritdoc}
+     *
+     * @throws \ReflectionException when a parent class/interface/trait is not found
      */
     public function isFresh($timestamp)
     {
@@ -68,12 +71,13 @@ public function isFresh($timestamp)
             if (!self::$autoloadLevel++) {
                 spl_autoload_register(__CLASS__.'::throwOnRequiredClass');
             }
+            $autoloadedClass = self::$autoloadedClass;
+            self::$autoloadedClass = $this->resource;
 
             try {
                 $exists = class_exists($this->resource) || interface_exists($this->resource, false) || trait_exists($this->resource, false);
-            } catch (\ReflectionException $e) {
-                $exists = false;
             } finally {
+                self::$autoloadedClass = $autoloadedClass;
                 if (!--self::$autoloadLevel) {
                     spl_autoload_unregister(__CLASS__.'::throwOnRequiredClass');
                 }
@@ -112,7 +116,10 @@ public function unserialize($serialized)
      */
     private static function throwOnRequiredClass($class)
     {
-        $e = new \ReflectionException("Class $class does not exist");
+        if (self::$autoloadedClass === $class) {
+            return;
+        }
+        $e = new \ReflectionException("Class $class not found");
         $trace = $e->getTrace();
         $autoloadFrame = array(
             'function' => 'spl_autoload_call',
@@ -138,6 +145,18 @@ private static function throwOnRequiredClass($class)
                 case 'is_callable':
                     return;
             }
+
+            $props = array(
+                'file' => $trace[$i]['file'],
+                'line' => $trace[$i]['line'],
+                'trace' => array_slice($trace, 0, 1 + $i),
+            );
+
+            foreach ($props as $p => $v) {
+                $r = new \ReflectionProperty('Exception', $p);
+                $r->setAccessible(true);
+                $r->setValue($e, $v);
+            }
         }
 
         throw $e;
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php b/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php
@@ -124,8 +124,8 @@ private function doProcessValue($value, $isRoot = false)
         if (!$value instanceof Definition || !$value->isAutowired() || $value->isAbstract() || !$value->getClass()) {
             return $value;
         }
-        if (!$reflectionClass = $this->container->getReflectionClass($value->getClass())) {
-            $this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" does not exist.', $this->currentId, $value->getClass()));
+        if (!$reflectionClass = $this->container->getReflectionClass($value->getClass(), false)) {
+            $this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" cannot be loaded.', $this->currentId, $value->getClass()));
 
             return $value;
         }
@@ -388,7 +388,7 @@ private function populateAvailableType($id, Definition $definition)
             unset($this->ambiguousServiceTypes[$type]);
         }
 
-        if ($definition->isDeprecated() || !$reflectionClass = $this->container->getReflectionClass($definition->getClass())) {
+        if ($definition->isDeprecated() || !$reflectionClass = $this->container->getReflectionClass($definition->getClass(), false)) {
             return;
         }
 
@@ -444,7 +444,7 @@ private function set($type, $id)
      */
     private function createAutowiredDefinition($type)
     {
-        if (!($typeHint = $this->container->getReflectionClass($type)) || !$typeHint->isInstantiable()) {
+        if (!($typeHint = $this->container->getReflectionClass($type, false)) || !$typeHint->isInstantiable()) {
             return;
         }
 
@@ -478,8 +478,8 @@ private function createAutowiredDefinition($type)
 
     private function createTypeNotFoundMessage(TypedReference $reference, $label)
     {
-        if (!$r = $this->container->getReflectionClass($type = $reference->getType())) {
-            $message = sprintf('has type "%s" but this class does not exist.', $type);
+        if (!$r = $this->container->getReflectionClass($type = $reference->getType(), false)) {
+            $message = sprintf('has type "%s" but this class cannot be loaded.', $type);
         } else {
             $message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists';
             $message = sprintf('references %s "%s" but %s.%s', $r->isInterface() ? 'interface' : 'class', $type, $message, $this->createTypeAlternatives($reference));
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php b/src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php
@@ -81,7 +81,7 @@ private function updateDefinition(ContainerBuilder $container, $id, Definition $
                 $class = $factory[0];
             }
 
-            if (!$m = $container->getReflectionClass($class)) {
+            if (!$m = $container->getReflectionClass($class, false)) {
                 return;
             }
             try {
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveInstanceofConditionalsPass.php
@@ -66,7 +66,7 @@ private function processDefinition(ContainerBuilder $container, $id, Definition
         $instanceofTags = array();
 
         foreach ($conditionals as $interface => $instanceofDefs) {
-            if ($interface !== $class && (!$container->getReflectionClass($class))) {
+            if ($interface !== $class && (!$container->getReflectionClass($class, false))) {
                 continue;
             }
 
diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -337,12 +337,15 @@ public function addClassResource(\ReflectionClass $class)
      * Retrieves the requested reflection class and registers it for resource tracking.
      *
      * @param string $class
+     * @param bool   $throw
      *
      * @return \ReflectionClass|null
      *
+     * @throws \ReflectionException when a parent class/interface/trait is not found and $throw is true
+     *
      * @final
      */
-    public function getReflectionClass($class)
+    public function getReflectionClass($class, $throw = true)
     {
         if (!$class = $this->getParameterBag()->resolveValue($class)) {
             return;
@@ -357,6 +360,9 @@ public function getReflectionClass($class)
                 $classReflector = $resource->isFresh(0) ? false : new \ReflectionClass($class);
             }
         } catch (\ReflectionException $e) {
+            if ($throw) {
+                throw $e;
+            }
             $classReflector = false;
         }
 
diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/AutowirePassTest.php
@@ -359,7 +359,7 @@ public function testDontTriggerAutowiring()
 
     /**
      * @expectedException \Symfony\Component\DependencyInjection\Exception\AutowiringFailedException
-     * @expectedExceptionMessage Cannot autowire service "a": argument "$r" of method "Symfony\Component\DependencyInjection\Tests\Compiler\BadTypeHintedArgument::__construct()" has type "Symfony\Component\DependencyInjection\Tests\Compiler\NotARealClass" but this class does not exist.
+     * @expectedExceptionMessage Cannot autowire service "a": argument "$r" of method "Symfony\Component\DependencyInjection\Tests\Compiler\BadTypeHintedArgument::__construct()" has type "Symfony\Component\DependencyInjection\Tests\Compiler\NotARealClass" but this class cannot be loaded.
      */
     public function testClassNotFoundThrowsException()
     {
@@ -374,7 +374,7 @@ public function testClassNotFoundThrowsException()
 
     /**
      * @expectedException \Symfony\Component\DependencyInjection\Exception\AutowiringFailedException
-     * @expectedExceptionMessage Cannot autowire service "a": argument "$r" of method "Symfony\Component\DependencyInjection\Tests\Compiler\BadParentTypeHintedArgument::__construct()" has type "Symfony\Component\DependencyInjection\Tests\Compiler\OptionalServiceClass" but this class does not exist.
+     * @expectedExceptionMessage Cannot autowire service "a": argument "$r" of method "Symfony\Component\DependencyInjection\Tests\Compiler\BadParentTypeHintedArgument::__construct()" has type "Symfony\Component\DependencyInjection\Tests\Compiler\OptionalServiceClass" but this class cannot be loaded.
      */
     public function testParentClassNotFoundThrowsException()
     {
@@ -744,7 +744,7 @@ public function testNotWireableCalls($method, $expectedMsg)
     public function provideNotWireableCalls()
     {
         return array(
-            array('setNotAutowireable', 'Cannot autowire service "foo": argument "$n" of method "Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setNotAutowireable()" has type "Symfony\Component\DependencyInjection\Tests\Compiler\NotARealClass" but this class does not exist.'),
+            array('setNotAutowireable', 'Cannot autowire service "foo": argument "$n" of method "Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setNotAutowireable()" has type "Symfony\Component\DependencyInjection\Tests\Compiler\NotARealClass" but this class cannot be loaded.'),
             array('setDifferentNamespace', 'Cannot autowire service "foo": argument "$n" of method "Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setDifferentNamespace()" references class "stdClass" but no such service exists. It cannot be auto-registered because it is from a different root namespace.'),
             array(null, 'Cannot autowire service "foo": method "Symfony\Component\DependencyInjection\Tests\Compiler\NotWireable::setProtectedMethod()" must be public.'),
         );

Original file line number	Diff line number	Diff line change
`@@ -124,8 +124,8 @@ private function doProcessValue($value, $isRoot = false)`
`124`	`124`	`if (!$value instanceof Definition \|\| !$value->isAutowired() \|\| $value->isAbstract() \|\| !$value->getClass()) {`
`125`	`125`	`return $value;`
`126`	`126`	`}`
`127`		`- if (!$reflectionClass = $this->container->getReflectionClass($value->getClass())) {`
`128`		`- $this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" does not exist.', $this->currentId, $value->getClass()));`
	`127`	`+ if (!$reflectionClass = $this->container->getReflectionClass($value->getClass(), false)) {`
	`128`	`+ $this->container->log($this, sprintf('Skipping service "%s": Class or interface "%s" cannot be loaded.', $this->currentId, $value->getClass()));`
`129`	`129`
`130`	`130`	`return $value;`
`131`	`131`	`}`
`@@ -388,7 +388,7 @@ private function populateAvailableType($id, Definition $definition)`
`388`	`388`	`unset($this->ambiguousServiceTypes[$type]);`
`389`	`389`	`}`
`390`	`390`
`391`		`- if ($definition->isDeprecated() \|\| !$reflectionClass = $this->container->getReflectionClass($definition->getClass())) {`
	`391`	`+ if ($definition->isDeprecated() \|\| !$reflectionClass = $this->container->getReflectionClass($definition->getClass(), false)) {`
`392`	`392`	`return;`
`393`	`393`	`}`
`394`	`394`
`@@ -444,7 +444,7 @@ private function set($type, $id)`
`444`	`444`	`*/`
`445`	`445`	`private function createAutowiredDefinition($type)`
`446`	`446`	`{`
`447`		`- if (!($typeHint = $this->container->getReflectionClass($type)) \|\| !$typeHint->isInstantiable()) {`
	`447`	`+ if (!($typeHint = $this->container->getReflectionClass($type, false)) \|\| !$typeHint->isInstantiable()) {`
`448`	`448`	`return;`
`449`	`449`	`}`
`450`	`450`
`@@ -478,8 +478,8 @@ private function createAutowiredDefinition($type)`
`478`	`478`
`479`	`479`	`private function createTypeNotFoundMessage(TypedReference $reference, $label)`
`480`	`480`	`{`
`481`		`- if (!$r = $this->container->getReflectionClass($type = $reference->getType())) {`
`482`		`- $message = sprintf('has type "%s" but this class does not exist.', $type);`
	`481`	`+ if (!$r = $this->container->getReflectionClass($type = $reference->getType(), false)) {`
	`482`	`+ $message = sprintf('has type "%s" but this class cannot be loaded.', $type);`
`483`	`483`	`} else {`
`484`	`484`	`$message = $this->container->has($type) ? 'this service is abstract' : 'no such service exists';`
`485`	`485`	`$message = sprintf('references %s "%s" but %s.%s', $r->isInterface() ? 'interface' : 'class', $type, $message, $this->createTypeAlternatives($reference));`
Original file line number	Diff line number	Diff line change
`@@ -81,7 +81,7 @@ private function updateDefinition(ContainerBuilder $container, $id, Definition $`
`81`	`81`	`$class = $factory[0];`
`82`	`82`	`}`
`83`	`83`
`84`		`- if (!$m = $container->getReflectionClass($class)) {`
	`84`	`+ if (!$m = $container->getReflectionClass($class, false)) {`
`85`	`85`	`return;`
`86`	`86`	`}`
`87`	`87`	`try {`
Original file line number	Diff line number	Diff line change
`@@ -66,7 +66,7 @@ private function processDefinition(ContainerBuilder $container, $id, Definition`
`66`	`66`	`$instanceofTags = array();`
`67`	`67`
`68`	`68`	`foreach ($conditionals as $interface => $instanceofDefs) {`
`69`		`- if ($interface !== $class && (!$container->getReflectionClass($class))) {`
	`69`	`+ if ($interface !== $class && (!$container->getReflectionClass($class, false))) {`
`70`	`70`	`continue;`
`71`	`71`	`}`
`72`	`72`
Original file line number	Diff line number	Diff line change
`@@ -337,12 +337,15 @@ public function addClassResource(\ReflectionClass $class)`
`337`	`337`	`* Retrieves the requested reflection class and registers it for resource tracking.`
`338`	`338`	`*`
`339`	`339`	`* @param string $class`
	`340`	`+ * @param bool $throw`
`340`	`341`	`*`
`341`	`342`	`* @return \ReflectionClass\|null`
`342`	`343`	`*`
	`344`	`+ * @throws \ReflectionException when a parent class/interface/trait is not found and $throw is true`
	`345`	`+ *`
`343`	`346`	`* @final`
`344`	`347`	`*/`
`345`		`- public function getReflectionClass($class)`
	`348`	`+ public function getReflectionClass($class, $throw = true)`
`346`	`349`	`{`
`347`	`350`	`if (!$class = $this->getParameterBag()->resolveValue($class)) {`
`348`	`351`	`return;`
`@@ -357,6 +360,9 @@ public function getReflectionClass($class)`
`357`	`360`	`$classReflector = $resource->isFresh(0) ? false : new \ReflectionClass($class);`
`358`	`361`	`}`
`359`	`362`	`} catch (\ReflectionException $e) {`
	`363`	`+ if ($throw) {`
	`364`	`+ throw $e;`
	`365`	`+ }`
`360`	`366`	`$classReflector = false;`
`361`	`367`	`}`
`362`	`368`