[DI] Add missing deprecation when fetching private services from ContainerBuilder #25244
Conversation
nicolas-grekas
commented
Dec 1, 2017
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #25242 |
| License | MIT |
| Doc PR | - |
|
Tests are red though, and this seems related. |
9f8d989 to
929c2d5
Compare
|
|
||
| foreach ($this->definitions + $this->aliasDefinitions as $id => $definition) { | ||
| if (!$definition->isPublic() || $definition->isPrivate()) { | ||
| $this->removedIds[$id] = true; |
There was a problem hiding this comment.
This is for ppl doing that, which is a no-op, but one we catch now:
| @@ -733,6 +733,7 @@ public function testEnvInId() | ||
| PsrContainerInterface::class => true, | ||
| ContainerInterface::class => true, | ||
| 'baz_%env(BAR)%' => true, | ||
| 'bar_%env(BAR)%' => true, |
There was a problem hiding this comment.
the added entries are the same as on 4.0/master: we now list the public-by-private legacy services as removed, even if they're not really for BC in 3.4.
There was a problem hiding this comment.
won't they appear as removed even if they are not in 4.0?
There was a problem hiding this comment.
wdyt? they are in 4.0
There was a problem hiding this comment.
definition isn't removed if injected twice or more, is it? If it isn't, wouldn't getRemovedIds() return an id that has not been removed? (I'm wondering why we populate removedIds from another place that the one which actually removes unused definitions)
| @@ -94,7 +94,7 @@ protected function instantiateController($class) | ||
| } catch (\TypeError $e) { | ||
| } | ||
|
|
||
| if ($this->container instanceof Container && in_array($class, $this->container->getRemovedIds(), true)) { | ||
| if ($this->container instanceof Container && isset($this->container->getRemovedIds()[strtolower($class)])) { |
There was a problem hiding this comment.
that's a bit unrelated, but still found while playing with getRemovedIds: removed services are provided as keys
There was a problem hiding this comment.
are you sure about the strtolower ? The normalized id is not the lowercase one
|
Now green |
929c2d5 to
93c0b38
Compare
|
Thank you @nicolas-grekas. |
…s from ContainerBuilder (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Add missing deprecation when fetching private services from ContainerBuilder | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25242 | License | MIT | Doc PR | - Commits ------- 93c0b38 [DI] Add missing deprecation when fetching private services from ContainerBuilder