Fix false-positive deprecation notices for TranslationLoader and WriteCheckSessionHandler #26193
Conversation
|
if we want to make code loading definitions of all services happy, we would have to avoid such deprecation everywhere in the codebase. A better solution would be to have your code checking services to skip services marked as deprecated (these services are properly deprecated). |
|
I agree with @stof here. |
|
Only these two classes trigger the deprecation notices - that is the whole point of this change. No other parts are affected. How could the autoconfiguration of Symfony be used to avoid this problem? Because in this case, we check each service for a specific parent class. Is there really an autoconfiguration for that? And the example is not super exotic: I took it from a library, and this kind of iterating over service definitions is not too special. In the original issue somebody referenced https://github.com/schmittjoh/JMSAopBundle/blob/master/DependencyInjection/Compiler/PointcutMatchingPass.php#L116 as an example. I suspect there are many more. Sure, you can just tell all the libraries that their iterating over service definitions is not optimal - good luck with that. |
|
@iquito autoconfiguration is opt-in. This means that only services configured to be autoconfigured would be checked (and core services are not enabling autoconfiguration on themselves as we configure them explicitly). |
|
In this example I am not using a tag - I am basically doing automatic setter injection for all services implementing a certain abstract class. I looked into autoconfiguration, but it does not do the same thing - the only workaround with autoconfiguration would be to define an extra tag which is only used to then get the services using that tag, which seems more nasty / confusing than just using a compiler pass, because you are using a public tag for an essentially "internal" usage. Going through the service definitions of the container to do something extra to some of the definitions seems like a valid use case to me, adding a tag is only one of those use cases - there are other use cases for decorating the services, which mostly require loading the classes of these services to analyze them. For me it was confusing and unexpected that deprecation warnings were generated in this case, because the classes emitting the warnings were never used - after container compilation the classes were never even loaded anymore, yet the warnings were displayed on every single page. I suggested these changes because I don't think I will be the last one to encounter these problems. Avoiding these specific type of false-positives would therefore help other people who use compiler passes in this way, or who use a library which uses such compiler passes, to not waste a lot of time analyzing deprecation problems that do not actually exist. You are not doing me a favor by changing the deprecation warnings - I have already over-analyzed this extensively and will probably ignore these kinds of deprecation warnings in the future, because I now know that such false positives can happen. I would suggest though that where and how deprecation warnings are emitted should be an ongoing discussion to specifically avoid showing such warnings when they are not warranted. |
|
@stof do you anticipate any downside merging this? |
|
Trying to find the source of this weird deprecation message for hours until I found this PR. |
|
@nurtext There is no clean way of avoiding the deprecations. If you are iterating over the container definitions in your own code, you can check if the service is deprecated and ignore it if it is - in my code this currently looks like this: If you are using a library which iterates over the container definitions for you, then there is probably nothing you can do, as the isDeprecated check is just a hack and not a solution. You also need to keep in mind that it prevents you from decorating deprecated services - so if you yourself deprecate services, or you are using library services which you want to decorate yet they are set as deprecated, then you are out of luck and might experience unexpected consequences. Another option, if that is available to you, is to upgrade to Symfony 4.0, which has removed these classes, so the deprecation notices will disappear as soon as you use Symfony 4.0. |
|
Let's be pragmatic here. Merging. |
…eCheckSessionHandler
|
Thank you @iquito. |
…er and WriteCheckSessionHandler (iquito) This PR was squashed before being merged into the 3.4 branch (closes #26193). Discussion ---------- Fix false-positive deprecation notices for TranslationLoader and WriteCheckSessionHandler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25518 | License | MIT Symfony 3.4 emits deprecation warnings for `TranslationLoader` and `WriteCheckSessionHandler` as soon as these classes are loaded, yet at the same time these classes are part of the default services defined in Symfony 3.4, so if these classes are loaded during container compilation a deprecation warning is emitted, even if these classes are never actually used. An example would be the following within a compiler pass: foreach ($containerBuilder->getDefinitions() as $definition) { if (is_subclass_of($definition->getClass(), SomeClass::class)) { $definition->addMethodCall('setSomething', [new Reference('someservice')]); } } This will load both `TranslationLoader` and `WriteCheckSessionHandler` in order to check their definition. No instance of the classes are ever used and the classes are not loaded after compilation ever, yet the deprecation notices are shown on every single page. More details are provided in issue #25518 . By moving the deprecation notices to the class constructors false-positives are avoided while actual usage of the classes should still generate the deprecation warnings. Commits ------- 1a427b1 Fix false-positive deprecation notices for TranslationLoader and WriteCheckSessionHandler
Symfony 3.4 emits deprecation warnings for
TranslationLoaderandWriteCheckSessionHandleras soon as these classes are loaded, yet at the same time these classes are part of the default services defined in Symfony 3.4, so if these classes are loaded during container compilation a deprecation warning is emitted, even if these classes are never actually used.An example would be the following within a compiler pass:
This will load both
TranslationLoaderandWriteCheckSessionHandlerin order to check their definition. No instance of the classes are ever used and the classes are not loaded after compilation ever, yet the deprecation notices are shown on every single page. More details are provided in issue #25518 .By moving the deprecation notices to the class constructors false-positives are avoided while actual usage of the classes should still generate the deprecation warnings.