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] Avoid private call to Container::has()#22912
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
| foreach ($servicesas$service) { | ||
| $conditions[] =sprintf("\$this->has('%s')",$service); | ||
| if ($this->container->hasDefinition($service) && !$this->container->getDefinition($service)->isPublic()) { | ||
| $conditions[] =sprintf("isset(\$this->methodMap['%s'])",$service); |
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.
in the "set" method, "methodMap" is not updated on override, whereas "privates" is,
shouldn't this check for "privates" ?
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.
if the container has the definition, why adding a condition ? It will have the service.
nicolas-grekas commentedMay 25, 2017
any test case possible? |
ro0NL commentedMay 25, 2017
Updated. The private definitions are now skipped from the service conditionals, like@stof mentioned. Note this only applies to dumping an uncompiled container, ie. this fest fails as of 3.3 and probably allows for the dumper to be further simplified (invalid references are removed due compilation, so it seems). |
nicolas-grekas left a comment
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.
👍
fabpot commentedMay 25, 2017
Thank you@ro0NL. |
This PR was merged into the 3.2 branch.Discussion----------[DI] Avoid private call to Container::has()| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | yes| New feature? | no| BC breaks? | no| 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-->Fixes `User Deprecated: Checking for the existence of the "debug.file_link_formatter" private service is deprecated since Symfony 3.2 and won't be supported anymore in Symfony 4.0.`Commits-------5689281 [DI] Avoid private call to Container::has()
stof commentedMay 26, 2017
we could even skip the condition when we know we have a public service here (as we already know that |
ro0NL commentedMay 26, 2017
😓 yes. Tend to leave it as is for now; not sure if worth it. And i definitely dont want to break stuff unintentionally :)) But i believe as of 4.0 (container is compiled) we can drop the logic entirely from phpdumper due |
Fixes
User Deprecated: Checking for the existence of the "debug.file_link_formatter" private service is deprecated since Symfony 3.2 and won't be supported anymore in Symfony 4.0.