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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DI] Avoid private call to Container::has() #22912

Merged
merged 1 commit into from May 25, 2017

Conversation

Projects
None yet
5 participants
@ro0NL
Contributor

ro0NL commented May 25, 2017

Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

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.

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@@ -1191,7 +1191,11 @@ private function wrapServiceConditionals($value, $code)
$conditions = array();
foreach ($services as $service) {
$conditions[] = sprintf("\$this->has('%s')", $service);
if ($this->container->hasDefinition($service) && !$this->container->getDefinition($service)->isPublic()) {
$conditions[] = sprintf("isset(\$this->methodMap['%s'])", $service);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 25, 2017

Member

in the "set" method, "methodMap" is not updated on override, whereas "privates" is,
shouldn't this check for "privates" ?

@nicolas-grekas

nicolas-grekas May 25, 2017

Member

in the "set" method, "methodMap" is not updated on override, whereas "privates" is,
shouldn't this check for "privates" ?

This comment has been minimized.

@stof

stof May 25, 2017

Member

if the container has the definition, why adding a condition ? It will have the service.

@stof

stof May 25, 2017

Member

if the container has the definition, why adding a condition ? It will have the service.

@nicolas-grekas nicolas-grekas added this to the 3.2 milestone May 25, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 25, 2017

Member

any test case possible?

Member

nicolas-grekas commented May 25, 2017

any test case possible?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 25, 2017

Contributor

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).

Contributor

ro0NL commented May 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

👍

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot May 25, 2017

Member

Thank you @ro0NL.

Member

fabpot commented May 25, 2017

Thank you @ro0NL.

@fabpot fabpot merged commit 5689281 into symfony:3.2 May 25, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 25, 2017

bug #22912 [DI] Avoid private call to Container::has() (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()

@ro0NL ro0NL deleted the ro0NL:di/private-has branch May 26, 2017

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 26, 2017

Member

we could even skip the condition when we know we have a public service here (as we already know that has will return true)

Member

stof commented May 26, 2017

we could even skip the condition when we know we have a public service here (as we already know that has will return true)

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 26, 2017

Contributor

😓 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 ResolveInvalidReferencesPass. Not sure though..

Contributor

ro0NL commented May 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 ResolveInvalidReferencesPass. Not sure though..

This was referenced May 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment