Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Dont use Container::get() when fetching private services internally#19708
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ro0NL commentedAug 22, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yes. Genious. 👍#19683 is more or less a new feature.. not sure this replaces that fully? |
| } | ||
| 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'])"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the assignment is useless. The factory method is already doing it internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cool! fixed
e282307 to83891a4Compare| } | ||
| if ($this->container->hasDefinition($id) && !$this->container->getDefinition($id)->isPublic()) { | ||
| return"\$this->services[(isset(\$this->services['$id']) ||\$this->{$this->generateMethodName($id)}()) && false ?: '$id']"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
nicolas-grekasAug 22, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
because the previous code did't work on hhvm nor php55 unfortunately (parse error) :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please add a comment explaining why such crazy syntax is used then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about a PHP version detection for generating? Or is this to much for such a simple case.
Btw. Pure Genious 😳
nicolas-grekasAug 23, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
f26bdcd toc4b34cdCompare| /** | ||
| * @deprecated since 3.2, to be removed in 4.0 | ||
| */ | ||
| protected$privates =array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 tonot bring back the original issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Lets hope for the best then. I would remove the docblock for now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should the protected$privates property be marked as@internal as well?
stof commentedAug 23, 2016
👍 |
stof commentedAug 23, 2016
@fabpot can you fix fabbot so that it stops reporting CS issues in fixtures files ? |
fabpot commentedSep 1, 2016
Thank you@nicolas-grekas. |
…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 | MITAs 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 servicesSincesymfony/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
…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 | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->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 commentedNov 19, 2018
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 So the solution is to trick PHP into declaring a variable as a side effect: Just leaving it here for reference if others wondered how and why this works. |
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.