Skip to content
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

[DI] Add missing deprecation when fetching private services from ContainerBuilder #25244

Merged
merged 1 commit into from Dec 1, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -232,9 +232,10 @@ public function testStopwatchExtensionAvailability($debug, $stopwatchEnabled, $e
}
$container->registerExtension(new TwigExtension());
$container->loadFromExtension('twig', array());
$container->setAlias('test.twig.extension.debug.stopwatch', 'twig.extension.debug.stopwatch')->setPublic(true);
$this->compileContainer($container);

$tokenParsers = $container->get('twig.extension.debug.stopwatch')->getTokenParsers();
$tokenParsers = $container->get('test.twig.extension.debug.stopwatch')->getTokenParsers();
$stopwatchIsAvailable = new \ReflectionProperty($tokenParsers[0], 'stopwatchIsAvailable');
$stopwatchIsAvailable->setAccessible(true);

Expand Down
17 changes: 8 additions & 9 deletions src/Symfony/Component/DependencyInjection/ContainerBuilder.php
Expand Up @@ -556,15 +556,8 @@ public function has($id)
*/
public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE)
{
if ($this->isCompiled()) {
$id = $this->normalizeId($id);

if (isset($this->definitions[$id]) && $this->definitions[$id]->isPrivate()) {
@trigger_error(sprintf('Fetching the "%s" private service is deprecated and will fail in Symfony 4.0. Make the service public instead.', $id), E_USER_DEPRECATED);
}
if (isset($this->aliasDefinitions[$id]) && $this->aliasDefinitions[$id]->isPrivate()) {
@trigger_error(sprintf('Fetching the "%s" private alias is deprecated and will fail in Symfony 4.0. Make the alias public instead.', $id), E_USER_DEPRECATED);
}
if ($this->isCompiled() && isset($this->removedIds[$id = $this->normalizeId($id)])) {
@trigger_error(sprintf('Fetching the "%s" private service or alias is deprecated since Symfony 3.4 and will fail in 4.0. Make it public instead.', $id), E_USER_DEPRECATED);
}

return $this->doGet($id, $invalidBehavior);
Expand Down Expand Up @@ -776,6 +769,12 @@ public function compile(/*$resolveEnvPlaceholders = false*/)
}

parent::compile();

foreach ($this->definitions + $this->aliasDefinitions as $id => $definition) {
if (!$definition->isPublic() || $definition->isPrivate()) {
$this->removedIds[$id] = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is for ppl doing that, which is a no-op, but one we catch now:
image

}
}
}

/**
Expand Down
Expand Up @@ -733,6 +733,7 @@ public function testEnvInId()
PsrContainerInterface::class => true,
ContainerInterface::class => true,
'baz_%env(BAR)%' => true,
'bar_%env(BAR)%' => true,
Copy link
Member Author

Choose a reason for hiding this comment

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

the added entries are the same as on 4.0/master: we now list the public-by-private legacy services as removed, even if they're not really for BC in 3.4.

Copy link
Member

Choose a reason for hiding this comment

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

won't they appear as removed even if they are not in 4.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

wdyt? they are in 4.0

Copy link
Member

@chalasr chalasr Dec 1, 2017

Choose a reason for hiding this comment

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

definition isn't removed if injected twice or more, is it? If it isn't, wouldn't getRemovedIds() return an id that has not been removed? (I'm wondering why we populate removedIds from another place that the one which actually removes unused definitions)

);
$this->assertSame($expected, $container->getRemovedIds());

Expand Down
Expand Up @@ -9,8 +9,10 @@ return array(
'configurator_service_simple' => true,
'decorated.pif-pouf' => true,
'decorator_service.inner' => true,
'factory_simple' => true,
'inlined' => true,
'new_factory' => true,
'tagged_iterator_foo' => true,
);

[Container%s/getBazService.php] => <?php
Expand Down
Expand Up @@ -70,8 +70,10 @@ public function getRemovedIds()
'configurator_service_simple' => true,
'decorated.pif-pouf' => true,
'decorator_service.inner' => true,
'factory_simple' => true,
'inlined' => true,
'new_factory' => true,
'tagged_iterator_foo' => true,
);
}

Expand Down
Expand Up @@ -44,6 +44,7 @@ public function getRemovedIds()
return array(
'Psr\\Container\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
'bar_%env(BAR)%' => true,
'baz_%env(BAR)%' => true,
);
}
Expand Down
Expand Up @@ -57,6 +57,7 @@ public function getRemovedIds()
return array(
'Psr\\Container\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\includes\\HotPath\\C3' => true,
);
}

Expand Down
Expand Up @@ -60,9 +60,17 @@ public function getRemovedIds()
return array(
'Psr\\Container\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
'decorated_private' => true,
'decorated_private_alias' => true,
'foo' => true,
'private' => true,
'private_alias' => true,
'private_alias_decorator.inner' => true,
'private_child' => true,
'private_decorator.inner' => true,
'private_not_inlined' => true,
'private_not_removed' => true,
'private_parent' => true,
);
}

Expand Down
Expand Up @@ -45,6 +45,7 @@ public function getRemovedIds()
return array(
'Psr\\Container\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
'baz_service' => true,
'translator.loader_1_locator' => true,
'translator.loader_2_locator' => true,
'translator.loader_3_locator' => true,
Expand Down
Expand Up @@ -39,6 +39,7 @@ public function getRemovedIds()
return array(
'Psr\\Container\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
'baz_service' => true,
);
}

Expand Down
Expand Up @@ -39,6 +39,7 @@ public function getRemovedIds()
'Psr\\Container\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
'private_bar' => true,
'private_foo' => true,
);
}

Expand Down
Expand Up @@ -43,6 +43,7 @@ public function getRemovedIds()
return array(
'Psr\\Container\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\Tests\\Fixtures\\CustomDefinition' => true,
'service_locator.jmktfsv' => true,
);
}
Expand Down
Expand Up @@ -41,6 +41,7 @@ public function getRemovedIds()
'Psr\\Container\\ContainerInterface' => true,
'Symfony\\Component\\DependencyInjection\\ContainerInterface' => true,
'foo2' => true,
'foo3' => true,
);
}

Expand Down
Expand Up @@ -94,7 +94,7 @@ protected function instantiateController($class)
} catch (\TypeError $e) {
}

if ($this->container instanceof Container && in_array($class, $this->container->getRemovedIds(), true)) {
if ($this->container instanceof Container && isset($this->container->getRemovedIds()[$class])) {
throw new \LogicException(sprintf('Controller "%s" cannot be fetched from the container because it is private. Did you forget to tag the service with "controller.service_arguments"?', $class), 0, $e);
}

Expand Down
Expand Up @@ -130,7 +130,7 @@ public function testNonConstructController()
$container->expects($this->atLeastOnce())
->method('getRemovedIds')
->with()
->will($this->returnValue(array(ImpossibleConstructController::class)))
->will($this->returnValue(array(ImpossibleConstructController::class => true)))
;

$resolver = $this->createControllerResolver(null, $container);
Expand Down