[DependencyInjection] deprecate access to private shared services. #19146

hhamon · 2016-06-22T20:08:37Z

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

hhamon · 2016-06-22T20:30:21Z

Looks like fabbot gives a false positive!

fabpot · 2016-06-23T05:39:16Z

But tests are truly broken :)

fabpot · 2016-06-23T05:39:42Z

UPGRADE-4.0.md

+
+ * Requesting a private shared service with the `get()` method of the `Container`
+   class is no longer supported and will raise an exception.
+


You also need to add a note in the component CHANGELOG file.

fabpot · 2016-06-23T05:43:51Z

I would have implemented this feature in a different way by renaming internally private services so that their original name is not available from the container (this is what we are doing for anonymous services for instance).

xabbuh · 2016-06-23T06:25:52Z

I think it's a good idea to switch to generated ids in 4.0. For now I like this approach as it allows users to fix their applications more smoothly.

javiereguiluz · 2016-06-23T06:35:35Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -198,6 +203,10 @@ public function has($id)
            if (--$i && $id !== $lcId = strtolower($id)) {
                $id = $lcId;
            } else {
+                if (isset($this->privates[$id])) {
+                    @trigger_error(sprintf('Checking for the existence of the private service "%s" is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. An exception will be thrown instead in Symfony 4.0.', $id), E_USER_DEPRECATED);


Why throw an exception in this case? If you check a private service for existence, the method should return false, right?

Yes it makes more sense!

fabpot · 2016-06-23T07:38:02Z

@xabbuh Fair enough, let's keep the current approach for now.

hhamon · 2016-06-23T09:19:02Z

I'll update the code later today to reflect your comments.

sstok · 2016-06-23T09:19:20Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -176,6 +177,10 @@ public function set($id, $service)
        if (null === $service) {
            unset($this->services[$id]);
        }
+
+        if (isset($this->privates[$id])) {
+            @trigger_error(sprintf('Resetting the private service "%s" from outside of the container is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. An exception will be thrown instead in Symfony 4.0.', $id), E_USER_DEPRECATED);


. and is part of new sentence part and should start with an uppercase 👍

Where is that?

stof · 2016-06-23T10:01:10Z

@fabpot generated ids are a good idea for 4.0 indeed, but this cannot be done for deprecations. If we want to switch to generated ids already, we would have to build another feature to to store a mapping between shared private service ids and their generated ids, to retrieve them this way and trigger a deprecation, which would again look like this approach.

hhamon · 2016-06-23T20:07:02Z

I don't understand why the tests suite fails on PHP 5.5 and HHVM. The failing test in the ProxyManager Bridge passes on my machine with PHP 5.5.36...

fabpot · 2016-06-24T05:26:35Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -268,6 +281,9 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE

                return;
            }
+            if (isset($this->privates[$id])) {
+                @trigger_error(sprintf('Requesting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. An exception will be thrown instead in Symfony 4.0.', $id), E_USER_DEPRECATED);


I won't necessarily throw an exception as it depends on the $invalidBehavior value.

yes that's right!

GuilhemN · 2016-06-24T13:25:00Z

The tests are missing for the ContainerBuilder :)

hhamon · 2016-06-24T16:41:35Z

Thanks @Ener-Getick! I'm going to add some ;)

hhamon · 2016-06-27T11:07:10Z

I guess it's now ready for another round of reviews if tests pass.

nicolas-grekas · 2016-06-27T11:24:07Z

👍 (fabbot fails on a fixture, unrelated)

fabpot · 2016-06-28T06:11:06Z

UPGRADE-4.0.md

+   no longer supported. Only public services can be set or unset.
+
+ * Checking the existence of a private service with the `Container::has()`
+   method is no longer supported.


It's not that it is not supported anymore but more that it's going to return false.

fabpot · 2016-06-28T06:12:59Z

Apart from my minor comment, 👍

…xes issue symfony#19117.

hhamon · 2016-06-28T07:17:24Z

@fabpot fixed! CI is running ;)

hhamon · 2016-06-28T07:45:35Z

Good to go!

fabpot · 2016-06-29T05:51:38Z

Thank you @hhamon.

…ed services. (hhamon) This PR was merged into the 3.2-dev branch. Discussion ---------- [DependencyInjection] deprecate access to private shared services. | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #19117 | License | MIT | Doc PR | ~ Commits ------- 4ed70c4 [DependencyInjection] deprecate access to private shared services. Fixes issue #19117.

@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

carsonbot added Status: Needs Review DependencyInjection Deprecation labels Jun 22, 2016

hhamon force-pushed the private-services-fix branch 4 times, most recently from 02e1480 to 9af32f8 Compare June 22, 2016 20:23

fabpot reviewed Jun 23, 2016
View reviewed changes

javiereguiluz reviewed Jun 23, 2016
View reviewed changes

hhamon changed the title ~~[DependencyInjection] deprecate access to private shared services.~~ [WIP] [DependencyInjection] deprecate access to private shared services. Jun 23, 2016

sstok reviewed Jun 23, 2016
View reviewed changes

hhamon force-pushed the private-services-fix branch 3 times, most recently from 18ba656 to 19f3c66 Compare June 23, 2016 19:41

fabpot reviewed Jun 24, 2016
View reviewed changes

hhamon force-pushed the private-services-fix branch 2 times, most recently from a43a537 to 13edce2 Compare June 27, 2016 11:03

hhamon changed the title ~~[WIP] [DependencyInjection] deprecate access to private shared services.~~ [DependencyInjection] deprecate access to private shared services. Jun 27, 2016

hhamon force-pushed the private-services-fix branch from 13edce2 to 326cbeb Compare June 27, 2016 11:13

nicolas-grekas mentioned this pull request Jun 27, 2016

Deprecate Container::set() for non-synthetic services #19192

Closed

nicolas-grekas added Status: Reviewed and removed Status: Needs Review labels Jun 27, 2016

fabpot reviewed Jun 28, 2016
View reviewed changes

[DependencyInjection] deprecate access to private shared services. Fi…

4ed70c4

…xes issue symfony#19117.

hhamon force-pushed the private-services-fix branch from 326cbeb to 4ed70c4 Compare June 28, 2016 07:16

fabpot merged commit 4ed70c4 into symfony:master Jun 29, 2016

linaori mentioned this pull request Jul 12, 2016

Controller as a Service cookbook article missing failfast mentioning symfony/symfony-docs#6749

Closed

ogizanagi mentioned this pull request Aug 20, 2016

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

Closed

ro0NL mentioned this pull request Aug 20, 2016

[DI] Randomize private services #19683

Closed

bestform mentioned this pull request Sep 2, 2016

Private services stay public when required by more than one service #12588

Closed

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

hhamon deleted the private-services-fix branch February 22, 2017 13:13

ro0NL mentioned this pull request May 20, 2017

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] deprecate access to private shared services. #19146

[DependencyInjection] deprecate access to private shared services. #19146

hhamon commented Jun 22, 2016 •

edited

Loading

hhamon commented Jun 22, 2016

fabpot commented Jun 23, 2016

fabpot Jun 23, 2016

fabpot commented Jun 23, 2016

xabbuh commented Jun 23, 2016

javiereguiluz Jun 23, 2016

fabpot Jun 23, 2016

hhamon Jun 23, 2016

fabpot commented Jun 23, 2016

hhamon commented Jun 23, 2016

sstok Jun 23, 2016

hhamon Jun 23, 2016

stof commented Jun 23, 2016

hhamon commented Jun 23, 2016

fabpot Jun 24, 2016

hhamon Jun 24, 2016

GuilhemN commented Jun 24, 2016

hhamon commented Jun 24, 2016

hhamon commented Jun 27, 2016

nicolas-grekas commented Jun 27, 2016

fabpot Jun 28, 2016

fabpot commented Jun 28, 2016

hhamon commented Jun 28, 2016

hhamon commented Jun 28, 2016

fabpot commented Jun 29, 2016


		* Requesting a private shared service with the `get()` method of the `Container`
		class is no longer supported and will raise an exception.

[DependencyInjection] deprecate access to private shared services. #19146

[DependencyInjection] deprecate access to private shared services. #19146

Conversation

hhamon commented Jun 22, 2016 • edited Loading

hhamon commented Jun 22, 2016

fabpot commented Jun 23, 2016

Choose a reason for hiding this comment

fabpot commented Jun 23, 2016

xabbuh commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 23, 2016

hhamon commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jun 23, 2016

hhamon commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN commented Jun 24, 2016

hhamon commented Jun 24, 2016

hhamon commented Jun 27, 2016

nicolas-grekas commented Jun 27, 2016

Choose a reason for hiding this comment

fabpot commented Jun 28, 2016

hhamon commented Jun 28, 2016

hhamon commented Jun 28, 2016

fabpot commented Jun 29, 2016

hhamon commented Jun 22, 2016 •

edited

Loading