[DI] Dont use Container::get() when fetching private services internally #19708

nicolas-grekas · 2016-08-22T17:09:08Z

Q	A
Branch?	master
Bug fix?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#19683, #19682, #19680
License	MIT

As spotted by @wouterj, we forgot to remove the deprecation notice when doing internal calls to get private services.

Yet, we don't need to get through this get() method, because we can already resolve many things at compile time for private services. This will provide another small performance optim, and fix the issue.

ro0NL · 2016-08-22T17:16:08Z

Yes. Genious. 👍 #19683 is more or less a new feature.. not sure this replaces that fully?

stof · 2016-08-22T17:17:26Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -1376,6 +1376,9 @@ private function getServiceCall($id, Reference $reference = null)
            return '$this';
        }

+        if ($this->container->hasDefinition($id) && !$this->container->getDefinition($id)->isPublic()) {
+            return "(!isset(\$this->services['$id']) ? \$this->services['$id'] = \$this->{$this->generateMethodName($id)}() : \$this->services['$id'])";


the assignment is useless. The factory method is already doing it internally.

cool! fixed

stof · 2016-08-22T17:36:12Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -1376,6 +1376,9 @@ private function getServiceCall($id, Reference $reference = null)
            return '$this';
        }

+        if ($this->container->hasDefinition($id) && !$this->container->getDefinition($id)->isPublic()) {
+            return "\$this->services[(isset(\$this->services['$id']) || \$this->{$this->generateMethodName($id)}()) && false ?: '$id']";


this code looks very hard to understand ? Why not keeping your previous proposal but simply removing the assignment ? the factory method handles the assignment (to share the service) but also returns it.

because the previous code did't work on hhvm nor php55 unfortunately (parse error) :(

btw, in case of a non-shared private service, your new code does not work anymore (and your previous code was also broken, as the assignment was sharing the service instance by mistake).

the code should just be $this->services['foobar'] ?? $this->getFoobarService(); (rewritten to support PHP 5)

Good catch, fixed. You'll scream when looking at the generated code, yet it works well and I didn't find any better syntax that worked on 5.5 :)

Please add a comment explaining why such crazy syntax is used then

What about a PHP version detection for generating? Or is this to much for such a simple case.

Btw. Pure Genious 😳

@stof comment added
@sstok not possible: the generated code must be version independent so that people can e.g. warump on 7 & deploy on 5 (even if not recommended at all)

@nicolas-grekas as soon as you use Twig, you cannot do such things for the cache warmup anyway. The Twig compiled templates are version-dependant.

ro0NL · 2016-08-22T21:47:49Z

src/Symfony/Component/DependencyInjection/Container.php

+    /**
+     * @deprecated since 3.2, to be removed in 4.0
+     */
+    protected $privates = array();


Note that, currently, removing this in 4.x brings back the original issue. IMO we should cover 4.x behavior if we deprecate this (ie. why i went with randomzing, it makes $this->privates truly a bridge till 4.x).

What the annotation says is that one should not rely on this property since it will be removed in 4.0. Of course, when doing so, we will also make the required changes to not bring back the original issue.

Lets hope for the best then. I would remove the docblock for now though.

Should the protected $privates property be marked as @internal as well?

stof · 2016-08-23T07:28:48Z

👍

stof · 2016-08-23T07:29:14Z

@fabpot can you fix fabbot so that it stops reporting CS issues in fixtures files ?

fabpot · 2016-09-01T17:39:54Z

Thank you @nicolas-grekas.

@wouterj

…ces internally (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [DI] Dont use Container::get() when fetching private services internally | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19683, #19682, #19680 | License | MIT As spotted by @wouterj, we forgot to remove the deprecation notice when doing internal calls to get private services. Yet, we don't need to get through this `get()` method, because we can already resolve many things at compile time for private services. This will provide another small performance optim, and fix the issue. Commits ------- a9c79fb [DI] Dont use Container::get() when fetching private services internally

…vate services (lemoinem) This PR was merged into the master branch. Discussion ---------- [ServiceContainer] Remove implementation details of private services Since symfony/symfony#19708, getting private services through `Container::get()` is deprecated in the cases where it worked up to now, and this will removed in 4.0. The ways a container optimizes private services instanciation are not useful information anymore and is confusing to some (see #6959). I removed the mentions of these implementations details to make the documentation clearer. Commits ------- a529157 [ServiceContainer] Remove any references to the implementation details of private services

@nicolas-grekas

…rvices (ro0NL) This PR was merged into the 4.0-dev branch. Discussion ---------- [DI] Removed deprecated setting private/pre-defined services | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  See #21533, #19708 and #19146 @nicolas-grekas privates should be excluded from `getServiceIds` right? i also wondered.. did we forget to deprecate checking privates with `initialized()`? Commits ------- 9f96952 [DI] Removed deprecated setting private/pre-defined services

sagikazarmark · 2018-11-19T12:36:08Z

A colleague approached me with the solution used in this PR which took some time to understand. It's quite a genius one actually, not sure I would ever think about this as a solution!

Here are the obvious solutions one would consider, but they don't work on PHP 5.x:

https://3v4l.org/rspdo
https://3v4l.org/SFl8a

So the solution is to trick PHP into declaring a variable as a side effect:

https://3v4l.org/YXXQm

Just leaving it here for reference if others wondered how and why this works.

carsonbot added Status: Needs Review Bug labels Aug 22, 2016

This was referenced Aug 22, 2016

[DI] Deprecate ContainerBuilder::set #19644

Closed

[DependencyInjection] Do not trigger deprecation when injecting private services #19682

Closed

stof reviewed Aug 22, 2016
View reviewed changes

nicolas-grekas force-pushed the fix-private branch 2 times, most recently from e282307 to 83891a4 Compare August 22, 2016 17:30

stof reviewed Aug 22, 2016
View reviewed changes

nicolas-grekas mentioned this pull request Aug 22, 2016

[DI] Randomize private services #19683

Closed

nicolas-grekas force-pushed the fix-private branch 2 times, most recently from f26bdcd to c4b34cd Compare August 22, 2016 20:31

ro0NL reviewed Aug 22, 2016
View reviewed changes

nicolas-grekas force-pushed the fix-private branch from c4b34cd to d1d8d3c Compare August 23, 2016 07:42

nicolas-grekas added Status: Reviewed Ready and removed Status: Needs Review labels Aug 23, 2016

nicolas-grekas force-pushed the fix-private branch from d1d8d3c to 01bd03b Compare August 24, 2016 17:11

[DI] Dont use Container::get() when fetching private services internally

a9c79fb

nicolas-grekas force-pushed the fix-private branch from 01bd03b to a9c79fb Compare August 26, 2016 15:25

fabpot merged commit a9c79fb into symfony:master Sep 1, 2016

nicolas-grekas deleted the fix-private branch September 1, 2016 19:10

sstok mentioned this pull request Sep 29, 2016

Public service, very vague symfony/symfony-docs#6959

Closed

lemoinem mentioned this pull request Oct 1, 2016

[ServiceContainer] Remove implementation details of private services symfony/symfony-docs#7021

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

ro0NL mentioned this pull request May 20, 2017

[DI] Removed deprecated setting private/pre-defined services #22801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Dont use Container::get() when fetching private services internally #19708

[DI] Dont use Container::get() when fetching private services internally #19708

nicolas-grekas commented Aug 22, 2016

ro0NL commented Aug 22, 2016 •

edited

Loading

stof Aug 22, 2016

nicolas-grekas Aug 22, 2016

stof Aug 22, 2016

nicolas-grekas Aug 22, 2016 •

edited

Loading

stof Aug 22, 2016

nicolas-grekas Aug 22, 2016

stof Aug 23, 2016

sstok Aug 23, 2016 •

edited

Loading

nicolas-grekas Aug 23, 2016 •

edited

Loading

stof Aug 23, 2016

ro0NL Aug 22, 2016

nicolas-grekas Aug 23, 2016

ro0NL Aug 23, 2016 •

edited

Loading

hhamon Aug 23, 2016 •

edited

Loading

stof commented Aug 23, 2016

stof commented Aug 23, 2016

fabpot commented Sep 1, 2016

sagikazarmark commented Nov 19, 2018

[DI] Dont use Container::get() when fetching private services internally #19708

[DI] Dont use Container::get() when fetching private services internally #19708

Conversation

nicolas-grekas commented Aug 22, 2016

ro0NL commented Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstok Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

hhamon Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

stof commented Aug 23, 2016

stof commented Aug 23, 2016

fabpot commented Sep 1, 2016

sagikazarmark commented Nov 19, 2018

ro0NL commented Aug 22, 2016 •

edited

Loading

nicolas-grekas Aug 22, 2016 •

edited

Loading

sstok Aug 23, 2016 •

edited

Loading

nicolas-grekas Aug 23, 2016 •

edited

Loading

ro0NL Aug 23, 2016 •

edited

Loading

hhamon Aug 23, 2016 •

edited

Loading