Skip to content

Adding Definition::addError() and a compiler pass to throw errors as exceptions #24290

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

Closed
wants to merge 6 commits into from
Closed
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
Expand Up @@ -22,6 +22,9 @@
*/
abstract class AbstractRecursivePass implements CompilerPassInterface
{
/**
* @var ContainerBuilder
*/
protected $container;
protected $currentId;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,15 @@

namespace Symfony\Component\DependencyInjection\Compiler;

@trigger_error('The '.__NAMESPACE__.'\AutowireExceptionPass class is deprecated since version 3.4 and will be removed in 4.0. Use the DefinitionErrorExceptionPass class instead.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

missing annotation on the class


use Symfony\Component\DependencyInjection\ContainerBuilder;

/**
* Throws autowire exceptions from AutowirePass for definitions that still exist.
*
* @deprecated since version 3.4, will be removed in 4.0.
*
* @author Ryan Weaver <ryan@knpuniversity.com>
*/
class AutowireExceptionPass implements CompilerPassInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,22 @@ class AutowirePass extends AbstractRecursivePass
private $autowiringExceptions = array();

/**
* @param bool $throwOnAutowireException If false, retrieved errors via getAutowiringExceptions
* @param bool $throwOnAutowireException Errors can be retrieved via Definition::getErrors()
*/
public function __construct($throwOnAutowireException = true)
{
$this->throwOnAutowiringException = $throwOnAutowireException;
}

/**
* @deprecated since version 3.4, to be removed in 4.0.
*
* @return AutowiringFailedException[]
*/
public function getAutowiringExceptions()
{
@trigger_error('Calling AutowirePass::getAutowiringExceptions() is deprecated since Symfony 3.4 and will be removed in 4.0. Use Definition::getErrors() instead.', E_USER_DEPRECATED);

return $this->autowiringExceptions;
}

Expand Down Expand Up @@ -106,6 +110,7 @@ protected function processValue($value, $isRoot = false)
}

$this->autowiringExceptions[] = $e;
$this->container->getDefinition($this->currentId)->addError($e->getMessage());

return parent::processValue($value, $isRoot);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@
*/
class CheckArgumentsValidityPass extends AbstractRecursivePass
{
private $throwExceptions;

public function __construct($throwExceptions = true)
{
$this->throwExceptions = $throwExceptions;
}

/**
* {@inheritdoc}
*/
Expand All @@ -35,10 +42,20 @@ protected function processValue($value, $isRoot = false)
foreach ($value->getArguments() as $k => $v) {
if ($k !== $i++) {
if (!is_int($k)) {
throw new RuntimeException(sprintf('Invalid constructor argument for service "%s": integer expected but found string "%s". Check your service definition.', $this->currentId, $k));
$msg = sprintf('Invalid constructor argument for service "%s": integer expected but found string "%s". Check your service definition.', $this->currentId, $k);
$value->addError($msg);
if ($this->throwExceptions) {
throw new RuntimeException($msg);
}

break;
}

throw new RuntimeException(sprintf('Invalid constructor argument %d for service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $this->currentId, $i));
$msg = sprintf('Invalid constructor argument %d for service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $this->currentId, $i);
$value->addError($msg);
if ($this->throwExceptions) {
throw new RuntimeException($msg);
}
}
}

Expand All @@ -47,10 +64,20 @@ protected function processValue($value, $isRoot = false)
foreach ($methodCall[1] as $k => $v) {
if ($k !== $i++) {
if (!is_int($k)) {
throw new RuntimeException(sprintf('Invalid argument for method call "%s" of service "%s": integer expected but found string "%s". Check your service definition.', $methodCall[0], $this->currentId, $k));
$msg = sprintf('Invalid argument for method call "%s" of service "%s": integer expected but found string "%s". Check your service definition.', $methodCall[0], $this->currentId, $k);
$value->addError($msg);
if ($this->throwExceptions) {
throw new RuntimeException($msg);
}

break;
}

throw new RuntimeException(sprintf('Invalid argument %d for method call "%s" of service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $methodCall[0], $this->currentId, $i));
$msg = sprintf('Invalid argument %d for method call "%s" of service "%s": argument %d must be defined before. Check your service definition.', 1 + $k, $methodCall[0], $this->currentId, $i);
$value->addError($msg);
if ($this->throwExceptions) {
throw new RuntimeException($msg);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?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\Definition;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;

/**
* Throws an exception for any Definitions that have errors and still exist.
*
* @author Ryan Weaver <ryan@knpuniversity.com>
*/
class DefinitionErrorExceptionPass extends AbstractRecursivePass
{
/**
* {@inheritdoc}
*/
protected function processValue($value, $isRoot = false)
{
if (!$value instanceof Definition || empty($value->getErrors())) {
return parent::processValue($value, $isRoot);
}

// only show the first error so they user can focus on it
$errors = $value->getErrors();
$message = reset($errors);

throw new RuntimeException($message);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,14 @@ public function setRepeatedPass(RepeatedPass $repeatedPass)
*
* The key is the inlined service id and its value is the list of services it was inlined into.
*
* @deprecated since version 3.4, to be removed in 4.0.
*
* @return array
*/
public function getInlinedServiceIds()
{
@trigger_error('Calling InlineServiceDefinitionsPass::getInlinedServiceIds() is deprecated since Symfony 3.4 and will be removed in 4.0.', E_USER_DEPRECATED);

return $this->inlinedServiceIds;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@ public function __construct()
new RegisterServiceSubscribersPass(),
new ResolveNamedArgumentsPass(),
new ResolveBindingsPass(),
$autowirePass = new AutowirePass(false),
new AutowirePass(false),
new ResolveServiceSubscribersPass(),
new ResolveReferencesToAliasesPass(),
new ResolveInvalidReferencesPass(),
new AnalyzeServiceReferencesPass(true),
new CheckCircularReferencesPass(),
new CheckReferenceValidityPass(),
new CheckArgumentsValidityPass(),
new CheckArgumentsValidityPass(false),
));

$this->removingPasses = array(array(
Expand All @@ -75,11 +75,11 @@ public function __construct()
new RemoveAbstractDefinitionsPass(),
new RepeatedPass(array(
new AnalyzeServiceReferencesPass(),
$inlinedServicePass = new InlineServiceDefinitionsPass(),
new InlineServiceDefinitionsPass(),
new AnalyzeServiceReferencesPass(),
new RemoveUnusedDefinitionsPass(),
)),
new AutowireExceptionPass($autowirePass, $inlinedServicePass),
new DefinitionErrorExceptionPass(),
new CheckExceptionOnInvalidReferenceBehaviorPass(),
));
}
Expand Down
21 changes: 21 additions & 0 deletions src/Symfony/Component/DependencyInjection/Definition.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class Definition
private $autowiringTypes = array();
private $changes = array();
private $bindings = array();
private $errors = array();

protected $arguments = array();

Expand Down Expand Up @@ -959,4 +960,24 @@ public function setBindings(array $bindings)

return $this;
}

/**
* Add an error that occurred when building this Definition.
*
* @param string $error
*/
public function addError($error)
{
$this->errors[] = $error;
}

/**
* Returns any errors that occurred while building this Definition.
*
* @return array
*/
public function getErrors()
{
return $this->errors;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Exception\AutowiringFailedException;

/**
* @group legacy
*/
class AutowireExceptionPassTest extends TestCase
{
public function testThrowsException()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ public function testCompleteExistingDefinitionWithNotDefinedArguments()
$this->assertEquals(DInterface::class, (string) $container->getDefinition('h')->getArgument(1));
}

/**
* @group legacy
*/
public function testExceptionsAreStored()
{
$container = new ContainerBuilder();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,15 @@ public function definitionProvider()
array(array(), array(array('baz', array(1 => 1)))),
);
}

public function testNoException()
{
$container = new ContainerBuilder();
$definition = $container->register('foo');
$definition->setArguments(array(null, 'a' => 'a'));

$pass = new CheckArgumentsValidityPass(false);
$pass->process($container);
$this->assertCount(1, $definition->getErrors());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?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\DefinitionErrorExceptionPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;

class DefinitionErrorExceptionPassTest extends TestCase
{
/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException
* @expectedExceptionMessage Things went wrong!
*/
public function testThrowsException()
{
$container = new ContainerBuilder();
$def = new Definition();
$def->addError('Things went wrong!');
$def->addError('Now something else!');
$container->register('foo_service_id')
->setArguments(array(
$def,
));

$pass = new DefinitionErrorExceptionPass();
$pass->process($container);
}

public function testNoExceptionThrown()
{
$container = new ContainerBuilder();
$def = new Definition();
$container->register('foo_service_id')
->setArguments(array(
$def,
));

$pass = new DefinitionErrorExceptionPass();
$pass->process($container);
$this->assertSame($def, $container->getDefinition('foo_service_id')->getArgument(0));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ public function testProcessDoesNotSetLazyArgumentValuesAfterInlining()
$this->assertSame('inline', (string) $values[0]);
}

/**
* @group legacy
*/
public function testGetInlinedServiceIdData()
{
$container = new ContainerBuilder();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,4 +387,13 @@ public function testShouldAutoconfigure()
$def->setAutoconfigured(true);
$this->assertTrue($def->isAutoconfigured());
}

public function testAddError()
{
$def = new Definition('stdClass');
$this->assertEmpty($def->getErrors());
$def->addError('First error');
$def->addError('Second error');
$this->assertSame(array('First error', 'Second error'), $def->getErrors());
}
}