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] Check for privates before shared services #22866

Merged
merged 1 commit into from May 24, 2017

Conversation

Projects
None yet
4 participants
@ro0NL
Contributor

ro0NL commented May 23, 2017

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

cc @stof

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 23, 2017

Member

CI is red :)

Member

nicolas-grekas commented May 23, 2017

CI is red :)

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

@stof

This comment has been minimized.

Show comment
Hide comment
@stof
Member

stof commented May 23, 2017

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

Kinda clueless here... trying to fix WebProfilerExtensionTest::testToolbarConfig

patched assertSaneContainer so it skips private id's (hardcoded locally to test :)

however these 2 assertions pass for all provided debug modes, indiviudally;

        $this->assertSaneContainer($this->getDumpedContainer(), '', array('web_profiler.csp.handler'));

        if ($listenerInjected) {
            $this->assertSame($listenerEnabled, $this->container->get('web_profiler.debug_toolbar')->isEnabled());
        }

the combination seems to trigger a deprecation 😕

Contributor

ro0NL commented May 23, 2017

Kinda clueless here... trying to fix WebProfilerExtensionTest::testToolbarConfig

patched assertSaneContainer so it skips private id's (hardcoded locally to test :)

however these 2 assertions pass for all provided debug modes, indiviudally;

        $this->assertSaneContainer($this->getDumpedContainer(), '', array('web_profiler.csp.handler'));

        if ($listenerInjected) {
            $this->assertSame($listenerEnabled, $this->container->get('web_profiler.debug_toolbar')->isEnabled());
        }

the combination seems to trigger a deprecation 😕

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

first patch.. lets see what happens :)

Contributor

ro0NL commented May 23, 2017

first patch.. lets see what happens :)

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

testStopwatchExtensionAvailability actually gets a private service 😠

$container->get('twig.extension.debug.stopwatch')->getTokenParsers()

is there a clean way to silence expected deprecations.. or should we do something else here.

@stof see deprecations at appveyor ;)

Contributor

ro0NL commented May 23, 2017

testStopwatchExtensionAvailability actually gets a private service 😠

$container->get('twig.extension.debug.stopwatch')->getTokenParsers()

is there a clean way to silence expected deprecations.. or should we do something else here.

@stof see deprecations at appveyor ;)

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 23, 2017

Member

well, the test should either not access a private service, or be marked as legacy. If the test is not meant to be removed in 4.0, marking it as legacy is not an option, and the test should be rewritten instead.
This is good that I catched the mistake in the deprecation trigger in the other PR, as this allowed us to catch mistakes in our own testsuite which was using deprecated APIs without getting the notice...

Member

stof commented May 23, 2017

well, the test should either not access a private service, or be marked as legacy. If the test is not meant to be removed in 4.0, marking it as legacy is not an option, and the test should be rewritten instead.
This is good that I catched the mistake in the deprecation trigger in the other PR, as this allowed us to catch mistakes in our own testsuite which was using deprecated APIs without getting the notice...

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

Updated.. this should do. But i dont like it really :) but i guess we have no choice as we never decided how privates reflects a compiled containerbuilder.

@stof to clarify.. the root issue is ContainerBuilder::get() calling parent::get

Contributor

ro0NL commented May 23, 2017

Updated.. this should do. But i dont like it really :) but i guess we have no choice as we never decided how privates reflects a compiled containerbuilder.

@stof to clarify.. the root issue is ContainerBuilder::get() calling parent::get

Show outdated Hide outdated ...ymfony/Bundle/TwigBundle/Tests/DependencyInjection/TwigExtensionTest.php
$this->compileContainer($container);
$tokenParsers = $container->get('twig.extension.debug.stopwatch')->getTokenParsers();
$tokenParsers = $container->get('test_debug_stopwatch')->stopwatch->getTokenParsers();

This comment has been minimized.

@stof

stof May 23, 2017

Member

a better solution would be to use $container->get('twig')->getExtension(StopwatchExtension::class) to access the extension, which avoids registering an extra service

@stof

stof May 23, 2017

Member

a better solution would be to use $container->get('twig')->getExtension(StopwatchExtension::class) to access the extension, which avoids registering an extra service

This comment has been minimized.

@ro0NL

ro0NL May 23, 2017

Contributor

fixed

@ro0NL

ro0NL May 23, 2017

Contributor

fixed

This comment has been minimized.

@ro0NL

ro0NL May 23, 2017

Contributor

seems to break stuff :/

1) Symfony\Bundle\TwigBundle\Tests\DependencyInjection\TwigExtensionTest::testStopwatchExtensionAvailability with data set "debug-and-stopwatch-enabled" (true, true, true)
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "twig.loader".
@ro0NL

ro0NL May 23, 2017

Contributor

seems to break stuff :/

1) Symfony\Bundle\TwigBundle\Tests\DependencyInjection\TwigExtensionTest::testStopwatchExtensionAvailability with data set "debug-and-stopwatch-enabled" (true, true, true)
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "twig.loader".

This comment has been minimized.

@ro0NL

ro0NL May 23, 2017

Contributor

reverted the whole change, as with privates being empty it keeps working :)

this test should fail whenever we decide to change behavior.

@ro0NL

ro0NL May 23, 2017

Contributor

reverted the whole change, as with privates being empty it keeps working :)

this test should fail whenever we decide to change behavior.

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Container.php
@@ -262,6 +262,10 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
if (isset($this->aliases[$id])) {
$id = $this->aliases[$id];
}
if (isset($this->privates[$id]) && !$this instanceof ContainerBuilder) {

This comment has been minimized.

@stof

stof May 23, 2017

Member

why the check on ContainerBuilder ? this is useless ($this->privates will be empty for them anyway)

@stof

stof May 23, 2017

Member

why the check on ContainerBuilder ? this is useless ($this->privates will be empty for them anyway)

This comment has been minimized.

This comment has been minimized.

@ro0NL

ro0NL May 23, 2017

Contributor

i think ContainerBuilder needs to behave the same as Container, after compilation. But that could be a separate PR i guess. The changelog doesnt mention ContainerBuilder really...

@ro0NL

ro0NL May 23, 2017

Contributor

i think ContainerBuilder needs to behave the same as Container, after compilation. But that could be a separate PR i guess. The changelog doesnt mention ContainerBuilder really...

This comment has been minimized.

@stof

stof May 23, 2017

Member

well, once compiled, the array is not empty anymore, but we do want the deprecation in this case. So please remove the extra check you added there (which was not there before moving the notice btw, so you are the one adding a difference in the behavior)

@stof

stof May 23, 2017

Member

well, once compiled, the array is not empty anymore, but we do want the deprecation in this case. So please remove the extra check you added there (which was not there before moving the notice btw, so you are the one adding a difference in the behavior)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 23, 2017

Member

I think we should move this to 3.3: it will trigger more notices in 3.2 apps and break some strict CI for no real benefit.

Member

nicolas-grekas commented May 23, 2017

I think we should move this to 3.3: it will trigger more notices in 3.2 apps and break some strict CI for no real benefit.

@nicolas-grekas nicolas-grekas modified the milestones: 3.3, 3.2 May 23, 2017

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

3.2 is truly buggy though.. (@stof's comment in previous PR) i think the ContainerBuilder check will prevent those unexpected notices actually; while fixing Container.

otherwise, could you just commit the changelog against 3.2 :)) but you probably prefer a separate PR for that :P

Contributor

ro0NL commented May 23, 2017

3.2 is truly buggy though.. (@stof's comment in previous PR) i think the ContainerBuilder check will prevent those unexpected notices actually; while fixing Container.

otherwise, could you just commit the changelog against 3.2 :)) but you probably prefer a separate PR for that :P

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 23, 2017

Member

@ro0NL the ContainerBuilder check does not solve anything in 3.2.
The issue was that we were missing deprecations when getting a private service which was already instantiated previously (due to being a dependency of another service you got earlier). This happens both for a ContainerBuilder and the dumped container.
Your check fixes the testsuite only because the testsuite does not do any such bad get calls on a dumped container (as most tests don't bother about dumping the container and then loading the dumped container to run assertions)

Member

stof commented May 23, 2017

@ro0NL the ContainerBuilder check does not solve anything in 3.2.
The issue was that we were missing deprecations when getting a private service which was already instantiated previously (due to being a dependency of another service you got earlier). This happens both for a ContainerBuilder and the dumped container.
Your check fixes the testsuite only because the testsuite does not do any such bad get calls on a dumped container (as most tests don't bother about dumping the container and then loading the dumped container to run assertions)

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

@stof im still a bit confused.. (ive reverted the containerbuilder checks).

moving the private check up (above checking for shared services), truly fixes your comments from #22801, i.e. https://github.com/symfony/symfony/pull/22866/files#diff-8aeba2b88d0347062017ee160a6beb49R408 fails otherwise.

imo. it needs a fix for 3.2 (properly deprecating private services), but im missing something regarding the container builder. AFAIK the issue is ContainerBuilder::get() => parent::get().

Updating this call to get() still triggers the deprecation for private service twig.extension.debug.stopwatch. Im curious whats the root issue here, or at least what the proper fix is for either 3.2 or 3.3.

Contributor

ro0NL commented May 23, 2017

@stof im still a bit confused.. (ive reverted the containerbuilder checks).

moving the private check up (above checking for shared services), truly fixes your comments from #22801, i.e. https://github.com/symfony/symfony/pull/22866/files#diff-8aeba2b88d0347062017ee160a6beb49R408 fails otherwise.

imo. it needs a fix for 3.2 (properly deprecating private services), but im missing something regarding the container builder. AFAIK the issue is ContainerBuilder::get() => parent::get().

Updating this call to get() still triggers the deprecation for private service twig.extension.debug.stopwatch. Im curious whats the root issue here, or at least what the proper fix is for either 3.2 or 3.3.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

Think this approach https://github.com/symfony/symfony/blob/v3.2.8/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L1488 needs to be done in container builder after compilation... ie. bypasssing get() internally for privates.

Contributor

ro0NL commented May 23, 2017

Think this approach https://github.com/symfony/symfony/blob/v3.2.8/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L1488 needs to be done in container builder after compilation... ie. bypasssing get() internally for privates.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 23, 2017

Member

My 2 cts: ContainerBuilder should not care about private or not. Thus, calling parent::get() shouldn't matter because $this->privates should not be populated.
I don't know where things fail right now but if we stick to this policy and tests fail, then I'd say these tests are broken, nothing else.

Member

nicolas-grekas commented May 23, 2017

My 2 cts: ContainerBuilder should not care about private or not. Thus, calling parent::get() shouldn't matter because $this->privates should not be populated.
I don't know where things fail right now but if we stick to this policy and tests fail, then I'd say these tests are broken, nothing else.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 23, 2017

Member

For me,

should be removed, that's the bug.
ContainerBuilder is a bootstrap-stage thing, and private/public is a "compiled-container" only thing.

Member

nicolas-grekas commented May 23, 2017

For me,

should be removed, that's the bug.
ContainerBuilder is a bootstrap-stage thing, and private/public is a "compiled-container" only thing.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

Agree. I figured it out ;) i tend to qualify it a bug though.. as my perception is compiled containerbuilder === compiled container === dumped compiled container, but for now we're good.

would be nice if ContainerBuilder::compile() returns a new (prepped) Container, that would allow to even further simplify (killing isCompiled and such), but that's quite a step though.

Squahsed&rebased. Ready for review :)

Sorry for the inconvenience, but this should do.

Contributor

ro0NL commented May 23, 2017

Agree. I figured it out ;) i tend to qualify it a bug though.. as my perception is compiled containerbuilder === compiled container === dumped compiled container, but for now we're good.

would be nice if ContainerBuilder::compile() returns a new (prepped) Container, that would allow to even further simplify (killing isCompiled and such), but that's quite a step though.

Squahsed&rebased. Ready for review :)

Sorry for the inconvenience, but this should do.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 23, 2017

Member

@nicolas-grekas currently, you can keep using a ContainerBuilder after compilation (which is what some projects do as they don't dump the container to the cache, for instance Behat)

Member

stof commented May 23, 2017

@nicolas-grekas currently, you can keep using a ContainerBuilder after compilation (which is what some projects do as they don't dump the container to the cache, for instance Behat)

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 23, 2017

Contributor

may i recall #20643 :) as containerbuilder is open for anything keeping privates empty is in line with that, and so a bugfix.

but the behavior is still wrong, however currently it's half right, half wrong.

Contributor

ro0NL commented May 23, 2017

may i recall #20643 :) as containerbuilder is open for anything keeping privates empty is in line with that, and so a bugfix.

but the behavior is still wrong, however currently it's half right, half wrong.

@nicolas-grekas

👍

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof May 24, 2017

Member

We discussed this with @nicolas-grekas and here is the summary of changes we would want to get:

  • add a hidden argument in getServiceIds to allow getting only public ids (i.e. opting in for the new behavior), and trigger a deprecation notice when getting the list with private ids in it (we have some existing code adding similar hidden arguments)
  • retarget the PR to 3.3 (due to the above previous deprecation)
  • try to keep the existing code filling the privates array in the ContainerBuilder, and debug test failures instead. As the 3.4 branch will allow using 4.0 deps, accessing private services in tests will be forbidden anyway. I suspect several failures are related to getServiceIds and so could be solved by the first point.

@ro0NL can you work on it ? Otherwise, I can take this PR over this evening to finish it

Member

stof commented May 24, 2017

We discussed this with @nicolas-grekas and here is the summary of changes we would want to get:

  • add a hidden argument in getServiceIds to allow getting only public ids (i.e. opting in for the new behavior), and trigger a deprecation notice when getting the list with private ids in it (we have some existing code adding similar hidden arguments)
  • retarget the PR to 3.3 (due to the above previous deprecation)
  • try to keep the existing code filling the privates array in the ContainerBuilder, and debug test failures instead. As the 3.4 branch will allow using 4.0 deps, accessing private services in tests will be forbidden anyway. I suspect several failures are related to getServiceIds and so could be solved by the first point.

@ro0NL can you work on it ? Otherwise, I can take this PR over this evening to finish it

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 24, 2017

Contributor

@stof i will finish it tonight :) so no changes for 3.2 right?

try to keep the existing code filling the privates array in the ContainerBuilder

so we dont allow fetching privates using containerbuilder right (after compiling). getServiceIds will be the least problem :) it needs to bypass get() for references etc. internally. ie. what the phpdumper does.

Contributor

ro0NL commented May 24, 2017

@stof i will finish it tonight :) so no changes for 3.2 right?

try to keep the existing code filling the privates array in the ContainerBuilder

so we dont allow fetching privates using containerbuilder right (after compiling). getServiceIds will be the least problem :) it needs to bypass get() for references etc. internally. ie. what the phpdumper does.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL May 24, 2017

Contributor

@stof Allow edits from maintainers is enabled.. so feel free to add commits though :)

Contributor

ro0NL commented May 24, 2017

@stof Allow edits from maintainers is enabled.. so feel free to add commits though :)

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot May 24, 2017

Member

Thank you @ro0NL.

Member

fabpot commented May 24, 2017

Thank you @ro0NL.

@fabpot fabpot merged commit 4f683a9 into symfony:3.2 May 24, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

bug #22866 [DI] Check for privates before shared services (ro0NL)
This PR was merged into the 3.2 branch.

Discussion
----------

[DI] Check for privates before shared services

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22801 (comment), #22801 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

cc @stof

Commits
-------

4f683a9 [DI] Check for privates before shared services
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas May 24, 2017

Member

3.2 now merged into 3.3 so that next steps can happen on 3.3

Member

nicolas-grekas commented May 24, 2017

3.2 now merged into 3.3 so that next steps can happen on 3.3

@ro0NL ro0NL deleted the ro0NL:di/privates-3.2 branch May 24, 2017

This was referenced May 29, 2017

ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018

bug #22866 [DI] Check for privates before shared services (ro0NL)
This PR was merged into the 3.2 branch.

Discussion
----------

[DI] Check for privates before shared services

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#22801 (comment), symfony#22801 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

cc @stof

Commits
-------

4f683a9 [DI] Check for privates before shared services
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment