[DI] Warn when a definition relies on a deprecated class in ContainerBuilder::createService() #16001
Conversation
| <service id="annotations.file_cache_reader" class="%annotations.file_cache_reader.class%" public="false"> | ||
| <deprecated>The "%service_id% service is deprecated since 2.8 and will be removed in 3.0."</deprecated> |
There was a problem hiding this comment.
or should I remove the service right now since it's private?
There was a problem hiding this comment.
a private service can still be used by any other service. It is not about restricting access to it, but about giving a hint to the optimizer that this service is private to the container (i.e. the container can give it to other services, but not to the outside through get, and so it can inline it)
There was a problem hiding this comment.
yep, but this does not answer my question :) I bet that for bc, we should keep the definition even if it's unlikely that anyone reuses this service?
There was a problem hiding this comment.
I think that "even if it is unlikely" is still likely but meh.
My comment was mainly for the fact that you misplaced the closing ". :}
|
this should also be triggered when dumping the container rather than instantiating things from the ContainerBuilder directly. |
|
If we trigger these notices when dumping the container, we will autoload all service classes (since we need to load the class definition to get the annotation). Is this an issue at all or not? |
|
I'm not quite sure we should trigger these warning if there is a |
|
@Taluu your argument could be used against you own PR (#15491) but in fact knowing which service definition triggered some class deprecation notice is really valuable and is a time saver for devs. |
265f90d to
4728481
Compare
|
@nicolas-grekas they should not be triggered at dump-time. They should be triggered at runtime. So IMO, the right solution would be to trigger the deprecation in doctrine itself, as we do in Symfony. #15491 is there to support the case where we want to deprecate using a class as service instead of deprecating the class itself (perfect example: the |
|
I spent a bit more time on this: I think that doing the check at runtime is not possible because of the overhead it would add. We could also do the check in Which means that I don't have any better idea than this PR. Even if it won't catch much cases since as stof said nobody (but tests) uses the builder directly to get services, it still helped find the issue with this deprecated doctrine class. For this reason, I'd propose to merge this PR asis. @stof btw, the #14307 issue targetted by #15491 is not only about "the case where we want to deprecate using a class as service instead of deprecating the class itself". If you read the issue description, it's also clearly about triggering more useful messages, and I fully support this use case! |
4728481 to
249377c
Compare
…Builder::createService()
249377c to
ca69fa3
Compare
|
Thank you @nicolas-grekas. |
…ss in ContainerBuilder::createService() (nicolas-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [DI] Warn when a definition relies on a deprecated class in ContainerBuilder::createService() | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - The new feature is in the DI component and it enlighten a deprecation from Doctrine that we ignored in FrameworkBundle, that is also fixed in this PR. See https://github.com/symfony/symfony/pull/16001/files?w=1 Commits ------- ca69fa3 [DI] Warn when a definition relies on a deprecated class in ContainerBuilder::createService()
|
It seems that this PR is causing trouble. Issue: #16167 |
The new feature is in the DI component and it enlighten a deprecation from Doctrine that we ignored in FrameworkBundle, that is also fixed in this PR.
See https://github.com/symfony/symfony/pull/16001/files?w=1