[RFC] [Discussion] #[\Deprecated] attribute again v1.3

On Tue, Apr 23, 2024 at 7:27 PM Levi Morrison levi.morrison@datadoghq.com
wrote:

On Tue, Apr 23, 2024 at 7:30 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

Hi internals,

My PR for #[\Deprecated] attribute was in hibernation for a long while
now and after some off-list discussion a few weeks ago I have decided to
revisit it and asked Tim to help me out with the work.

Tim has cleaned up the PR quite a bit and also worked in additional
features such as #[Deprecated] support in stub generation.

While there are still some small todos, at this point we want to restart
the discussion about the RFC for inclusion in 8.4:

RFC: https://wiki.php.net/rfc/deprecated_attribute
PR: https://github.com/php/php-src/pull/11293
Old discussion: https://externals.io/message/112554#112554

Let me know about your questions and feedback.

greetings
Benjamin

I skimmed through the previous discussion and didn't see anything
about adding a since property. This is occasionally useful, at least
in my limited usage of it in Rust. The names below are modelled after
the names in Rust's deprecated attribute, but "note" is the same
as the proposed "message":

#[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_FUNCTION)]
class Deprecated
{
    public function __construct(
        public readonly ?string $note = null,
        public readonly ?string $since = null
    ) {
    }
}

#[Deprecated(since: "1.3", note: "this is not good, use good_pls_use")]
function bad_dont_use() {}

#[Deprecated("this wasn't meant to be public, use good_pls_use instead")
function oops_dont_use() {}

function good_pls_use() {}

In Rust, you get a message for each of "since" and "note". In
PHP, this might look something like:

Deprecated: Function bad_dont_use() is deprecated since 1.3,
this wasn't meant to be public, use good_pls_use instead in %s
on line %d

This request is similar to Roman's question about a replacement parameter
elsewhere. We are unsure about these, because from an engine POV they do
not add value, and from a user messaging perspective they could be put into
the message with #[\Deprecated("since 1.3, this is not good, use
good_pls_use")].

The only reason this might make sense is to allow third party tooling to
work on this, but there are no conventions ala php-doc here in place
already. If tools need more infos, they could just introduce their own
attributes. example:

#[\Deprecated, Since("1.3"), Replacement("good_pls_use()")].

It feels arbitrary if we add parameters that the engine does not use and
where no tooling conventions exist on how they are being used, so we left
them out for now.

On Tue, Apr 23, 2024 at 7:27 PM Levi Morrison levi.morrison@datadoghq.com
wrote:

On Tue, Apr 23, 2024 at 7:30 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

Hi internals,

My PR for #[\Deprecated] attribute was in hibernation for a long while
now and after some off-list discussion a few weeks ago I have decided to
revisit it and asked Tim to help me out with the work.

Tim has cleaned up the PR quite a bit and also worked in additional
features such as #[Deprecated] support in stub generation.

While there are still some small todos, at this point we want to restart
the discussion about the RFC for inclusion in 8.4:

RFC: https://wiki.php.net/rfc/deprecated_attribute
PR: https://github.com/php/php-src/pull/11293
Old discussion: https://externals.io/message/112554#112554

Let me know about your questions and feedback.

greetings
Benjamin

I skimmed through the previous discussion and didn't see anything
about adding a since property. This is occasionally useful, at least
in my limited usage of it in Rust. The names below are modelled after
the names in Rust's deprecated attribute, but "note" is the same
as the proposed "message":

#[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_FUNCTION)]
class Deprecated
{
    public function __construct(
        public readonly ?string $note = null,
        public readonly ?string $since = null
    ) {
    }
}

#[Deprecated(since: "1.3", note: "this is not good, use good_pls_use")]
function bad_dont_use() {}

#[Deprecated("this wasn't meant to be public, use good_pls_use instead")
function oops_dont_use() {}

function good_pls_use() {}

In Rust, you get a message for each of "since" and "note". In
PHP, this might look something like:

Deprecated: Function bad_dont_use() is deprecated since 1.3,
this wasn't meant to be public, use good_pls_use instead in %s
on line %d

After discussing with Mate shortly one reason for adding $since from a PHP
project POV is that we do show the $since information in the generated
documentation output.

Integrating with the work in progress to auto generate parts of the
function docs based on the stub files, having the $since attribute on the
stubs would allow to use this as the central information in code. Therefore
we would reconsider and add the $since argument to the Deprecated class.

I am still partly on the side on Rowan (
https://externals.io/message/123184#123206) that i don't find the arguments
for this parameter very convincing but at least there is a use-case
internally now that warrants adding it.

On Tue, Apr 23, 2024 at 3:30 PM Benjamin Außenhofer kontakt@beberlei.de
wrote:

Hi internals,

My PR for #[\Deprecated] attribute was in hibernation for a long while
now and after some off-list discussion a few weeks ago I have decided to
revisit it and asked Tim to help me out with the work.

Tim has cleaned up the PR quite a bit and also worked in additional
features such as #[Deprecated] support in stub generation.

While there are still some small todos, at this point we want to restart
the discussion about the RFC for inclusion in 8.4:

RFC: https://wiki.php.net/rfc/deprecated_attribute
PR: https://github.com/php/php-src/pull/11293
Old discussion: https://externals.io/message/112554#112554

Let me know about your questions and feedback.

greetings
Benjamin

JetBrains was mentioned in the previous discussion but not sure if this
was considered in the RFC:
https://github.com/JetBrains/phpstorm-attributes/blob/master/src/Deprecated.php

Yes we considered it, but this is one implementation of a vendor, it might
be conflicting with requirements of other projects and vendors. For the
implementation and the engine itself this information is just "weight" that
is carried around, so we decided not to add them, because its harder to
decide on what to include than recommending tools to add their own
supplemental attributes.

On Wed, Apr 24, 2024 at 3:57 PM Nicolas Grekas nicolas.grekas+php@gmail.com
wrote:

Hi Benjamin,

My PR for #[\Deprecated] attribute was in hibernation for a long while now

and after some off-list discussion a few weeks ago I have decided to
revisit it and asked Tim to help me out with the work.

Tim has cleaned up the PR quite a bit and also worked in additional
features such as #[Deprecated] support in stub generation.

While there are still some small todos, at this point we want to restart
the discussion about the RFC for inclusion in 8.4:

RFC: https://wiki.php.net/rfc/deprecated_attribute
PR: https://github.com/php/php-src/pull/11293
Old discussion: https://externals.io/message/112554#112554

Let me know about your questions and feedback.

Thanks for the RFC.

While I'd like to be able to replace as many annotations as possible with
attributes, the devil is in the details and here, I'm not sold about the
details:

• I concur with others about the "since" property being missing

• We'd also need a "package" property so that it's trivial to know
  which composer package is deprecating.

• The attribute class should not be final, so that packages could
  subclass, e.g. to define the "since" or "package" property once for all -
  or to define a custom constructor, etc.

As the RFC mentions, this is technically not possible, because the
attribute is processed during compilation and an instanceof check at that
point is not legal (cant autoload more during compiling).

Even considering we can solve this technically, it would also otherwise
secretly mean that all method/function attributes are getting autoloaded
and this would break the core assumption of attributes being backed by
classes on Reflection access only.

• We should be able to add the attribute to a class.

This is in the future scope as it requires a very different (orthogonal)
implementation.

Yes, the "package" + "since" info can be put in the message itself, but
having a structured way to declare them is critical to 1. be sure that
authors don't forget to give that info and 2. build tools around that.

Are there tools around them? I can't find a use case in my head what the
"since" property can be used for.

Both since and package would be optional attributes, so tooling that checks
they are not forgotten is needed anyways. It could just as well check for
missing additional attributes next to the internal Deprecated attribute.

You're saying that these are not useful because the engine wouldn't make
anything useful out of e.g. "since".
That's true, although I'd suggest using them to prefix the final message.
If that's the policy around attributes for the engine, then I'm wondering
if the attribute shouldn't live elsewhere. Or if we shouldn't discuss this
policy.

The attribute is there to close the gap with internal functions
ZEND_ACC_DEPRECATED flag and ReflectionFunction/Method::isDeprecated
already existing. This cannot live elsewhere.

The since, package (and replacement) requirements however could live
elsewhere.

Given the many different requirements, we could think about adding an extra
variadic argument, so that you could add whatever information you pass to
the attributa via a key value array returned from
Deprecated::getExtraInformation() ;

I also anticipate a problem with the adoption period of the attribute: in
order to be sure that a call to a deprecated method will trigger a
deprecation, authors will have to 1. add the attribute and 2. still call
trigger_error when running on PHP < 8.next. That's a lot of boilerplate.

This is a problem of any new feature that covers a use-case previously
solved differently, for example attributes themselves, where
libraries/Frameworks shipped both at the same time for a while. Taking this
argument at face value would mean we stop evolving PHP.

Personally, I'm not convinced that there should be any runtime
side-effects to the attribute. I'd prefer having it be just reported by
reflection.

This RFC does not introduce the runtime side effects of ZEND_ACC_DEPRECATED
flag on methods or functions. They already exist with internal functions.
Having this attribute not trigger the deprecation where they are already
triggered for internal functions introduces an inconsistency.

We should talk about how PHP implements deprecation tracking and logging as
a separate issue, I have a lot of ideas here how to change that.

Nicolas