[DI] Exclude private services from alternatives #19679
Conversation
|
This should go into However, you won't be able to exclude privates as |
| try { | ||
| $sc->get('int'); | ||
| $this->fail('->get() throws an Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException if the key does not exist'); | ||
| } catch (\Exception $e) { |
There was a problem hiding this comment.
Why not using @expectedException & @expectedExceptionMessage annotations ?
There was a problem hiding this comment.
IMHO, unless there is a good reason, new tests should use the annotations.
When oldest tests where introduced, I guess those annotations (as well as TestCase::setExpectedException()) were not available with the PHPUnit version in use :)
There was a problem hiding this comment.
Indeed, new tests should use the annotations.
|
@ogizanagi could be done :) i dont mind. |
|
@ro0NL: I'd suggest you to create another PR for 2.7 and up, and keep this one to not loose track of the private services handling in 3.2. |
| @@ -270,6 +270,9 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE | |||
|
|
|||
| $alternatives = array(); | |||
| foreach ($this->services as $key => $associatedService) { | |||
| if (isset($this->privates[$key])) { | |||
There was a problem hiding this comment.
What about moving this check into getServiceIds() and add an optional $private = true argument (which will be deprecated later, in order to the getServiceIds() to only return public services ids ?
There was a problem hiding this comment.
Makes sense for having getServiceIds only expose public id's.
Not sure why we want $private = true. Opposed to refactoring stuff relying on the fact it returns private ids.
We should assume the foreach uses getServiceIds now.
There was a problem hiding this comment.
Not sure why we want $private = true. Opposed to refactoring stuff relying on the fact it returns private ids.
That's only in order to ensure BC. Having this argument set to true should trigger a deprecation. Internally, we'll only use false. (if any internal call needs privates services, we'll have to add another $triggerDeprecation = true argument and set it to false, but I doubt there is.)
We should assume the foreach uses getServiceIds now.
Yes. Why did you change ?
There was a problem hiding this comment.
Yes. Why did you changed ?
Waiting for upstream to change 😛, but ill add it. Makes sense.
Got your point about getServiceIds. It's public API.. which should keep returning privates till 4.0. Ill have a look at it.
| if (func_num_args() === 1) { | ||
| $includePrivates = !!func_get_arg(0); | ||
| if ($includePrivates) { | ||
| @trigger_error(sprintf('Including private services in the list of service id\'s is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); |
There was a problem hiding this comment.
@ogizanagi something like this?
edit: it makes no real sense though.. we can just deprecate privates in getServiceIds on master, right? ie. new/update PR. as it would fix this issue automatically.
There was a problem hiding this comment.
Sorry, I don't understand your concerns 😕 ?
You can't just deprecate privates in getServiceIds if you have no idea the privates were asked. And simply not returning them will be a BC break. Thus, the new argument to explicit the intention, and trigger the deprecation.
There was a problem hiding this comment.
Got it. master != 4.x this keeps confusing me 😅
| { | ||
| $includePrivates = true; | ||
| if (func_num_args() === 1) { |
There was a problem hiding this comment.
Nobody calls this with an argument, which means nobody will see the deprecation notice.
Yet, I think we should only add a note to remind about not returning private services in 4.0. No need for any deprecations: if people use the private services returned today from this method, they will get a deprecation because fetching them is deprecated.
After #19685 & #19690