Hi
On 6/27/24 13:10, Nicolas Grekas wrote:
That's sad news, because I keep explaining why engine-triggered runtime
notices are a terrible idea, yet you're planning to add more of them. The
consistency argument Tim wrote in another email isn't sound to me:
consistency with a bad idea doesn't make a good idea.
I've yet to hear a better solution for deprecation handling that does not involve third-party tooling. Being able to deprecate functionality is a core part of being able to evolve a programming language and just a note in the documentation no one reads anyways does not suffice.
In the hope it may be used as food for thoughts: when we use @deprecated
on a class, Symfony's DebugClassLoader throws a deprecation ONLY when a
class *extends* one of those @deprecated classes.
Thank you. I'll keep that in mind if/when we'll actually start working on an RFC adding support for deprecating classes.
For the runtime notice itself, we decide case by case among two
possibilities: either triggering the notice just before the class
declaration, or in the constructor. The reason is that sometimes we have to
load the class but we don't want to trigger the deprecation because loading
the class doesn't trigger any side-effects that users should be warned
about in relation to the deprecation. In such situations, we trigger in the
constructor.
I agree that just loading or defining a class should not emit a deprecation warning, just like loading or defining a function does not emit a deprecation warning.
About Attribute::TARGET_CLASS itself, not adding it right now will lead to
a situation where userland will have a hard time writing code that's
compatible with both 8.4 and 8.5 (assuming 8.5 is the moment where the flag
is added): using #[Deprecated] will be "illegal" when running on 8.4, yet
legal when running in 8.5?
That's another reason to allow the flag right away: smooth upgrades.
That is a fair concern.
I could imagine backporting the Attribute::TARGET_CLASS to all supported version if/when support for deprecating classes is actually added to the language and semantics are clear and considering that to be effectively a bugfix then.
My suggestion is quite restricted, and that'd solve all my concerns: it is
to enable the flag right now, and to add ReflectionClass::isDeprecated()
right now also if you want. But don't plan anything more on the topic,
except maybe a deprecation notice when *extending* a #[Deprecated] class.
But nothing more please. No runtime notice when loading the class.
The PR implementing the #[\Deprecated] attribute will implement exactly the semantics that were decided in the RFC. The implementation is already be complete and is currently pending JIT review by Dmitry. Assuming no change requests during the review I'll make a final cleanup pass and then it should be ready to merge.
Anything that is not in the RFC will (need to) be part of a separate PR.
Best regards
Tim Düsterhus