-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] Do not throw autowiring exceptions for a service that will be removed #22665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Component\DependencyInjection\Compiler; | ||
|
||
use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
|
||
/** | ||
* Throws autowire exceptions from AutowirePass for definitions that still exist. | ||
* | ||
* @author Ryan Weaver <ryan@knpuniversity.com> | ||
*/ | ||
class AutowireExceptionPass implements CompilerPassInterface | ||
{ | ||
private $autowirePass; | ||
|
||
public function __construct(AutowirePass $autowirePass) | ||
{ | ||
$this->autowirePass = $autowirePass; | ||
} | ||
|
||
public function process(ContainerBuilder $container) | ||
{ | ||
foreach ($this->autowirePass->getAutowiringExceptions() as $exception) { | ||
if ($container->hasDefinition($exception->getServiceId())) { | ||
throw $exception; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
use Symfony\Component\DependencyInjection\Config\AutowireServiceResource; | ||
use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
use Symfony\Component\DependencyInjection\Definition; | ||
use Symfony\Component\DependencyInjection\Exception\AutowiringFailedException; | ||
use Symfony\Component\DependencyInjection\Exception\RuntimeException; | ||
use Symfony\Component\DependencyInjection\LazyProxy\ProxyHelper; | ||
use Symfony\Component\DependencyInjection\TypedReference; | ||
|
@@ -31,12 +32,33 @@ class AutowirePass extends AbstractRecursivePass | |
private $ambiguousServiceTypes = array(); | ||
private $autowired = array(); | ||
private $lastFailure; | ||
private $throwOnAutowiringException; | ||
private $autowiringExceptions = array(); | ||
|
||
/** | ||
* @param bool $throwOnAutowireException If false, retrieved errors via getAutowiringExceptions | ||
*/ | ||
public function __construct($throwOnAutowireException = true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why adding this flag? It looks unused. For BC? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly - for BC if you're using the class directly. Seems like an unlikely thing, but Nicolas mentioned it... and technically, we do need this for BC. |
||
{ | ||
$this->throwOnAutowiringException = $throwOnAutowireException; | ||
} | ||
|
||
/** | ||
* @return AutowiringFailedException[] | ||
*/ | ||
public function getAutowiringExceptions() | ||
{ | ||
return $this->autowiringExceptions; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function process(ContainerBuilder $container) | ||
{ | ||
// clear out any possibly stored exceptions from before | ||
$this->autowiringExceptions = array(); | ||
|
||
try { | ||
parent::process($container); | ||
} finally { | ||
|
@@ -75,6 +97,21 @@ public static function createResourceForClass(\ReflectionClass $reflectionClass) | |
* {@inheritdoc} | ||
*/ | ||
protected function processValue($value, $isRoot = false) | ||
{ | ||
try { | ||
return $this->doProcessValue($value, $isRoot); | ||
} catch (AutowiringFailedException $e) { | ||
if ($this->throwOnAutowiringException) { | ||
throw $e; | ||
} | ||
|
||
$this->autowiringExceptions[] = $e; | ||
|
||
return parent::processValue($value, $isRoot); | ||
} | ||
} | ||
|
||
private function doProcessValue($value, $isRoot = false) | ||
{ | ||
if ($value instanceof TypedReference) { | ||
if ($ref = $this->getAutowiredReference($value)) { | ||
|
@@ -190,7 +227,7 @@ private function autowireCalls(\ReflectionClass $reflectionClass, array $methodC | |
|
||
if (!$reflectionMethod->isPublic()) { | ||
$class = $reflectionClass->name; | ||
throw new RuntimeException(sprintf('Cannot autowire service "%s": method "%s()" must be public.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method)); | ||
throw new AutowiringFailedException($this->currentId, sprintf('Cannot autowire service "%s": method "%s()" must be public.', $this->currentId, $class !== $this->currentId ? $class.'::'.$method : $method)); | ||
} | ||
$methodCalls[] = array($method, $this->autowireMethod($reflectionMethod, array())); | ||
} | ||
|
@@ -231,7 +268,7 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a | |
|
||
// no default value? Then fail | ||
if (!$parameter->isDefaultValueAvailable()) { | ||
throw new RuntimeException(sprintf('Cannot autowire service "%s": argument "$%s" of method "%s()" must have a type-hint or be given a value explicitly.', $this->currentId, $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method)); | ||
throw new AutowiringFailedException($this->currentId, sprintf('Cannot autowire service "%s": argument "$%s" of method "%s()" must have a type-hint or be given a value explicitly.', $this->currentId, $parameter->name, $class !== $this->currentId ? $class.'::'.$method : $method)); | ||
} | ||
|
||
// specifically pass the default value | ||
|
@@ -246,7 +283,7 @@ private function autowireMethod(\ReflectionFunctionAbstract $reflectionMethod, a | |
if ($parameter->isDefaultValueAvailable()) { | ||
$value = $parameter->getDefaultValue(); | ||
} elseif (!$parameter->allowsNull()) { | ||
throw new RuntimeException($failureMessage); | ||
throw new AutowiringFailedException($this->currentId, $failureMessage); | ||
} | ||
$this->container->log($this, $failureMessage); | ||
} | ||
|
@@ -407,15 +444,18 @@ private function createAutowiredDefinition($type) | |
$argumentDefinition->setAutowired(true); | ||
|
||
try { | ||
$originalThrowSetting = $this->throwOnAutowiringException; | ||
$this->throwOnAutowiringException = true; | ||
$this->processValue($argumentDefinition, true); | ||
$this->container->setDefinition($argumentId, $argumentDefinition); | ||
} catch (RuntimeException $e) { | ||
} catch (AutowiringFailedException $e) { | ||
$this->autowired[$type] = false; | ||
$this->lastFailure = $e->getMessage(); | ||
$this->container->log($this, $this->lastFailure); | ||
|
||
return; | ||
} finally { | ||
$this->throwOnAutowiringException = $originalThrowSetting; | ||
$this->currentId = $currentId; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Component\DependencyInjection\Exception; | ||
|
||
/** | ||
* Thrown when a definition cannot be autowired. | ||
*/ | ||
class AutowiringFailedException extends RuntimeException | ||
{ | ||
private $serviceId; | ||
|
||
public function __construct($serviceId, $message = '', $code = 0, Exception $previous = null) | ||
{ | ||
$this->serviceId = $serviceId; | ||
|
||
parent::__construct($message, $code, $previous); | ||
} | ||
|
||
public function getServiceId() | ||
{ | ||
return $this->serviceId; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Component\DependencyInjection\Tests\Compiler; | ||
|
||
use PHPUnit\Framework\TestCase; | ||
use Symfony\Component\DependencyInjection\Compiler\AutowireExceptionPass; | ||
use Symfony\Component\DependencyInjection\Compiler\AutowirePass; | ||
use Symfony\Component\DependencyInjection\ContainerBuilder; | ||
use Symfony\Component\DependencyInjection\Exception\AutowiringFailedException; | ||
|
||
class AutowireExceptionPassTest extends TestCase | ||
{ | ||
public function testThrowsException() | ||
{ | ||
$autowirePass = $this->getMockBuilder(AutowirePass::class) | ||
->getMock(); | ||
|
||
$autowireException = new AutowiringFailedException('foo_service_id', 'An autowiring exception message'); | ||
$autowirePass->expects($this->any()) | ||
->method('getAutowiringExceptions') | ||
->will($this->returnValue(array($autowireException))); | ||
|
||
$container = new ContainerBuilder(); | ||
$container->register('foo_service_id'); | ||
|
||
$pass = new AutowireExceptionPass($autowirePass); | ||
|
||
try { | ||
$pass->process($container); | ||
$this->fail('->process() should throw the exception if the service id exists'); | ||
} catch (\Exception $e) { | ||
$this->assertSame($autowireException, $e); | ||
} | ||
} | ||
|
||
public function testNoExceptionIfServiceRemoved() | ||
{ | ||
$autowirePass = $this->getMockBuilder(AutowirePass::class) | ||
->getMock(); | ||
|
||
$autowireException = new AutowiringFailedException('non_existent_service'); | ||
$autowirePass->expects($this->any()) | ||
->method('getAutowiringExceptions') | ||
->will($this->returnValue(array($autowireException))); | ||
|
||
$container = new ContainerBuilder(); | ||
|
||
$pass = new AutowireExceptionPass($autowirePass); | ||
|
||
$pass->process($container); | ||
// mark the test as passed | ||
$this->assertTrue(true); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird to have a persistent state on this class. Shouldn't it be cleared at the end of the process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'm going to clear
$autowiringExceptions
just like the others are cleared.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, except that I'm clearing it at the beginning of the process. The pass does need persistent state - the other pass uses it. There's not a "better" way of doing this (i.e. storing somewhere in the container) that I'm aware of. Anyways, I am now clearing it at the beginning of process, so if it's called twice, it's cleared between.