[DependencyInjection] ContainerBuilder: Remove obsolete definitions #19608
Conversation
| || | ||
| (isset($this->obsoleteDefinitions[$id]) && !$this->obsoleteDefinitions[$id]->isSynthetic()) | ||
| ) { | ||
| if (!isset($this->definitions[$id]) || !$this->definitions[$id]->isSynthetic()) { | ||
| throw new BadMethodCallException(sprintf('Setting service "%s" on a frozen container is not allowed.', $id)); |
There was a problem hiding this comment.
Maybe this should be explicit: Setting service "%s" for an unknown or non-synthetic service definition is not allowed when the container is frozen.
There was a problem hiding this comment.
What about a check if $service is an instance of $definition->getClass() for sanity. Or "synchronize" the definition here accordingly, ie $definition->setClass(get_class($service));
There was a problem hiding this comment.
Thanks @ro0NL . I've changed the message according to your proposition.
About your other suggestion, I think it should be considered as a new "DX" feature, as it has been done in the past for similar exceptions giving better hint to the end user.
|
Thank you @ogizanagi. |
…definitions (ogizanagi) This PR was merged into the 3.1 branch. Discussion ---------- [DependencyInjection] ContainerBuilder: Remove obsolete definitions | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? |no | BC breaks? | may be considered | Deprecations? | may be considered | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - IIUC, [the obsolete definitions thing was tied to scoped & sync services](#7007). Thus, this code [is not meant to be used since 3.0 and can be removed](#13289). However, it may be considered as a BC break and would require introducing a new deprecation instead, because the current code allows to set a service on a frozen container if it has an obsolete synthetic definition...which is weird, isn't it ? (I doubt this feature was explicitly intended to allow setting a synthetic service more than once, and it wasn't before sync services introduction) I suggest this as a patch for 3.1, under the pretext of forgotten code tied to a previous deprecation & feature removal, but let me know if it should be considered differently. Commits ------- daa7d00 [DependencyInjection] ContainerBuilder: Remove obsolete definitions
|
Hello,
Yeah, me too, but here I was, trying to bend the container to my will and this solution worked. :) Fortunately I was able to rewrite the tests so that they set the service only once. |
|
@Revisor : Thanks for your feedback. I'm happy you were able to rewrite your tests. |
|
The services I wanted to swap were originally not synthetic, so I thought I had to first switch their definitions to synthetic, then compile the I managed to compress this into one step, just swap the services before compilation. So yeah, I was definitely doing it in a needlessly complicated way and this commit forced me to find a simpler solution. :] |
IIUC, the obsolete definitions thing was tied to scoped & sync services.
Thus, this code is not meant to be used since 3.0 and can be removed.
However, it may be considered as a BC break and would require introducing a new deprecation instead, because the current code allows to set a service on a frozen container if it has an obsolete synthetic definition...which is weird, isn't it ? (I doubt this feature was explicitly intended to allow setting a synthetic service more than once, and it wasn't before sync services introduction)
I suggest this as a patch for 3.1, under the pretext of forgotten code tied to a previous deprecation & feature removal, but let me know if it should be considered differently.