Skip to content

Commit

Permalink
feature #24484 [DI] Throw accurate failures when accessing removed se…
Browse files Browse the repository at this point in the history 
…rvices (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Throw accurate failures when accessing removed services

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24444
| License       | MIT
| Doc PR        | -

See linked issue.
This will throw a useful message when accessing a removed service.
When setting a removed service, a deprecation notice will be thrown also so that in master we can throw an exception then.

Commits
-------

fe7f26d [DI] Throw accurate failures when accessing removed services
  • Loading branch information
@fabpot
fabpot committed Oct 10, 2017
2 parents 5904d34 + fe7f26d commit d48bcbf
Show file tree
Hide file tree
Showing 33 changed files with 377 additions and 77 deletions.
50 changes: 35 additions & 15 deletions src/Symfony/Component/DependencyInjection/Container.php
View file
Expand Up @@ -50,6 +50,7 @@ class Container implements ResettableContainerInterface
protected $aliases = array();
protected $loading = array();
protected $resolving = array();
protected $syntheticIds = array();

/**
* @internal
Expand Down Expand Up @@ -179,31 +180,34 @@ public function set($id, $service)
throw new InvalidArgumentException('You cannot set service "service_container".');
}

if (isset($this->aliases[$id])) {
unset($this->aliases[$id]);
}

$wasSet = isset($this->services[$id]);
$this->services[$id] = $service;

if (null === $service) {
unset($this->services[$id]);
}

if (isset($this->privates[$id])) {
if (null === $service) {
if (isset($this->privates[$id]) || !(isset($this->fileMap[$id]) || isset($this->methodMap[$id]))) {
if (isset($this->syntheticIds[$id]) || (!isset($this->privates[$id]) && !isset($this->getRemovedIds()[$id]))) {
// no-op
} elseif (null === $service) {
@trigger_error(sprintf('The "%s" service is private, unsetting it is deprecated since Symfony 3.2 and will fail in 4.0.', $id), E_USER_DEPRECATED);
unset($this->privates[$id]);
} else {
@trigger_error(sprintf('The "%s" service is private, replacing it is deprecated since Symfony 3.2 and will fail in 4.0.', $id), E_USER_DEPRECATED);
}
} elseif ($wasSet && (isset($this->fileMap[$id]) || isset($this->methodMap[$id]))) {
} elseif (isset($this->services[$id])) {
if (null === $service) {
@trigger_error(sprintf('The "%s" service is already initialized, unsetting it is deprecated since Symfony 3.3 and will fail in 4.0.', $id), E_USER_DEPRECATED);
} else {
@trigger_error(sprintf('The "%s" service is already initialized, replacing it is deprecated since Symfony 3.3 and will fail in 4.0.', $id), E_USER_DEPRECATED);
}
}

if (isset($this->aliases[$id])) {
unset($this->aliases[$id]);
}

if (null === $service) {
unset($this->services[$id]);

return;
}

$this->services[$id] = $service;
}

/**
Expand Down Expand Up @@ -275,7 +279,7 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
// calling $this->normalizeId($id) unless necessary.
for ($i = 2;;) {
if (isset($this->privates[$id])) {
@trigger_error(sprintf('The "%s" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.', $id), E_USER_DEPRECATED);
@trigger_error(sprintf('The "%s" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.', $id), E_USER_DEPRECATED);
}
if (isset($this->aliases[$id])) {
$id = $this->aliases[$id];
Expand Down Expand Up @@ -325,6 +329,12 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
if (!$id) {
throw new ServiceNotFoundException($id);
}
if (isset($this->syntheticIds[$id])) {
throw new ServiceNotFoundException($id, null, null, array(), sprintf('The "%s" service is synthetic, it needs to be set at boot time before it can be used.', $id));
}
if (isset($this->getRemovedIds()[$id])) {
throw new ServiceNotFoundException($id, null, null, array(), sprintf('The "%s" service or alias has been removed or inlined when the container was compiled. You should either make it public, or stop using the container directly and use dependency injection instead.', $id));
}

$alternatives = array();
foreach ($this->getServiceIds() as $knownId) {
Expand Down Expand Up @@ -397,6 +407,16 @@ public function getServiceIds()
return array_unique(array_merge($ids, array_keys($this->methodMap), array_keys($this->fileMap), array_keys($this->services)));
}

/**
* Gets service ids that existed at compile time.
*
* @return array
*/
public function getRemovedIds()
{
return array();
}

/**
* Camelizes a string.
*
Expand Down
28 changes: 23 additions & 5 deletions src/Symfony/Component/DependencyInjection/ContainerBuilder.php
View file
Expand Up @@ -121,6 +121,8 @@ class ContainerBuilder extends Container implements TaggedContainerInterface

private $autoconfiguredInstanceof = array();

private $removedIds = array();

public function __construct(ParameterBagInterface $parameterBag = null)
{
parent::__construct($parameterBag);
Expand Down Expand Up @@ -517,7 +519,7 @@ public function set($id, $service)
throw new BadMethodCallException(sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a compiled container is not allowed.', $id));
}

unset($this->definitions[$id], $this->aliasDefinitions[$id]);
unset($this->definitions[$id], $this->aliasDefinitions[$id], $this->removedIds[$id]);

parent::set($id, $service);
}
Expand All @@ -529,7 +531,10 @@ public function set($id, $service)
*/
public function removeDefinition($id)
{
unset($this->definitions[$this->normalizeId($id)]);
if (isset($this->definitions[$id = $this->normalizeId($id)])) {
unset($this->definitions[$id]);
$this->removedIds[$id] = true;
}
}

/**
Expand Down Expand Up @@ -793,6 +798,16 @@ public function getServiceIds()
return array_unique(array_merge(array_keys($this->getDefinitions()), array_keys($this->aliasDefinitions), parent::getServiceIds()));
}

/**
* Gets removed service or alias ids.
*
* @return array
*/
public function getRemovedIds()
{
return $this->removedIds;
}

/**
* Adds the service aliases.
*
Expand Down Expand Up @@ -841,7 +856,7 @@ public function setAlias($alias, $id)
throw new InvalidArgumentException(sprintf('An alias can not reference itself, got a circular reference on "%s".', $alias));
}

unset($this->definitions[$alias]);
unset($this->definitions[$alias], $this->removedIds[$alias]);

return $this->aliasDefinitions[$alias] = $id;
}
Expand All @@ -853,7 +868,10 @@ public function setAlias($alias, $id)
*/
public function removeAlias($alias)
{
unset($this->aliasDefinitions[$this->normalizeId($alias)]);
if (isset($this->aliasDefinitions[$alias = $this->normalizeId($alias)])) {
unset($this->aliasDefinitions[$alias]);
$this->removedIds[$alias] = true;
}
}

/**
Expand Down Expand Up @@ -981,7 +999,7 @@ public function setDefinition($id, Definition $definition)

$id = $this->normalizeId($id);

unset($this->aliasDefinitions[$id]);
unset($this->aliasDefinitions[$id], $this->removedIds[$id]);

return $this->definitions[$id] = $definition;
}
Expand Down
65 changes: 65 additions & 0 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Expand Up @@ -172,6 +172,16 @@ public function dump(array $options = array())
EOF;
$files = array();

if ($ids = array_keys($this->container->getRemovedIds())) {
sort($ids);
$c = "<?php\n\nreturn array(\n";
foreach ($ids as $id) {
$c .= ' '.$this->export($id)." => true,\n";
}
$files['removed-ids.php'] = $c .= ");\n";
}

foreach ($this->generateServiceFiles() as $file => $c) {
$files[$file] = $fileStart.$c;
}
Expand Down Expand Up @@ -888,6 +898,7 @@ public function __construct()
}

$code .= $this->addNormalizedIds();
$code .= $this->addSyntheticIds();
$code .= $this->addMethodMap();
$code .= $this->asFiles ? $this->addFileMap() : '';
$code .= $this->addPrivateServices();
Expand All @@ -896,6 +907,8 @@ public function __construct()
}
EOF;
$code .= $this->addRemovedIds();

if ($this->container->isCompiled()) {
$code .= <<<EOF
Expand Down Expand Up @@ -978,6 +991,58 @@ private function addNormalizedIds()
return $code ? " \$this->normalizedIds = array(\n".$code." );\n" : '';
}

/**
* Adds the syntheticIds definition.
*
* @return string
*/
private function addSyntheticIds()
{
$code = '';
$definitions = $this->container->getDefinitions();
ksort($definitions);
foreach ($definitions as $id => $definition) {
if ($definition->isSynthetic() && 'service_container' !== $id) {
$code .= ' '.$this->export($id)." => true,\n";
}
}

return $code ? " \$this->syntheticIds = array(\n{$code} );\n" : '';
}

/**
* Adds the removedIds definition.
*
* @return string
*/
private function addRemovedIds()
{
if (!$ids = $this->container->getRemovedIds()) {
return '';
}
if ($this->asFiles) {
$code = "require __DIR__.'/removed-ids.php'";
} else {
$code = '';
$ids = array_keys($ids);
sort($ids);
foreach ($ids as $id) {
$code .= ' '.$this->export($id)." => true,\n";
}

$code = "array(\n{$code} )";
}

return <<<EOF
public function getRemovedIds()
{
return {$code};
}
EOF;
}

/**
* Adds the methodMap property definition.
*
Expand Down
6 changes: 4 additions & 2 deletions src/Symfony/Component/DependencyInjection/Exception/ServiceNotFoundException.php
View file
Expand Up @@ -24,9 +24,11 @@ class ServiceNotFoundException extends InvalidArgumentException implements NotFo
private $sourceId;
private $alternatives;

public function __construct($id, $sourceId = null, \Exception $previous = null, array $alternatives = array())
public function __construct($id, $sourceId = null, \Exception $previous = null, array $alternatives = array(), $msg = null)
{
if (null === $sourceId) {
if (null !== $msg) {
// no-op
} elseif (null === $sourceId) {
$msg = sprintf('You have requested a non-existent service "%s".', $id);
} else {
$msg = sprintf('The service "%s" has a dependency on a non-existent service "%s".', $sourceId, $id);
Expand Down
18 changes: 15 additions & 3 deletions src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
View file
Expand Up @@ -333,16 +333,28 @@ public function testGetCircularReference()

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException
* @expectedExceptionMessage You have requested a non-existent service "request".
* @expectedExceptionMessage The "request" service is synthetic, it needs to be set at boot time before it can be used.
*/
public function testGetSyntheticServiceThrows()
{
require_once __DIR__.'/Fixtures/php/services9.php';
require_once __DIR__.'/Fixtures/php/services9_compiled.php';

$container = new \ProjectServiceContainer();
$container->get('request');
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException
* @expectedExceptionMessage The "inlined" service or alias has been removed or inlined when the container was compiled. You should either make it public, or stop using the container directly and use dependency injection instead.
*/
public function testGetRemovedServiceThrows()
{
require_once __DIR__.'/Fixtures/php/services9_compiled.php';

$container = new \ProjectServiceContainer();
$container->get('inlined');
}

public function testHas()
{
$sc = new ProjectServiceContainer();
Expand Down Expand Up @@ -505,7 +517,7 @@ public function testCheckExistenceOfAnInternalPrivateServiceIsDeprecated()

/**
* @group legacy
* @expectedDeprecation The "internal" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "internal" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
*/
public function testRequestAnInternalSharedPrivateServiceIsDeprecated()
{
Expand Down
46 changes: 13 additions & 33 deletions src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
View file
Expand Up @@ -287,37 +287,17 @@ public function testFrozenContainerWithoutAliases()

/**
* @group legacy
* @expectedDeprecation The "bar" service is already initialized, replacing it is deprecated since Symfony 3.3 and will fail in 4.0.
* @expectedDeprecation The "decorator_service" service is already initialized, replacing it is deprecated since Symfony 3.3 and will fail in 4.0.
*/
public function testOverrideServiceWhenUsingADumpedContainer()
{
require_once self::$fixturesPath.'/php/services9.php';
require_once self::$fixturesPath.'/includes/foo.php';
require_once self::$fixturesPath.'/php/services9_compiled.php';

$container = new \ProjectServiceContainer();
$container->setParameter('foo_bar', 'foo_bar');
$container->get('bar');
$container->set('bar', $bar = new \stdClass());

$this->assertSame($bar, $container->get('bar'), '->set() overrides an already defined service');
}

/**
* @group legacy
* @expectedDeprecation The "bar" service is already initialized, replacing it is deprecated since Symfony 3.3 and will fail in 4.0.
*/
public function testOverrideServiceWhenUsingADumpedContainerAndServiceIsUsedFromAnotherOne()
{
require_once self::$fixturesPath.'/php/services9.php';
require_once self::$fixturesPath.'/includes/foo.php';
require_once self::$fixturesPath.'/includes/classes.php';

$container = new \ProjectServiceContainer();
$container->setParameter('foo_bar', 'foo_bar');
$container->get('bar');
$container->set('bar', $bar = new \stdClass());
$container->get('decorator_service');
$container->set('decorator_service', $decorator = new \stdClass());

$this->assertSame($bar, $container->get('foo')->bar, '->set() overrides an already defined service');
$this->assertSame($decorator, $container->get('decorator_service'), '->set() overrides an already defined service');
}

/**
Expand Down Expand Up @@ -799,14 +779,14 @@ public function testDumpHandlesLiteralClassWithRootNamespace()

/**
* @group legacy
* @expectedDeprecation The "private" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "private_alias" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "decorated_private" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "decorated_private_alias" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "private_not_inlined" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "private_not_removed" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "private_child" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "private_parent" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop getting services directly from the container and use dependency injection instead.
* @expectedDeprecation The "private" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
* @expectedDeprecation The "private_alias" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
* @expectedDeprecation The "decorated_private" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
* @expectedDeprecation The "decorated_private_alias" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
* @expectedDeprecation The "private_not_inlined" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
* @expectedDeprecation The "private_not_removed" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
* @expectedDeprecation The "private_child" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
* @expectedDeprecation The "private_parent" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.
*/
public function testLegacyPrivateServices()
{
Expand Down

0 comments on commit d48bcbf

Please sign in to comment.