-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DI] Deprecate Container::set for nulls, initialized and alias services #19668
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
) { | ||
return true; | ||
} | ||
if (array_key_exists($id, $this->services)) { | ||
@trigger_error(sprintf('Checking for the existence of a unset "%s" service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); |
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.
this does not make sense, as there won't be any unset services in 4.0, and code calling has()
has no way to know whether the service instance was reset or no.
And btw, you change the code to unset the array entry instead of setting it to null
, so this if
won't match anymore
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.
hmm no, we were already unsetting it, so we already don't have null values in this array
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.
The current behavior is messed up and untrusty. The property is protected (!). Lets be safe.
If we can go with private.. that be great, as of 4.0 this still can be considered a safeguard when sticking with protected.
I would say yes, because |
$this->services[$id] = $service; | ||
|
||
if (null === $service) { | ||
if (null === $service) { // safeguard for 4.0 |
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.
As long as we stick with protected $services
(opposed to private) i tend to keep this safeguard in 4.0, for historic reasons i guess.
It would be better to remove this whole block (adhere to the doc, like always) and remove all array_key_exists
thingy's in 4.0 (as those are safeguards too in a way, again due protected).
( @nicolas-grekas WDYT? |
Long story short; we should consider a BC-break marking |
@ro0NL we cannot make it private, as the methods generated in the dumped container need to access this property |
@stof makes sense. I still need to dig in this method generation, how that is affected. But first thought is to have a |
This deprecates setting an existing service to I think its easier to handle those cases in the very same PR. WDYT ? |
Agree. Im not sure about the real problem though, hence i asked nicolas-grekas. We could do typechecks, allow it only for the (exact) same type. But yeah, im curious how this is a real issue... edit: from the |
@ro0NL : This is an issue (from my POV) because of the reason stated by @sstok in #19668 (comment) and the fact that the container should probably not allow to be modified by any third party, appart from setting synthetic services. As most services should be stateless, this should not be a major issue (resetting their state should be something handled by something else than the container (The manager registry in the Doctrine case)). |
Got your point. I'm thinking too much registry-pattern. From the DI pov that makes really sense. |
@@ -169,22 +170,25 @@ public function set($id, $service) | |||
} | |||
|
|||
if (isset($this->aliases[$id])) { | |||
unset($this->aliases[$id]); | |||
@trigger_error(sprintf('Replacing the "%s" (alias of "%s") service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id, $this->aliases[$id]), E_USER_DEPRECATED); | |||
} elseif (isset($this->services[$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.
I think isset
is not enough to check if a previous service exists (see Container::has()
).
But I don't know if we should allow replacing a non instantiated service or not.
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.
AFAIK null
/whatever can only be expected due the fact of protected $services
, or what am i missing here? Ie. set()
was already calling unset($this->services[$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 I'll answer well your question, but have a look to the ProjectServiceContainer.
The protected $services
is an empty array. has
will however return true for the ->has('bar')
call, because the getBarService()
method exists. ->get('bar')
will normally lazily create the service by calling the method, creating the instance and assign it to the service id. (However, it seems this container is missing the service id assignation in the getBarService()
. Have a look to your appDevDebugProjectContainer
eventually)
So, actually, a non-initialized service may not have its id in $this->services
, thus isset($this->services[$id])
won't be sufficient.
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.
Undertand, we should deal with the possible method mapping, right? I thought you meant adding array_key_exists here.
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.
Right.
@@ -839,6 +840,7 @@ public function __construct() | |||
|
|||
$code .= "\n \$this->services = array();\n"; | |||
$code .= $this->addMethodMap(); |
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 we forgot $code .= $this->addPrivateServices();
here (see addConstructor
), should we add it?
@nicolas-grekas @ogizanagi i think i got it pretty much covered. This day was crazy :-) My last commit seemed to be the missing piece. However i cant really explain the failing 7.1 tests, it seems to crash and a bunch of unrelated errors. fabbot.io also wants me to break tests ;-) And updated description. |
@@ -545,6 +545,9 @@ public function compile() | |||
if (!$definition->isPublic()) { | |||
$this->privates[$id] = true; | |||
} | |||
if ($definition->isSynthetic()) { | |||
$this->synthetics[$id] = true; |
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 know it was introduced this way for the $privates
array, but to me, it doesn't make entirely sense.
Why not simply keeping an array of synthetics services' ids instead of this map ?
Even if it won't happen, that's misleading, as we use isset($this->privates/synthetics[$idToCheck])
later in the codebase, which returns true
even if the value in the map is false
.
A simple in_array($idToCheck, $this->privates/synthetics)
would make more sense.
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.
Agree. I was also thinking $this->serviceTypes
something to track&check things in 1 go.
$this->serviceTypes[$id] & self::IS_PRIVATE
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.
@hhamon: Could you, if you can recall, tell us if there is any reason it was implemented this way in #19146 ?
- Maybe it was supposed to hold the generated id instead of a boolean later ? (Thus, it still won't make sense for synthetics service to behaves the same).
- Maybe for performances reasons ? I doubt it'll be called often enough to have a real impact, but it probably deserves profiling it with blackfire on a real application.
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.
Easy/fast isset
i guess.
Still considering we should deprecate (ie. track) previously declared services, perhaps we're making it unnecessarily complex. Like i explained in #19683 (comment); from the container pov this really doesnt matter too much. Although i can imagine we dont want to allow for this specific behavior on our implementation. Long story short; im not sure we should keep tracking stuff like this and maybe think more compiler passes. Ie. #19683 rules out the |
@@ -66,6 +66,7 @@ class Container implements ResettableContainerInterface | |||
protected $services = array(); | |||
protected $methodMap = array(); | |||
protected $privates = array(); | |||
protected $synthetics = array(); |
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.
we don't need this is we don't put synthetic services in $this->methodMap
, should allow simplifying many things
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.
Are you sure? https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L883
But yeah, maybe we shouldnt. Relates to #19690 (comment)
edit: We lose service id's that way
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.
Method for synthetic services throw exceptions, which means they are functionally useless. So for me it's safe to remove them from methodMap yes.
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.
True. They need to be included in getServiceIds
though.. however falling back to method_exists/get_class_method
at runtime will cover.
Which is only needed to cover synthetics.. ie. having it al in methodMap
would avoid such calls (as we can lazy load the map once).
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.
they don't need to be listed in getServiceIds
when they are not yet defined to me
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.
Could be, perhaps ideally we should just deprecate this method and dont care :) it can be
interpreted in different ways.
From the component pov i think it should include all possible validly id's (not depending on state).
Different approach; only disallow overwriting initialized services, including privates. |
rebase needed |
if (null === $service) { | ||
unset($this->services[$id]); | ||
@trigger_error(sprintf('Unsetting a service ("%s") is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); |
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.
the previous message was fine, and I'd keep the existing style for new messages too (the "%s" service/alias
vs a service (%s)
)
I can work on it this week(end). |
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.
Looks still good to me, and I think there should be a next step (later PR): since laziness is going to be a first class citizen, I think we might allow resetting a service again using set('foo', null)
only and only if the foo service is lazy, either in the current way, or in the new ways (closure-proxy, etc). This would basically generalize #19203 (which could then be moved back to a simple set($name, null)
.
|
||
/** | ||
* @group legacy | ||
* @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered |
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.
to be replaced by @expectedDeprecation
instead
if (isset($this->privates[$id])) { | ||
if (null === $service) { | ||
@trigger_error(sprintf('Unsetting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); | ||
unset($this->privates[$id]); | ||
} else { | ||
@trigger_error(sprintf('Setting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. A new public service will be created instead.', $id), E_USER_DEPRECATED); | ||
} | ||
} elseif (null === $service) { | ||
@trigger_error(sprintf('Unsetting the "%s" service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); |
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.
3.3
if (isset($this->services[$this->aliases[$id]])) { | ||
@trigger_error(sprintf('Replacing the initialized "%s" service alias is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); | ||
} else { | ||
@trigger_error(sprintf('Replacing the "%s" service alias is deprecated since Symfony 3.2 and will set the aliased service instead in Symfony 4.0.', $id), E_USER_DEPRECATED); |
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.
this message is strange: how is one supposed to patch its code to not get it anymore?
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.. the behavior is strange also :). Imo. you cant override an alias, per definition.
So the path would be; dont set the alias?
Sounds good.
I think we need a reset-able container for this, ie. |
} | ||
|
||
if (isset($this->aliases[$id])) { | ||
if (isset($this->services[$this->aliases[$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.
This "if" should be removed, we shouldn't add more conditionals here.
|
||
if (isset($this->services[$id])) { | ||
@trigger_error(sprintf('Replacing the initialized "%s" service is deprecated since Symfony 3.3 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); | ||
} |
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.
there is a missing case here, which is in fact what's described in #19192: initialized or not, we should deprecate setting a pre-defined non-synthetic services. This means we should check methodsMap and if there is a match, trigger the deprecation.
Updated. @nicolas-grekas can you have one more look at tests.. i had to do some changes. edit: yeah.. how we detect if a service is synthetic? |
if (isset($this->privates[$id])) { | ||
if (null === $service) { | ||
@trigger_error(sprintf('Unsetting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); | ||
unset($this->privates[$id]); | ||
} else { | ||
@trigger_error(sprintf('Setting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. A new public service will be created instead.', $id), E_USER_DEPRECATED); | ||
} | ||
} elseif (null === $service) { | ||
@trigger_error(sprintf('Unsetting the "%s" service is deprecated since Symfony 3.3 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); | ||
unset($this->services[$id], $this->aliases[$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.
This should probably unset methodMap[$id]
as well..
@trigger_error(sprintf('Replacing the initialized "%s" service is deprecated since Symfony 3.3 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED); | ||
} | ||
|
||
if (isset($this->methodMap[$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.
elseif with previous if so we're sure to trigger only one 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.
set('kernel', $x)
breaks a lot of tests now.. i was thinking to additionally track synthetic ids, or maybe simpler; dump a special method name func synthetic_getFooService()
?
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.
or remove synthetic services from methodMap? they don't need to be there to me (neither to we have do generate any method for them), wdyt?
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.
See #21244
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.
see #21244 also
} | ||
|
||
if (isset($this->aliases[$id])) { | ||
@trigger_error(sprintf('Replacing the "%s" service alias is deprecated since Symfony 3.3 and will set the aliased service instead in Symfony 4.0.', $id), E_USER_DEPRECATED); |
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'm sure I'm missing something here :)
What will be the code/behavior you'd expect on 4.0? Not an exception?
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.
$id = $this->aliases[$id]
and forward :) i think it's ok.
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.
return $this->set($this->aliases[$id], $service);
actually.
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.
OK. I'd be stricter here, and plan to throw also. No need for any special case, from the outside POV, alias or not, redefining something should throw.
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 was also leaning that way.. makes sense. Lets do #21244 first, ill finish this one the weekend.
ContainerBuilder also needs an update, isn't it? |
…ted methods (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Remove synthetic services from methodMap + generated methods | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - For synthetic services, the generated methods are just dead code to fill opcache ;) And having them in "methodMap" prevents using the property for checking if a service comes from a (non-synthetic) definition or not. This prepares some changes that we'd like to do in 4.0, see #19668. Commits ------- c1e1e99 [DI] Remove synthetic services from methodMap + generated methods
Rebased. Looking at
|
I think this one should be closed in favor of #21533: setting no |
Figured it out :)... this is not gonna happen. |
This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Deprecate (un)setting pre-defined services | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | no | Fixed tickets | #19192 | License | MIT | Doc PR | - This PR is the subset of #19668 that fixes #19192: it deprecates (un)setting pre-defined services. This opens the path to some optimizations in the dumped container in 4.0. Commits ------- fdb2140 [DI] Deprecate (un)setting pre-defined services
Not sure what i got myself into, it's a true adventure so far. But this deprecates
Container::set('id', null)
😱 andContainer::set('id', $previousSetService)
ContainerInterface::remove
if we need to support it (so far im not sure really)ContainerBuilder
- [DI] Deprecate Container::isFrozen and introduce isCompiled #19673, [DI] Allow getting synthetic before compilation #19715Before/After:
set('id', null)
set('id', $instance)
set('alias', $instance)