Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123948
Message-ID: <b53d2868-d3c1-4755-9386-1ca20faf713d@bastelstu.be>
Date: Thu, 27 Jun 2024 19:55:29 +0200
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PHP-DEV] Re: [RFC] [Vote] #[\Deprecated] attribute
To: Nicolas Grekas <nicolas.grekas+php@gmail.com>,
 =?UTF-8?Q?Benjamin_Au=C3=9Fenhofer?= <kontakt@beberlei.de>
Cc: PHP Internals <internals@lists.php.net>
References: <CAEPJdu=ZTyPQ3yabhOx=rxnv5niuA+9O4BP6EsvAdkZRSS5Q2g@mail.gmail.com>
 <CAEPJdu=Qum4-HygCbs8xgpiNhm+gdeP6rG6Ua8jApz7pZTKfvA@mail.gmail.com>
 <CAOWwgp=TXMJC2+mgnCGJH0jXLo8WiY2uMb8i5LVZJSdoodVH3w@mail.gmail.com>
 <CAEPJdunrH90G2Eg8UXNDBhLhg4rf+0QdmWwMwWvyn0UxtnnHzQ@mail.gmail.com>
 <CAOWwgpkAzMh7+Cj56Jw+Gyh-bfk6T_SeK5m-y2YMbEVAPA6u-A@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CAOWwgpkAzMh7+Cj56Jw+Gyh-bfk6T_SeK5m-y2YMbEVAPA6u-A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

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