Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[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 |
3.3 no? the sooner the better right; given 3.3 is still in RC fase, cant we push it? |
3.4 is fine, 3.3 is closed for "features" (deprecations are) |
| } | ||
| if (isset($this->privates[$id])) { | ||
| throw new InvalidArgumentException(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 :)
rebase needed |
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. |
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. |
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
| if (isset($this->services[$id])) { | ||
| return true; | ||
| } | ||
| 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 :)
| @@ -317,9 +317,6 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE | |||
| 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
rebase needed |
| @@ -299,7 +294,7 @@ public function initialized($id) | |||
| } | |||
| 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); | |||
| return false; | |||
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
ping@ro0NL do you need help for this one? would be great to finish it before working on your other ones :) |
@nicolas-grekas rebased. To me it's ready.. :) |
| @@ -225,8 +220,26 @@ public function has($id) | |||
| */ | |||
| public function get($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.
Perhaps something to start thinking about; are we allowed to break alias chains? |
ie? isn't that something already handled by some compiler pass (resolving aliases?) |
Hm, if you have Not really played with it yet, just looking at the code path :) |
What I said: this is already resolved by a compiler pass so it cannot happen in practice. |
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()?