Skip to content

[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

Merged
merged 1 commit into from
May 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Expand Up @@ -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;
Expand All @@ -31,12 +32,33 @@ class AutowirePass extends AbstractRecursivePass
private $ambiguousServiceTypes = array();
private $autowired = array();
private $lastFailure;
private $throwOnAutowiringException;
private $autowiringExceptions = array();
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.


/**
* @param bool $throwOnAutowireException If false, retrieved errors via getAutowiringExceptions
*/
public function __construct($throwOnAutowireException = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why adding this flag? It looks unused. For BC?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 {
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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()));
}
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
Expand Down Expand Up @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public function __construct()
new CheckDefinitionValidityPass(),
new RegisterServiceSubscribersPass(),
new ResolveNamedArgumentsPass(),
new AutowirePass(),
$autowirePass = new AutowirePass(false),
new ResolveServiceSubscribersPass(),
new ResolveReferencesToAliasesPass(),
new ResolveInvalidReferencesPass(),
Expand All @@ -77,6 +77,7 @@ public function __construct()
new AnalyzeServiceReferencesPass(),
new RemoveUnusedDefinitionsPass(),
)),
new AutowireExceptionPass($autowirePass),
new CheckExceptionOnInvalidReferenceBehaviorPass(),
));
}
Expand Down
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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,21 @@ public function testCompleteExistingDefinitionWithNotDefinedArguments()
$this->assertEquals(DInterface::class, (string) $container->getDefinition('h')->getArgument(1));
}

public function testExceptionsAreStored()
{
$container = new ContainerBuilder();

$container->register('c1', __NAMESPACE__.'\CollisionA');
$container->register('c2', __NAMESPACE__.'\CollisionB');
$container->register('c3', __NAMESPACE__.'\CollisionB');
$aDefinition = $container->register('a', __NAMESPACE__.'\CannotBeAutowired');
$aDefinition->setAutowired(true);

$pass = new AutowirePass(false);
$pass->process($container);
$this->assertCount(1, $pass->getAutowiringExceptions());
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
* @expectedExceptionMessage Cannot autowire service "a": argument "$collision" of method "Symfony\Component\DependencyInjection\Tests\Compiler\CannotBeAutowired::__construct()" references interface "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "c1", "c2", "c3".
Expand Down