Skip to content

[DI] Exclude private services from alternatives #19679

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

[DI] Exclude private services from alternatives #19679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76cae3d
include dynamic services in list of alternatives
ro0NL Aug 20, 2016
ff2fb12
exclude private services from alternatives
ro0NL Aug 20, 2016
01c3651
better test
ro0NL Aug 20, 2016
6bf4107
deprecate including privates in getServiceIds
ro0NL Aug 20, 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
21 changes: 16 additions & 5 deletions src/Symfony/Component/DependencyInjection/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,10 +269,10 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
}

$alternatives = array();
foreach ($this->services as $key => $associatedService) {
$lev = levenshtein($id, $key);
if ($lev <= strlen($id) / 3 || false !== strpos($key, $id)) {
$alternatives[] = $key;
foreach ($this->getServiceIds(false) as $knownId) {
Copy link
Member

Choose a reason for hiding this comment

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

this change is unrelated and should be discussed in #19685 (thus no need for the patch here)

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.

Agree. Im reverting it so we check for privates here again.

Deprecating privates from getServiceIds is a new PR i guess which cleans this up.. it needs to be taken care of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolas-grekas if #19685 and a new PR, fixing excluding privates from getServiceIds, are done.... this becomes a non-issue. Shall we close it?

Copy link
Member

Choose a reason for hiding this comment

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

I think this PR should be closed yes.

$lev = levenshtein($id, $knownId);
if ($lev <= strlen($id) / 3 || false !== strpos($knownId, $id)) {
$alternatives[] = $knownId;
}
}

Expand Down Expand Up @@ -336,11 +336,22 @@ public function reset()
*
* @return array An array of all defined service ids
*/
public function getServiceIds()
public function getServiceIds(/*$includePrivates = true*/)
{
$includePrivates = true;
if (func_num_args() === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Nobody calls this with an argument, which means nobody will see the deprecation notice.
Yet, I think we should only add a note to remind about not returning private services in 4.0. No need for any deprecations: if people use the private services returned today from this method, they will get a deprecation because fetching them is deprecated.

$includePrivates = (bool) func_get_arg(0);
if ($includePrivates) {
@trigger_error(sprintf('Including private services in the list of service id\'s is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
Copy link
Contributor Author

@ro0NL ro0NL Aug 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@ogizanagi something like this?

edit: it makes no real sense though.. we can just deprecate privates in getServiceIds on master, right? ie. new/update PR. as it would fix this issue automatically.

Copy link
Contributor

@ogizanagi ogizanagi Aug 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I don't understand your concerns 😕 ?

You can't just deprecate privates in getServiceIds if you have no idea the privates were asked. And simply not returning them will be a BC break. Thus, the new argument to explicit the intention, and trigger the deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. master != 4.x this keeps confusing me 😅

}
}
$ids = array();
foreach (get_class_methods($this) as $method) {
if (preg_match('/^get(.+)Service$/', $method, $match)) {
$id = self::underscore($match[1]);
if (!$includePrivates && isset($this->privates[$id])) {
continue;
}
$ids[] = self::underscore($match[1]);
}
}
Expand Down
17 changes: 16 additions & 1 deletion src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ public function testGetThrowServiceNotFoundException()
{
$sc = new ProjectServiceContainer();
$sc->set('foo', $foo = new \stdClass());
$sc->set('bar', $foo = new \stdClass());
$sc->set('baz', $foo = new \stdClass());

try {
Expand All @@ -198,6 +197,22 @@ public function testGetThrowServiceNotFoundException()
}
}

/**
* @expectedException \Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException
* @expectedExceptionMessage You have requested a non-existent service "int". Did you mean this: "internal_lookalike"?
*/
public function testGetThrowServiceNotFoundExceptionWithCorrectAlternatives()
{
$sc = new ProjectServiceContainer();
$sc->set('internal_lookalike', $foo = new \stdClass());
$reflectionProperty = new \ReflectionProperty(ProjectServiceContainer::class, 'privates');
$reflectionProperty->setAccessible(true);

$this->assertArrayHasKey('internal', $reflectionProperty->getValue($sc));
$this->assertFalse($sc->has('int'));
$sc->get('int');
}

public function testGetCircularReference()
{
$sc = new ProjectServiceContainer();
Expand Down