Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123568
Message-ID: <4b6331be-e00c-4719-b3fe-d9d70995fde2@bastelstu.be>
Date: Mon, 10 Jun 2024 19:22:12 +0200
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PHP-DEV] Re: [RFC] [Vote] #[\Deprecated] attribute
To: =?UTF-8?Q?Benjamin_Au=C3=9Fenhofer?= <kontakt@beberlei.de>,
 PHP Internals <internals@lists.php.net>
References: <CAEPJdu=ZTyPQ3yabhOx=rxnv5niuA+9O4BP6EsvAdkZRSS5Q2g@mail.gmail.com>
 <CAEPJdu=Qum4-HygCbs8xgpiNhm+gdeP6rG6Ua8jApz7pZTKfvA@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CAEPJdu=Qum4-HygCbs8xgpiNhm+gdeP6rG6Ua8jApz7pZTKfvA@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/5/24 10:16, Benjamin Außenhofer wrote:
> Tim will finalize the implementation PR now and work on its merge in the
> upcoming days.
> 

During review Ilija pointed out that adding the internal 
ZEND_ACC_DEPRECATED flag was skipped for abstract methods and interface 
methods, questioning the decision. The decision dates back to Benjamin's 
initial PoC implementation and I've had a short discussion with him 
about this, with the result that it's probably best to ask the list, as 
it was not specified in the RFC.

Currently adding the #[\Deprecated] attribute on an abstract method or 
interface method does not do anything special. It only applies the 
attribute, which then can be accessed via Reflection.

It would be easy to change the implementation to also apply the 
ZEND_ACC_DEPRECATED flag. This would have the following consequences:

1. `ReflectionMethod::isDeprecated()` will return `true`.
2. No deprecation error will be emitted. Neither when implementing the 
method in a child class, nor when calling the method of the child class.

(1) is probably the obviously correct behavior. For (2) the reasoning is 
as follows:

- The implementation relies on the pre-existing behavior of 
ZEND_ACC_DEPRECATED, which does not emit a deprecation error for 
overridden methods / implemented abstract or interface methods.

- If the deprecation error would be emitted when implementing the 
method, then the error would not be actionable. Child classes are still 
forced to implement the abstract method / interface method, thus there 
is no way to change the code to not emit the error.

- It is not obviously correct to inherit the deprecation when overriding 
a non-abstract method of a parent class, just like attributes themselves 
are not inherited [1]. Abstract methods and interface methods should 
keep consistency with non-abstract methods. The reasoning why it's not 
obviously correct to inherit the deprecation is that due to not 
inheriting the Attribute, a custom deprecation message would not be applied.

It goes without saying that IDEs can opt to suggest adding the 
#[\Deprecated] attribute if the overridden method is deprecated itself 
by means of the existing code diagnosis functionality, making the 
attribute on abstract methods / interface methods useful by that means.

We plan to adjust the implementation to also apply to abstract and 
interface methods with the semantics outlined in (1) and (2). Does 
anyone have an opinion about this / does anyone disagree with that decision?

Best regards
Tim Düsterhus

[1] https://3v4l.org/GIZRj