Skip to content

Use the method map as authoritative list of factories for dumped containers #20113

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
Oct 8, 2016
Merged

Use the method map as authoritative list of factories for dumped containers #20113

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
4 changes: 4 additions & 0 deletions src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/Fixtures/php/lazy_service.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
*/
class LazyServiceProjectServiceContainer extends Container
{
protected $methodMap = array(
'foo' => 'getFooService',
);

/**
* Constructor.
*/
Expand Down
4 changes: 4 additions & 0 deletions src/Symfony/Bridge/ProxyManager/Tests/LazyProxy/Fixtures/php/lazy_service_with_hints.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
*/
class LazyServiceProjectServiceContainer extends Container
{
protected $methodMap = array(
'foo' => 'getFooService',
);

/**
* Constructor.
*/
Expand Down
44 changes: 34 additions & 10 deletions src/Symfony/Component/DependencyInjection/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,29 @@ public function has($id)
) {
return true;
}

if (isset($this->privates[$id])) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Reorganizing this code was done to match the way the code is organized in get, so that we first try to find the service as is, and then do the lowercase conversion if needed. This way, we won't make a strtolower call when checking ->has() for an existing service anymore when providing the lowercase string already.

@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);
}

if (isset($this->methodMap[$id])) {
return true;
}

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);
}
continue;
}

// We only check the convention-based factory in a compiled container (i.e. a child class other than a ContainerBuilder,
// and only when the dumper has not generated the method map (otherwise the method map is considered to be fully populated by the dumper)
if (!$this->methodMap && !$this instanceof ContainerBuilder && __CLASS__ !== static::class && method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service')) {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be cleaner to introduced a new interface for dumped containers instead of having this condition which hardcodes some names and conventions from the PHP dumper?

Copy link
Member

Choose a reason for hiding this comment

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

Or can' we just simplify this like that?
if (!$this->methodMap && method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service')) {

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicolas-grekas in get, we could. But in has, this would not allow to warn properly in all cases

@fabpot the convention is the deprecated one. Adding a new interface won't help, as non-updated dumpers would not implement this interface either, and updated dumpers will never enter this code due to !$this->methodMap. This code is precisely about handling things for custom dumpers to warn them to upgrade.

@trigger_error('Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.', E_USER_DEPRECATED);

return method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service');
return true;
}

return false;
}
}

Expand Down Expand Up @@ -262,7 +276,10 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
} elseif (--$i && $id !== $lcId = strtolower($id)) {
$id = $lcId;
continue;
} elseif (method_exists($this, $method = 'get'.strtr($id, $this->underscoreMap).'Service')) {
} elseif (!$this->methodMap && !$this instanceof ContainerBuilder && __CLASS__ !== static::class && method_exists($this, $method = 'get'.strtr($id, $this->underscoreMap).'Service')) {
// We only check the convention-based factory in a compiled container (i.e. a child class other than a ContainerBuilder,
// and only when the dumper has not generated the method map (otherwise the method map is considered to be fully populated by the dumper)
@trigger_error('Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.', E_USER_DEPRECATED);
// $method is set to the right value, proceed
} else {
if (self::EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior) {
Expand Down Expand Up @@ -341,14 +358,21 @@ public function reset()
public function getServiceIds()
{
$ids = array();
foreach (get_class_methods($this) as $method) {
if (preg_match('/^get(.+)Service$/', $method, $match)) {
$ids[] = self::underscore($match[1]);

if (!$this->methodMap && !$this instanceof ContainerBuilder && __CLASS__ !== static::class) {
// We only check the convention-based factory in a compiled container (i.e. a child class other than a ContainerBuilder,
// and only when the dumper has not generated the method map (otherwise the method map is considered to be fully populated by the dumper)
@trigger_error('Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.', E_USER_DEPRECATED);

foreach (get_class_methods($this) as $method) {
if (preg_match('/^get(.+)Service$/', $method, $match)) {
$ids[] = self::underscore($match[1]);
}
}
}
$ids[] = 'service_container';

return array_unique(array_merge($ids, array_keys($this->services)));
return array_unique(array_merge($ids, array_keys($this->methodMap), array_keys($this->services)));
}

/**
Expand Down
151 changes: 148 additions & 3 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 @@ -126,7 +126,25 @@ public function testGetServiceIds()

$sc = new ProjectServiceContainer();
$sc->set('foo', $obj = new \stdClass());
$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()');
$this->assertEquals(array('service_container', 'internal', 'bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'foo'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids by factory methods in the method map, followed by service ids defined by set()');
}

/**
* @group legacy
* @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered
*/
public function testGetLegacyServiceIds()
{
$sc = new LegacyProjectServiceContainer();
$sc->set('foo', $obj = new \stdClass());

$deprecations = array(
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
);

ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () use ($sc) {
$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 @@ -160,7 +178,6 @@ public function testGet()
$this->assertEquals($sc->__bar, $sc->get('bar'), '->get() returns the service for the given id');
$this->assertEquals($sc->__foo_bar, $sc->get('foo_bar'), '->get() returns the service if a get*Method() is defined');
$this->assertEquals($sc->__foo_baz, $sc->get('foo.baz'), '->get() returns the service if a get*Method() is defined');
$this->assertEquals($sc->__foo_baz, $sc->get('foo\\baz'), '->get() returns the service if a get*Method() is defined');

$sc->set('bar', $bar = new \stdClass());
$this->assertEquals($bar, $sc->get('bar'), '->get() prefers to return a service defined with set() than one defined with a getXXXMethod()');
Expand All @@ -174,6 +191,43 @@ public function testGet()
$this->assertNull($sc->get('', ContainerInterface::NULL_ON_INVALID_REFERENCE), '->get() returns null if the service is empty');
}

/**
* @group legacy
* @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered
*/
public function testLegacyGet()
{
$sc = new LegacyProjectServiceContainer();
$sc->set('foo', $foo = new \stdClass());

$deprecations = array(
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
);

ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () use ($sc, $foo) {
$this->assertEquals($foo, $sc->get('foo'), '->get() returns the service for the given id');
$this->assertEquals($foo, $sc->get('Foo'), '->get() returns the service for the given id, and converts id to lowercase');
$this->assertEquals($sc->__bar, $sc->get('bar'), '->get() returns the service for the given id');
$this->assertEquals($sc->__foo_bar, $sc->get('foo_bar'), '->get() returns the service if a get*Method() is defined');
$this->assertEquals($sc->__foo_baz, $sc->get('foo.baz'), '->get() returns the service if a get*Method() is defined');
$this->assertEquals($sc->__foo_baz, $sc->get('foo\\baz'), '->get() returns the service if a get*Method() is defined');

$sc->set('bar', $bar = new \stdClass());
$this->assertEquals($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');
} catch (\Exception $e) {
$this->assertInstanceOf('Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException', $e, '->get() throws a ServiceNotFoundException exception if the service is empty');
}
$this->assertNull($sc->get('', ContainerInterface::NULL_ON_INVALID_REFERENCE), '->get() returns null if the service is empty');
});
}

public function testGetThrowServiceNotFoundException()
{
$sc = new ProjectServiceContainer();
Expand Down Expand Up @@ -230,7 +284,32 @@ public function testHas()
$this->assertTrue($sc->has('bar'), '->has() returns true if a get*Method() is defined');
$this->assertTrue($sc->has('foo_bar'), '->has() returns true if a get*Method() is defined');
$this->assertTrue($sc->has('foo.baz'), '->has() returns true if a get*Method() is defined');
$this->assertTrue($sc->has('foo\\baz'), '->has() returns true if a get*Method() is defined');
}

/**
* @group legacy
* @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered
*/
public function testLegacyHas()
{
$sc = new LegacyProjectServiceContainer();
$sc->set('foo', new \stdClass());

$deprecations = array(
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
'Generating a dumped container without populating the method map is deprecated since 3.2 and will be unsupported in 4.0. Update your dumper to generate the method map.',
);

ErrorAssert::assertDeprecationsAreTriggered($deprecations, function () use ($sc) {
$this->assertFalse($sc->has('foo1'), '->has() returns false if the service does not exist');
$this->assertTrue($sc->has('foo'), '->has() returns true if the service exists');
$this->assertTrue($sc->has('bar'), '->has() returns true if a get*Method() is defined');
$this->assertTrue($sc->has('foo_bar'), '->has() returns true if a get*Method() is defined');
$this->assertTrue($sc->has('foo.baz'), '->has() returns true if a get*Method() is defined');
$this->assertTrue($sc->has('foo\\baz'), '->has() returns true if a get*Method() is defined');
});
}

public function testInitialized()
Expand Down Expand Up @@ -385,6 +464,72 @@ public function testRequestAnInternalSharedPrivateServiceIsDeprecated()
}

class ProjectServiceContainer extends Container
{
public $__bar;
public $__foo_bar;
public $__foo_baz;
public $__internal;
protected $methodMap = array(
'internal' => 'getInternalService',
'bar' => 'getBarService',
'foo_bar' => 'getFooBarService',
'foo.baz' => 'getFoo_BazService',
'circular' => 'getCircularService',
'throw_exception' => 'getThrowExceptionService',
'throws_exception_on_service_configuration' => 'getThrowsExceptionOnServiceConfigurationService',
);

public function __construct()
{
parent::__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;
}

protected function getFooBarService()
{
return $this->__foo_bar;
}

protected function getFoo_BazService()
{
return $this->__foo_baz;
}

protected function getCircularService()
{
return $this->get('circular');
}

protected function getThrowExceptionService()
{
throw new \Exception('Something went terribly wrong!');
}

protected function getThrowsExceptionOnServiceConfigurationService()
{
$this->services['throws_exception_on_service_configuration'] = $instance = new \stdClass();

throw new \Exception('Something was terribly wrong while trying to configure the service!');
}
}

class LegacyProjectServiceContainer extends Container
{
public $__bar;
public $__foo_bar;
Expand Down