Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[DependencyInjection] deprecate access to private shared services. #19146

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.

Sign up for GitHub

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
Jun 29, 2016
Merged

[DependencyInjection] deprecate access to private shared services. #19146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions 9 UPGRADE-4.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ DependencyInjection

* Using unsupported options to configure service aliases raises an exception.

* Setting or unsetting a private service with the `Container::set()` method is
no longer supported. Only public services can be set or unset.

* Checking the existence of a private service with the `Container::has()`
method is no longer supported and will return `false`.

* Requesting a private service with the `Container::get()` method is no longer
supported.

Copy link
Member

Choose a reason for hiding this comment

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

You also need to add a note in the component CHANGELOG file.

Form
----

Expand Down
3 changes: 3 additions & 0 deletions 3 src/Symfony/Component/DependencyInjection/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ CHANGELOG

* allowed to prioritize compiler passes by introducing a third argument to `PassConfig::addPass()`, to `Compiler::addPass` and to `ContainerBuilder::addCompilerPass()`
* added support for PHP constants in YAML configuration files
* deprecated the ability to set or unset a private service with the `Container::set()` method
* deprecated the ability to check for the existence of a private service with the `Container::has()` method
* deprecated the ability to request a private service with the `Container::get()` method

3.0.0
-----
Expand Down
16 changes: 16 additions & 0 deletions 16 src/Symfony/Component/DependencyInjection/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class Container implements ResettableContainerInterface

protected $services = array();
protected $methodMap = array();
protected $privates = array();
protected $aliases = array();
protected $loading = array();

Expand Down Expand Up @@ -176,6 +177,14 @@ public function set($id, $service)
if (null === $service) {
unset($this->services[$id]);
}

if (isset($this->privates[$id])) {
if (null === $service) {
@trigger_error(sprintf('Unsetting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
} else {
@trigger_error(sprintf('Setting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. A new public service will be created instead.', $id), 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.

If we don't keep the private var in 4.0 but replace it with random names, setting here will work as the container will just define a new service, which will probably confuse people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be confusing for people thinking they are overriding a private service but they'll get a new one in the end...

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a bad practice imo. Why would someone override a service if it is not synthetic ?

Copy link
Member

@nicolas-grekas nicolas-grekas Jun 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @fabpot here, it's the same for private properties on objects btw: they exists from the inside but not from the outside so that one can use the prop name without any collision in a child class.

Copy link
Member

@nicolas-grekas nicolas-grekas Jun 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Which makes me wonder: is set() really useful on containers? Shouldn't we deprecate it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolas-grekas it's useful for synthetic services at least.

Copy link
Contributor Author

@hhamon hhamon Jun 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering if we should two different Container classes in Symfony 4.0 instead. Something like a SimpleServiceContainer class that doesn't support private services and that justs allows to register and resolve definitions. Then the real Container class that allows to support private services (all other fancy advanced features) as well as dumped containers when they're built from a ContainerBuilder.

Copy link
Member

Choose a reason for hiding this comment

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

I suggested that before when we deprecated calling get() before the container is compiled: #18728 (comment)

So yes I would say let's deprecate the method entirely for services that are not synthetic.

Copy link
Member

Choose a reason for hiding this comment

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

set('doctrine.orm.entity_manager', null) is used by the Doctrine manager registry (in the bridge) to reset the entity manager service (due to the fact that the entity manager becomes unusable after an exception during a flush, because the unit of work is know to be in a broken state).
So we should still at least support this use case.
Allowing to reset an existing instance of the ORM cannot happen without a major change in the ORM (which I will ask to be done in Doctrine 3, but this is not planned to happen soon), so we need to be able to reset the service, to be able to use the ORM in long running scripts like a RabbitMQ consumer (if we cannot reset the service, the only solution would be to kill the process)

Copy link
Member

Choose a reason for hiding this comment

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

See #19192

}
}

/**
Expand All @@ -198,6 +207,10 @@ public function has($id)
if (--$i && $id !== $lcId = strtolower($id)) {
$id = $lcId;
} else {
if (isset($this->privates[$id])) {
@trigger_error(sprintf('Checking for the existence of the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
}

return method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service');
}
}
Expand Down Expand Up @@ -268,6 +281,9 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE

return;
}
if (isset($this->privates[$id])) {
@trigger_error(sprintf('Requesting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
}

$this->loading[$id] = true;

Expand Down
11 changes: 6 additions & 5 deletions 11 src/Symfony/Component/DependencyInjection/ContainerBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,11 +559,12 @@ public function compile()
$compiler->compile($this);
$this->compiled = true;

if ($this->trackResources) {
foreach ($this->definitions as $definition) {
if ($definition->isLazy() && ($class = $definition->getClass()) && class_exists($class)) {
$this->addClassResource(new \ReflectionClass($class));
}
foreach ($this->definitions as $id => $definition) {
if (!$definition->isPublic()) {
$this->privates[$id] = true;
}
if ($this->trackResources && $definition->isLazy() && ($class = $definition->getClass()) && class_exists($class)) {
$this->addClassResource(new \ReflectionClass($class));
}
}

Expand Down
31 changes: 31 additions & 0 deletions 31 src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,7 @@ public function __construct()
EOF;

$code .= $this->addMethodMap();
$code .= $this->addPrivateServices();
$code .= $this->addAliases();

$code .= <<<'EOF'
Expand Down Expand Up @@ -888,6 +889,36 @@ private function addMethodMap()
return $code." );\n";
}

/**
* Adds the privates property definition.
*
* @return string
*/
private function addPrivateServices()
{
if (!$definitions = $this->container->getDefinitions()) {
return '';
}

$code = '';
ksort($definitions);
foreach ($definitions as $id => $definition) {
if (!$definition->isPublic()) {
$code .= ' '.var_export($id, true)." => true,\n";
}
}

if (empty($code)) {
return '';
}

$out = " \$this->privates = array(\n";
$out .= $code;
$out .= " );\n";

return $out;
}

/**
* Adds the aliases property definition.
*
Expand Down
64 changes: 62 additions & 2 deletions 64 src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\DependencyInjection\Tests;

use Symfony\Bridge\PhpUnit\ErrorAssert;
use Symfony\Component\DependencyInjection\Container;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
Expand Down Expand Up @@ -125,7 +126,7 @@ public function testGetServiceIds()

$sc = new ProjectServiceContainer();
$sc->set('foo', $obj = new \stdClass());
$this->assertEquals(array('bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'service_container', 'foo'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids by getXXXService() methods, followed by service ids defined by set()');
$this->assertEquals(array('internal', 'bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'service_container', 'foo'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids by getXXXService() methods, followed by service ids defined by set()');
}

public function testSet()
Expand Down Expand Up @@ -306,11 +307,63 @@ public function testThatCloningIsNotSupported()
$this->assertFalse($class->isCloneable());
$this->assertTrue($clone->isPrivate());
}

/** @group legacy */
public function testUnsetInternalPrivateServiceIsDeprecated()
{
$deprecations = array(
'Unsetting the "internal" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.',
);

ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () {
$c = new ProjectServiceContainer();
$c->set('internal', null);
});
}

/** @group legacy */
public function testChangeInternalPrivateServiceIsDeprecated()
{
$deprecations = array(
'Setting the "internal" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. A new public service will be created instead.',
);

ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () {
$c = new ProjectServiceContainer();
$c->set('internal', new \stdClass());
});
}

/** @group legacy */
public function testCheckExistenceOfAnInternalPrivateServiceIsDeprecated()
{
$deprecations = array(
'Checking for the existence of the "internal" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.',
);

ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () {
$c = new ProjectServiceContainer();
$c->has('internal');
});
}

/** @group legacy */
public function testRequestAnInternalSharedPrivateServiceIsDeprecated()
{
$deprecations = array(
'Requesting the "internal" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.',
);

ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () {
$c = new ProjectServiceContainer();
$c->get('internal');
});
}
}

class ProjectServiceContainer extends Container
{
public $__bar, $__foo_bar, $__foo_baz;
public $__bar, $__foo_bar, $__foo_baz, $__internal;

public function __construct()
{
Expand All @@ -319,9 +372,16 @@ public function __construct()
$this->__bar = new \stdClass();
$this->__foo_bar = new \stdClass();
$this->__foo_baz = new \stdClass();
$this->__internal = new \stdClass();
$this->privates = array('internal' => true);
$this->aliases = array('alias' => 'bar');
}

protected function getInternalService()
{
return $this->__internal;
}

protected function getBarService()
{
return $this->__bar;
Expand Down
7 changes: 7 additions & 0 deletions 7 src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ public function __construct()
'request' => 'getRequestService',
'service_from_static_method' => 'getServiceFromStaticMethodService',
);
$this->privates = array(
'configurator_service' => true,
'configurator_service_simple' => true,
'factory_simple' => true,
'inlined' => true,
'new_factory' => true,
);
$this->aliases = array(
'alias_for_alias' => 'foo',
'alias_for_foo' => 'foo',
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.