Skip to content

[DI] Deprecate Container::set for nulls, initialized and alias services #19668

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

Closed
wants to merge 1 commit into from
+74 −24
Closed

[DI] Deprecate Container::set for nulls, initialized and alias services #19668

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
3 changes: 1 addition & 2 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Templating/PhpEngineTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ public function testEvaluateWithoutAvailableRequest()
$loader = $this->getMockForAbstractClass('Symfony\Component\Templating\Loader\Loader');
$engine = new PhpEngine(new TemplateNameParser(), $container, $loader, new GlobalVariables($container));

$container->set('request_stack', null);

$this->assertFalse($container->has('request_stack'));
$globals = $engine->getGlobals();
$this->assertEmpty($globals['app']->getRequest());
}
Expand Down
30 changes: 17 additions & 13 deletions src/Symfony/Component/DependencyInjection/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,6 @@ public function setParameter($name, $value)
/**
* Sets a service.
*
* Setting a service to null resets the service: has() returns false and get()
* behaves in the same way as if the service was never created.
*
* @param string $id The service identifier
* @param object $service The service instance
*/
Expand All @@ -175,24 +172,31 @@ public function set($id, $service)
throw new InvalidArgumentException('You cannot set service "service_container".');
}

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

$this->services[$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);
unset($this->privates[$id]);
} 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);
}
} elseif (null === $service) {
@trigger_error(sprintf('Unsetting the "%s" service is deprecated since Symfony 3.3 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
unset($this->services[$id], $this->aliases[$id], $this->methodMap[$id]);

return;
}

if (isset($this->aliases[$id])) {
@trigger_error(sprintf('Replacing the "%s" service alias is deprecated since Symfony 3.3 and will set the aliased service instead in Symfony 4.0.', $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.

I'm sure I'm missing something here :)
What will be the code/behavior you'd expect on 4.0? Not an exception?

Copy link
Contributor Author

@ro0NL ro0NL Jan 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

$id = $this->aliases[$id] and forward :) i think it's ok.

Copy link
Contributor Author

@ro0NL ro0NL Jan 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

return $this->set($this->aliases[$id], $service); actually.

Copy link
Member

Choose a reason for hiding this comment

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

OK. I'd be stricter here, and plan to throw also. No need for any special case, from the outside POV, alias or not, redefining something should throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also leaning that way.. makes sense. Lets do #21244 first, ill finish this one the weekend.

unset($this->aliases[$id]);
} elseif (isset($this->services[$id])) {
@trigger_error(sprintf('Replacing the initialized "%s" service is deprecated since Symfony 3.3 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
} elseif (isset($this->methodMap[$id])) {
@trigger_error(sprintf('Replacing the pre-defined "%s" service is deprecated since Symfony 3.3 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
unset($this->methodMap[$id]);
}
Copy link
Member

@nicolas-grekas nicolas-grekas Jan 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

there is a missing case here, which is in fact what's described in #19192: initialized or not, we should deprecate setting a pre-defined non-synthetic services. This means we should check methodsMap and if there is a match, trigger the deprecation.


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

/**
Expand Down
53 changes: 45 additions & 8 deletions src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,27 @@ public function testSet()
$this->assertSame($foo, $sc->get('foo'), '->set() sets a service');
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

this test should not be removed but be marked as @group legacy, as it covers the legacy feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stof i moved it back down, after other legacy tests. This is my first time on this, so i wasnt sure.

Copy link
Member

Choose a reason for hiding this comment

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

@ro0NL please move the test here again to ease merges

Copy link
Member

Choose a reason for hiding this comment

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

ease merges and reviews in fact

Copy link
Member

Choose a reason for hiding this comment

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

ping @ro0NL

* @group legacy
* @expectedDeprecation Unsetting the "foo" service is deprecated since Symfony 3.3 and won't be supported anymore in Symfony 4.0.
* @expectedDeprecation Unsetting the "bar" service is deprecated since Symfony 3.3 and won't be supported anymore in Symfony 4.0.
*/
public function testSetWithNullResetTheService()
{
$sc = new Container();
$sc->set('foo', new \stdClass());
$sc->set('foo', null);
$this->assertFalse($sc->has('foo'), '->set() with null service resets the service');

$sc = new ProjectServiceContainer();
$sc->set('bar', null);
$this->assertFalse($sc->has('bar'), '->set() with null service resets the pre-defined service');
}

/**
* @group legacy
* @expectedDeprecation Replacing the "alias" service alias is deprecated since Symfony 3.3 and will set the aliased service instead in Symfony 4.0.
*/
public function testSetReplacesAlias()
{
$c = new ProjectServiceContainer();
Expand All @@ -172,9 +186,6 @@ public function testGet()
$this->assertSame($sc->__foo_bar, $sc->get('foo_bar'), '->get() returns the service if a get*Method() is defined');
$this->assertSame($sc->__foo_baz, $sc->get('foo.baz'), '->get() returns the service if a get*Method() is defined');

$sc->set('bar', $bar = new \stdClass());
$this->assertSame($bar, $sc->get('bar'), '->get() prefers to return a service defined with set() than one defined with a getXXXMethod()');

try {
$sc->get('');
$this->fail('->get() throws a \InvalidArgumentException exception if the service is empty');
Expand Down Expand Up @@ -337,7 +348,7 @@ public function testInitialized()
$this->assertFalse($sc->initialized('bar'), '->initialized() returns false if a service is defined, but not currently loaded');
$this->assertFalse($sc->initialized('alias'), '->initialized() returns false if an aliased service is not initialized');

$sc->set('bar', new \stdClass());
$sc->get('bar');
$this->assertTrue($sc->initialized('alias'), '->initialized() returns true for alias if aliased service is initialized');
}

Expand Down Expand Up @@ -453,6 +464,32 @@ public function testRequestAnInternalSharedPrivateServiceIsDeprecated()
$c = new ProjectServiceContainer();
$c->get('internal');
}

/**
* @group legacy
* @expectedDeprecation Replacing the pre-defined "bar" service is deprecated since Symfony 3.3 and won't be supported anymore in Symfony 4.0.
*/
public function testReplacingAPreDefinedServiceIsDeprecated()
{
$c = new ProjectServiceContainer();
$c->set('bar', new \stdClass());
$c->set('bar', $bar = new \stdClass());

$this->assertSame($bar, $c->get('bar'), '->set() replaces a pre-defined service');
}

/**
* @group legacy
* @expectedDeprecation Replacing the initialized "new" service is deprecated since Symfony 3.3 and won't be supported anymore in Symfony 4.0.
*/
public function testReplacingAnInitializedServiceIsDeprecated()
{
$c = new ProjectServiceContainer();
$c->set('new', new \stdClass());
$c->set('new', $new = new \stdClass());

$this->assertEquals($new, $c->get('new'), '->set() replaces an initialized service');
}
}

class ProjectServiceContainer extends Container
Expand Down Expand Up @@ -485,22 +522,22 @@ public function __construct()

protected function getInternalService()
{
return $this->__internal;
return $this->services['internal'] = $this->__internal;
}

protected function getBarService()
{
return $this->__bar;
return $this->services['bar'] = $this->__bar;
}

protected function getFooBarService()
{
return $this->__foo_bar;
return $this->services['foo_bar'] = $this->__foo_bar;
}

protected function getFoo_BazService()
{
return $this->__foo_baz;
return $this->services['foo.baz'] = $this->__foo_baz;
}

protected function getCircularService()
Expand Down
11 changes: 10 additions & 1 deletion src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,13 @@ public function provideInvalidFactories()
public function testAliases()
{
$container = include self::$fixturesPath.'/containers/container9.php';
$container->setParameter('foo_bar', 'foo_bar');
$container->compile();
$dumper = new PhpDumper($container);
eval('?>'.$dumper->dump(array('class' => 'Symfony_DI_PhpDumper_Test_Aliases')));

$container = new \Symfony_DI_PhpDumper_Test_Aliases();
$container->set('foo', $foo = new \stdClass());
$foo = $container->get('foo');
$this->assertSame($foo, $container->get('foo'));
$this->assertSame($foo, $container->get('alias_for_foo'));
$this->assertSame($foo, $container->get('alias_for_alias'));
Expand All @@ -263,6 +264,10 @@ public function testFrozenContainerWithoutAliases()
$this->assertFalse($container->has('foo'));
}

/**
* @group legacy
* @expectedDeprecation Replacing the pre-defined "bar" service is deprecated since Symfony 3.3 and won't be supported anymore in Symfony 4.0.
*/
public function testOverrideServiceWhenUsingADumpedContainer()
{
require_once self::$fixturesPath.'/php/services9.php';
Expand All @@ -275,6 +280,10 @@ public function testOverrideServiceWhenUsingADumpedContainer()
$this->assertSame($bar, $container->get('bar'), '->set() overrides an already defined service');
}

/**
* @group legacy
* @expectedDeprecation Replacing the pre-defined "bar" service is deprecated since Symfony 3.3 and won't be supported anymore in Symfony 4.0.
*/
public function testOverrideServiceWhenUsingADumpedContainerAndServiceIsUsedFromAnotherOne()
{
require_once self::$fixturesPath.'/php/services9.php';
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container9.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<?php

require_once __DIR__.'/../includes/classes.php';
require_once __DIR__.'/../includes/foo.php';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allows to actually call $container->get('foo') (class not found error otherwise).


use Symfony\Component\DependencyInjection\Argument\ClosureProxyArgument;
use Symfony\Component\DependencyInjection\Argument\IteratorArgument;
Expand Down