[DI] Deprecate (un)setting pre-defined services #21533

nicolas-grekas · 2017-02-04T19:26:15Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	yes
Tests pass?	no
Fixed tickets	#19192
License	MIT
Doc PR	-

This PR is the subset of #19668 that fixes #19192: it deprecates (un)setting pre-defined services.
This opens the path to some optimizations in the dumped container in 4.0.

nicolas-grekas · 2017-02-04T19:28:48Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -190,7 +190,14 @@ public function set($id, $service)
                @trigger_error(sprintf('Unsetting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
                unset($this->privates[$id]);
            } else {
-                @trigger_error(sprintf('Setting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. A new public service will be created instead.', $id), E_USER_DEPRECATED);
+                @trigger_error(sprintf('Setting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);


Removing this tail:

A new public service will be created instead.

it's not obvious to me that this will be the behavior we should implement in 4.0 :)

ro0NL · 2017-02-05T10:09:35Z

Shouldnt we deprecate the null scenario first? So other conditional deprecations are practically not needed.

nicolas-grekas · 2017-02-05T10:27:41Z

That's the point of this PR: #19668 is deprecating much more than technically required to enhance the dumped containers in 4.0. In fact, even if I listen to the "best practice" arguments in #19668, we know that forbidding altogether not-best-practices can break legitimate edge-case use cases (remember the deprecation of "get" in non dumped containers that we reverted). Thus, I'm not sure at all we should merge #19668 - but I'm sure we should merge this one, because it will make SF4 better at the technical level.

stof · 2017-02-07T10:14:20Z

👍

fabpot · 2017-02-12T11:53:02Z

Thank you @nicolas-grekas.

This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Deprecate (un)setting pre-defined services | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | no | Fixed tickets | #19192 | License | MIT | Doc PR | - This PR is the subset of #19668 that fixes #19192: it deprecates (un)setting pre-defined services. This opens the path to some optimizations in the dumped container in 4.0. Commits ------- fdb2140 [DI] Deprecate (un)setting pre-defined services

xkobal · 2017-05-17T12:37:14Z

Hi @nicolas-grekas

I am testing SF 3.3BETA1 and I've got a trouble with that Pull Request:

What about mock of service in functional test ?

When I try to replace Service with $kernel->getContainer()->set() I have a deprecation notice:

Setting the "SERVICE" pre-defined service is deprecated since Symfony 3.3 and won't be supported anymore in Symfony 4.0

What is my solution for this kind of functional tests ?

Thanks for the answer.

chalasr · 2017-05-17T14:06:55Z

By loading dedicated config files in your tests? See https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Tests/Functional/CachePoolClearCommandTest.php https://github.com/symfony/symfony/tree/master/src/Symfony/Bundle/FrameworkBundle/Tests/Functional/app/CachePoolClear for inspiration

stof · 2017-05-17T14:12:59Z

@xkobal injecting a PHPUnit mock to replace a service instance is a very fragile setup: if any other services depending on this service got instantiated already before your set call, it will keep using the other object rather than the mock. Replacing service instances at runtime cannot work reliably (which is exactly why we deprecated it).
Loading specific config for functional tests is indeed a much more reliable alternative. It does not allow to use mocks generated by PHPUnit though, or at least not without much pain. But it can be used by injecting hand-crafted mocks (i.e. custom implementations of your interface)

OwlyCode · 2017-05-17T14:16:15Z

The config approach is indeed much stronger. I also used to mock services with the set() method, heavily inspired by http://blog.lyrixx.info/2013/04/12/symfony2-how-to-mock-services-during-functional-tests.html (at the boot before any service gets used by the test).

xkobal · 2017-05-17T14:16:54Z

I'm agree with you on that point, and I'll change my tests, but, thats a way to test services, very very used on a lot of project ! It'll be a painful point for a lot of projects to upgrade to SF 3.3

Thanks for your answers.

nicolas-grekas · 2017-05-17T14:18:36Z

Note that you don't need to fix the deprecations when moving to 3.3.
But you will when moving to 4.0.

xkobal · 2017-05-17T14:20:13Z

I never upgrade a project letting some deprecations, our CI is failing on phpunit tests when there are some deprecations, but that's our choice, others can ignore them.

stof · 2017-05-17T14:32:01Z

Well, you can mark these tests as @legacy to avoid making them fail temporarily, to give you time to migrate the tests, if that takes time. This is the whole point of deprecations: you can migrate progressively (I also prefer removing deprecations as fast as possible, but for some of them, it may require more time)

@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

scott-r-lindsey · 2017-08-02T20:38:52Z

This is going to be difficult for my use case -- I replace guzzle clients with guzzle mock handlers loaded with responses (per test), and I validate both the response and the request made.

@chalasr "use config files" assumes that a service can be reconfigured (or replaced) by way of configuration alone, and also that tests can all use the same configuration.

@stof We're talking about testing here -- tests are supposed to be fragile.

This isn't looking good. Am I going to have to forgo booting a kernel and just build my services by hand before testing them? Or, perhaps, I create intermediary objects that can know how to replace themselves on demand? Either way, I have to take over work that I could previously count on my framework to do.

nicolas-grekas · 2017-08-02T20:41:47Z

Please open an issue to discuss this.

scott-r-lindsey · 2017-08-02T21:15:14Z

I'm adding a comment to #23311.

chalasr · 2017-08-02T21:16:54Z

You should open a new issue really. Comments on closed tickets aren't tracked.

scott-r-lindsey · 2017-08-03T00:32:20Z

Thanks @chalasr I've opened #23772

nicolas-grekas added this to the 3.3 milestone Feb 4, 2017

carsonbot added Status: Needs Review DependencyInjection Deprecation labels Feb 4, 2017

nicolas-grekas commented Feb 4, 2017

View reviewed changes

nicolas-grekas force-pushed the di-deprec-set branch 2 times, most recently from bcb4b45 to 90eb386 Compare February 4, 2017 19:32

[DI] Deprecate (un)setting pre-defined services

fdb2140

nicolas-grekas force-pushed the di-deprec-set branch from 90eb386 to fdb2140 Compare February 5, 2017 10:30

nicolas-grekas changed the title ~~[DI] Deprecate Container::set for nulls, initialized and alias services~~ [DI] Deprecate (un)setting pre-defined services Feb 5, 2017

This was referenced Feb 7, 2017

[FrameworkBundle] Wire ArrayCache for annotation reader at bootstrap #21556

Merged

[DI] Deprecate Container::set for nulls, initialized and alias services #19668

Closed

nicolas-grekas added the Ready label Feb 11, 2017

fabpot merged commit fdb2140 into symfony:master Feb 12, 2017

javiereguiluz mentioned this pull request Feb 12, 2017

(Un)setting pre-defined services is now deprecated symfony/symfony-docs#7464

Closed

nicolas-grekas deleted the di-deprec-set branch February 16, 2017 22:45

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

ro0NL mentioned this pull request May 20, 2017

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

Merged

chalasr mentioned this pull request Jul 21, 2017

Overriding services for functional testing symfony/symfony-docs#8203

Closed

scott-r-lindsey mentioned this pull request Aug 2, 2017

Injecting dependencies and using Phpunit mocks after 3.2 deprecation #23311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Deprecate (un)setting pre-defined services #21533

[DI] Deprecate (un)setting pre-defined services #21533

nicolas-grekas commented Feb 4, 2017 •

edited

Loading

nicolas-grekas Feb 4, 2017

ro0NL commented Feb 5, 2017

nicolas-grekas commented Feb 5, 2017 •

edited

Loading

stof commented Feb 7, 2017

fabpot commented Feb 12, 2017

xkobal commented May 17, 2017

chalasr commented May 17, 2017

stof commented May 17, 2017

OwlyCode commented May 17, 2017

xkobal commented May 17, 2017 •

edited

Loading

nicolas-grekas commented May 17, 2017

xkobal commented May 17, 2017 •

edited

Loading

stof commented May 17, 2017

scott-r-lindsey commented Aug 2, 2017

nicolas-grekas commented Aug 2, 2017

scott-r-lindsey commented Aug 2, 2017

chalasr commented Aug 2, 2017

scott-r-lindsey commented Aug 3, 2017

[DI] Deprecate (un)setting pre-defined services #21533

[DI] Deprecate (un)setting pre-defined services #21533

Conversation

nicolas-grekas commented Feb 4, 2017 • edited Loading

nicolas-grekas Feb 4, 2017

Choose a reason for hiding this comment

ro0NL commented Feb 5, 2017

nicolas-grekas commented Feb 5, 2017 • edited Loading

stof commented Feb 7, 2017

fabpot commented Feb 12, 2017

xkobal commented May 17, 2017

chalasr commented May 17, 2017

stof commented May 17, 2017

OwlyCode commented May 17, 2017

xkobal commented May 17, 2017 • edited Loading

nicolas-grekas commented May 17, 2017

xkobal commented May 17, 2017 • edited Loading

stof commented May 17, 2017

scott-r-lindsey commented Aug 2, 2017

nicolas-grekas commented Aug 2, 2017

scott-r-lindsey commented Aug 2, 2017

chalasr commented Aug 2, 2017

scott-r-lindsey commented Aug 3, 2017

nicolas-grekas commented Feb 4, 2017 •

edited

Loading

nicolas-grekas commented Feb 5, 2017 •

edited

Loading

xkobal commented May 17, 2017 •

edited

Loading

xkobal commented May 17, 2017 •

edited

Loading