[DI] Removed deprecated setting private/pre-defined services #22801
Conversation
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.
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.
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.
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.
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 @stof Commits ------- 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 @stof Commits ------- 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.
@nicolas-grekas made a mistake here... alias ID resolution happens first. We dont do that in has 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.
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.
Would static $serviceNotFound = function ($id, $invalidBehavior) {} be better? Otherwise extract levenshtein into getAlternativeServiceIds() and inline from there?
There was a problem hiding this comment.
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.
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
See #21533, #19708 and #19146
@nicolas-grekas privates should be excluded from
getServiceIdsright? i also wondered.. did we forget to deprecate checking privates withinitialized()?