Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Exclude private services from alternatives#19679
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ogizanagi commentedAug 20, 2016
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) { |
ogizanagiAug 20, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not using@expectedException &@expectedExceptionMessage annotations ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Copy paste :)
ogizanagiAug 20, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO, unless there is a good reason, new tests should use the annotations.
When oldest tests where introduced, I guess those annotations (as well asTestCase::setExpectedException()) were not available with the PHPUnit version in use :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, new tests should use the annotations.
ro0NL commentedAug 20, 2016
@ogizanagi could be done :) i dont mind. |
ogizanagi commentedAug 20, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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. |
| $alternatives =array(); | ||
| foreach ($this->servicesas$key =>$associatedService) { | ||
| if (isset($this->privates[$key])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about moving this check intogetServiceIds() and add an optional$private = true argument (which will be deprecated later, in order to thegetServiceIds() to only return public services ids ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense for havinggetServiceIds 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 usesgetServiceIds now.
ogizanagiAug 20, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 usefalse. (if any internal call needs privates services, we'll have to add another$triggerDeprecation = true argument and set it tofalse, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes. Why did you changed ?
Waiting for upstream to change 😛, but ill add it. Makes sense.
Got your point aboutgetServiceIds. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ogizanagi something like this?
edit: it makes no real sense though.. we can just deprecate privates ingetServiceIds on master, right? ie. new/update PR. as it would fix this issue automatically.
ogizanagiAug 20, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I don't understand your concerns 😕 ?
You can't just deprecate privates ingetServiceIds 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Got it.master != 4.x this keeps confusing me 😅
| publicfunctiongetServiceIds(/*$includePrivates = true*/) | ||
| { | ||
| $includePrivates =true; | ||
| if (func_num_args() ===1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
After#19685 䳪