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] Removed deprecated setting private/pre-defined services#22801
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
nicolas-grekas commentedMay 20, 2017 • 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.
right, private are "invisible"
looks like so, we have to do it in 3.4 |
ro0NL commentedMay 20, 2017
3.3 no? the sooner the better right; given 3.3 is still in RC fase, cant we push it? |
nicolas-grekas commentedMay 20, 2017
3.4 is fine, 3.3 is closed for "features" (deprecations are) |
| } | ||
| if (isset($this->privates[$id])) { | ||
| thrownewInvalidArgumentException(sprintf('You cannot set the private service "%s".',$id)); |
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 if this should throw actually? compared to setting a new service (Container::$synthetics?)
thats a bit more work though.. but we have to decide in this PR i guess :)
nicolas-grekas commentedMay 22, 2017
rebase needed |
xabbuh commentedMay 22, 2017
Can't we merge all your DI related PRs into one big PR that removes all deprecated code paths for that component? That would be easier to review IMO. |
ro0NL commentedMay 22, 2017
Not sure.. if it's OK i'd like to do it per feature :) prepared the PR's pretty well, and like to test/rebase after each update. |
nicolas-grekas commentedMay 22, 2017
so, you can rebase again |
…ro0NL)This PR was merged into the 3.4 branch.Discussion----------[DI] Deprecate Container::initialized() for privates| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes-ish| New feature? | yes| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->See#22801 (comment)Failing test seems unrelated.Commits-------e0eb247 [DI] Deprecate Container::initialized() for privates
| returntrue; | ||
| } | ||
| if (isset($this->privates[$id])) { |
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.
shouldn't this be moved earlier in 3.x ? If the private service was already instantiated, the code would return true above without triggering the notice
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.
Think so... will have a look soonish :)
| return; | ||
| } | ||
| if (isset($this->privates[$id])) { |
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.
Same here, shouldn't it be moved above the check for shared services ? Otherwise, getting an already initialized private service will not trigger the deprecation
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@stofCommits-------4f683a9 [DI] Check for privates before shared services
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/symfony#22801 (comment),symfony/symfony#22801 (comment)| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->cc@stofCommits-------4f683a9a5b [DI] Check for privates before shared services
nicolas-grekas commentedJul 11, 2017
rebase needed |
| if (isset($this->privates[$id])) { | ||
| @trigger_error(sprintf('Checking for the initialization of the "%s" private service is deprecated since Symfony 3.4 and won\'t be supported anymore in Symfony 4.0.',$id),E_USER_DEPRECATED); | ||
| returnfalse; |
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.
@nicolas-grekas made a mistake here... alias ID resolution happens first. We dont do that inhas etc.
Although i think we actually should do that =/ (could be a new feature and/or patch 3.3/4; wdyt?)
edit: see#23492
nicolas-grekas commentedJul 17, 2017
ping@ro0NL do you need help for this one? would be great to finish it before working on your other ones :) |
ro0NL commentedJul 17, 2017
@nicolas-grekas rebased. To me it's ready.. :) |
| */ | ||
| publicfunctionget($id,$invalidBehavior =self::EXCEPTION_ON_INVALID_REFERENCE) | ||
| { | ||
| $serviceNotFound =function ($id)use ($invalidBehavior) { |
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.
I don't think this is correct. Perf wise, this will create a closure for each "get". But get is a perf critical piece, so that it should do as little as possible on each code path.
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.
Wouldstatic $serviceNotFound = function ($id, $invalidBehavior) {} be better? Otherwise extract levenshtein intogetAlternativeServiceIds() and inline from there?
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.
I think there is no need for any levenstein reco for private services. There is no typo there.
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.
of course :) updated.
ro0NL commentedJul 17, 2017
Perhaps something to start thinking about; are we allowed to break alias chains? |
nicolas-grekas commentedJul 17, 2017
ie? isn't that something already handled by some compiler pass (resolving aliases?) |
ro0NL commentedJul 17, 2017
Hm, if you have Not really played with it yet, just looking at the code path :) |
nicolas-grekas commentedJul 17, 2017
What I said: this is already resolved by a compiler pass so it cannot happen in practice. |
fabpot commentedJul 17, 2017
Thank you@ro0NL. |
…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 | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->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
Uh oh!
There was an error while loading.Please reload this page.
See#21533,#19708 and#19146
@nicolas-grekas privates should be excluded from
getServiceIdsright? i also wondered.. did we forget to deprecate checking privates withinitialized()?