Skip to content

[DI] Include ids from method map in known services #19690

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 2 commits into from
Closed

[DI] Include ids from method map in known services #19690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ec8725d
include service ids from method map
ro0NL Aug 21, 2016
770b77b
simplified
ro0NL Aug 23, 2016
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
16 changes: 10 additions & 6 deletions src/Symfony/Component/DependencyInjection/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,7 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER)
$this->scopedServices[$scope][$id] = $service;
}

if (isset($this->aliases[$id])) {
unset($this->aliases[$id]);
}
unset($this->aliases[$id], $this->methodMap[$id]);
Copy link
Member

Choose a reason for hiding this comment

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

👍, even if it won't have any practical effect because $this->services[$id] is checked first in the get() method

Copy link
Member

Choose a reason for hiding this comment

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

For the record, I'd be 👎 now, because that would trigger a not-required copy-on-write for $this->methodMap.


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

Expand Down Expand Up @@ -222,6 +220,7 @@ public function has($id)
for ($i = 2;;) {
if ('service_container' === $id
|| isset($this->aliases[$id])
|| isset($this->methodMap[$id])
Copy link
Member

Choose a reason for hiding this comment

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

👍

|| isset($this->services[$id])
) {
return true;
Expand Down Expand Up @@ -357,15 +356,20 @@ public function initialized($id)
*/
public function getServiceIds()
{
$ids = array();
$ids = array_keys($this->methodMap + $this->services);
$reversedMethodMap = array_change_key_case(array_flip($this->methodMap), \CASE_LOWER);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the changes on this method are legitimate: get*Service are the only "source", methodMap must be in sync with the correspond list and we don't need to merge it.

Copy link
Contributor Author

@ro0NL ro0NL Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Tests proves it works... you cant imagine how creative users can be. Sorry, couldnt resist ;-)

$this->methodMap is a compilation artifact

What if we make it a feature?

Copy link
Member

@nicolas-grekas nicolas-grekas Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

you cant imagine how creative users can

except that as you spotted, it never worked :)

What if we make it feature?

then it should go on master and should have a supporting use case

Copy link
Contributor Author

@ro0NL ro0NL Aug 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, i didnt consider PhpDumper::generateMethodName on this, which shows method names are generated from id.

However, they can get suffixed due duplicate methods, but that doesnt mean the id should change as well (this is the key in methodMap which can be perfectly unique).

I covered exactly that in tests (imagine a suffixed/generated method though).

foreach (get_class_methods($this) as $method) {
if (preg_match('/^get(.+)Service$/', $method, $match)) {
Copy link
Member

@nicolas-grekas nicolas-grekas Aug 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

So, shouldn't we remove this and rely sololy on methodMap?

the full method could be reduced to the following implementation, isn't it?:

public function getServiceIds()
{
    return array_keys($this->methodMap + $this->services);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends how this method should work before/after compilation.

We could lazily load the method map when needed, which also happens when dumping. Im assuming the methods wont change during runtime anyway :)

$ids[] = self::underscore($match[1]);
if (isset($reversedMethodMap[$methodLc = 'get'.strtolower($match[1]).'service'])) {
$ids[] = $reversedMethodMap[$methodLc];
} else {
$ids[] = self::underscore($match[1]);
}
}
}
$ids[] = 'service_container';

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

/**
Expand Down
22 changes: 19 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 @@ -123,11 +123,11 @@ public function testGetServiceIds()
$sc = new Container();
$sc->set('foo', $obj = new \stdClass());
$sc->set('bar', $obj = new \stdClass());
$this->assertEquals(array('service_container', 'foo', 'bar'), $sc->getServiceIds(), '->getServiceIds() returns all defined service ids');
$this->assertEquals(array('foo', 'bar', 'service_container'), $sc->getServiceIds(), '->getServiceIds() returns all defined service ids');

$sc = new ProjectServiceContainer();
$sc->set('foo', $obj = new \stdClass());
$this->assertEquals(array('scoped', 'scoped_foo', 'scoped_synchronized_foo', 'inactive', '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('mapped_method_id', 'mapped_method_id2', 'foo', 'scoped', 'scoped_foo', 'scoped_synchronized_foo', 'inactive', 'bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'service_container'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids resolved from methods, followed by service ids defined by set()');
}

public function testSet()
Expand Down Expand Up @@ -202,6 +202,7 @@ public function testGet()
$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');
$this->assertEquals(new \stdClass(), $sc->get('mapped_method_id2'), '->get() returns the service if a method is mapped custom');

$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 Down Expand Up @@ -234,7 +235,7 @@ public function testGetThrowServiceNotFoundException()
$this->fail('->get() throws an Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException if the key does not exist');
} catch (\Exception $e) {
$this->assertInstanceOf('Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException', $e, '->get() throws an Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException if the key does not exist');
$this->assertEquals('You have requested a non-existent service "bag". Did you mean one of these: "bar", "baz"?', $e->getMessage(), '->get() throws an Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException with some advices');
$this->assertEquals('You have requested a non-existent service "bag". Did you mean one of these: "baz", "bar"?', $e->getMessage(), '->get() throws an Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException with some advices');
}
}

Expand Down Expand Up @@ -266,6 +267,7 @@ public function testHas()
$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');
$this->assertTrue($sc->has('mapped_method_id'), '->has() returns true if a method is mapped custom');
}

public function testInitialized()
Expand Down Expand Up @@ -611,6 +613,10 @@ public function __construct()
$this->__foo_bar = new \stdClass();
$this->__foo_baz = new \stdClass();
$this->synchronized = false;
$this->methodMap = array(
'mapped_method_id' => 'get_CamelcasedMethodEndingWith_Service',
'mapped_method_id2' => '_get_by_different_convention',
);
$this->aliases = array('alias' => 'bar');
}

Expand Down Expand Up @@ -651,6 +657,16 @@ protected function getInactiveService()
throw new InactiveScopeException('request', 'request');
}

protected function get_camelcasedmethodendingwith_Service()
{
return new \stdClass();
}

protected function _get_by_different_convention()
{
return new \stdClass();
}

protected function getBarService()
{
return $this->__bar;
Expand Down