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

Levi Morrison

2 years ago

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][1], but "note" is the same
as the proposed "message":

```php
#[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"][2]. 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

[1]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
[2]: https://github.com/rust-lang/rfcs/blob/master/text/1270-deprecation.md#intended-use

Benjamin Eberlei

2 years ago

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][1], but "note" is the same
> as the proposed "message":
>
> ```php
> #[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"][2]. 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.

Larry Garfield

2 years ago

On Wed, Apr 24, 2024, at 4:01 AM, Benjamin Außenhofer wrote:

> 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][1], but "note" is the same
>> as the proposed "message":
>>
>> ```php
>> #[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"][2]. 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.

I support this attribute, but would also like to see it fleshed out more.  A since parameter or a separate attribute I'm flexible on and open to discussing; I don't have a strong preference right now.  We should definitely see what other languages have found useful here beyond just Rust, which could give us a better idea of what is conventional/helpful.

My other question is whether E_USER_DEPRECATED is still the right error to throw, rather than E_DEPRECATED.  I'm not saying it isn't, just that I'm unsure and would like to dig into that a bit more.

--Larry Garfield

Benjamin Eberlei

2 years ago

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][1], but "note" is the same
> as the proposed "message":
>
> ```php
> #[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"][2]. 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.

Alex Wells

2 years ago

On Fri, Apr 26, 2024 at 11:01 AM Benjamin Außenhofer <kontakt@beberlei.de>
wrote:

> 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.
>

Hey. Before including the `since` parameter, please consider this:

Let's assume the usual (or at least, the most common) development flow
includes creating a separate branch where development happens before it's
merged into the default branch. With that in mind, let's go through the
flow:

- tag 2.1.1 exists at some ref
- the `master` branch is ahead of tag `2.1.1` and contains some
unreleased, untagged changes
- a new branch `deprecate-something` is created from `master`
- a function is deprecated with the attribute: `#[Deprecated(since:
'2.1.1')]`
- a pull request is created from `deprecate-something` into `master`, but
isn't merged yet
- a new version is tagged and released - either `2.1.2`, `2.2.0` or
`3.0.0`

At this point `deprecate-something` contains an incorrectly specified
`since: '2.1.1'` which needs to be updated by hand, or require additional
tooling on top to update it automatically.

Updating it by hand is tedious and may be needed to be done multiple times
before the PR is merged. It's also not error-prone, as you may make a typo
or just forget about the deprecation. This is also why Composer parses
package version from the Git tags and advocates against specifying it
manually: https://getcomposer.org/doc/04-schema.md#version

Lynn

2 years ago

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

Benjamin Eberlei

2 years ago

On Wed, Apr 24, 2024 at 2:55 PM Lynn <kjarli@gmail.com> wrote:

>
>
> 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.

Joerg Sowa

2 years ago

I like the proposition and I like the idea of $since parameter, however,
this option is too ambiguous about what should store. Should it store the
PHP version, package version, or the date?

What about setting this parameter vaguely as the boolean we can pass?

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

#[Deprecated(since: $packageVersion > 5.5)]
#[Deprecated(since: PHP_VERSION_ID > 80100)]
#[Deprecated(since: date("Y-m-d") > "2024-01-21")]

Kind regards,
Jorg

Benjamin Eberlei

2 years ago

On Wed, Apr 24, 2024 at 7:18 PM Jorg Sowa <jorg.sowa@gmail.com> wrote:

> I like the proposition and I like the idea of $since parameter, however,
> this option is too ambiguous about what should store. Should it store the
> PHP version, package version, or the date?
>
> What about setting this parameter vaguely as the boolean we can pass?
>
> #[Attribute(Attribute::TARGET_METHOD | Attribute::TARGET_FUNCTION)]
> class Deprecated
> {
>     public function __construct(
>         public readonly ?string $note = null,
>         public readonly ?bool $since = null
>     ) {
>     }
> }
>
> #[Deprecated(since: $packageVersion > 5.5)]
> #[Deprecated(since: PHP_VERSION_ID > 80100)]
> #[Deprecated(since: date("Y-m-d") > "2024-01-21")]
>

These are not valid uses of attributes, the expressions for since parameter
are illegal in the language.

PHP Fatal error:  Constant expression contains invalid operations in
/private/tmp/dep.php on line 6

And this proposal would only make the condition human readable, the machine
would see $since as either true or false, and that is not really something
that tools will find valuable enough.

But I agree on $since being less useful, users could put a date, a string,
or anyhing in there.

The way I see it vendors should add their own attributes, example:

#[\Deprecated, Since(package: "symfony/http-request", version: "7.0.2")]

or for PHPStorms replacement functinoality (potentially usable by Rector):

#[]Deprecated, Replacement('%class%->setPublic(!%parameter0%)')]

This is
a.) composable, which increases the potential use-cases for different
vendor and communities and doesn't restrict them to what core "designed in
the ivory tower once".
b.) allows vendors to have very strict validation on their attributes
arguments. I.e. Symfony could enforce package to be a valid composer
package string and version to follow composers version parser. This is
something we cannot enforce in the engine.

Rowan Tommins [IMSoP]

2 years ago

On 24 April 2024 18:18:28 BST, Jorg Sowa <jorg.sowa@gmail.com> wrote:

> What about setting this parameter vaguely as the boolean we can pass?
> ...
> #[Deprecated(since: $packageVersion > 5.5)]
> #[Deprecated(since: PHP_VERSION_ID > 80100)]
> #[Deprecated(since: date("Y-m-d") > "2024-01-21")]

Even if these expressions were legal, as far as I know, standard reflection doesn't give any access to the source code or AST of how the attribute was written, so this would just end up with a meaningless "$since = true", and some source code that might as well be a comment.

To be honest, I'm not really sure what I'd do with the information in a "since" field even if it was there. If you were running PHP 7.4, what difference would it make to know that create_function was deprecated in 7.2, rather than in 7.1 or 7.3? The two relevant facts are when the suggested replacement was introduced (in case you need to support multiple versions); and what is the soonest that the deprecated feature will be removed. The second in particular is something I would like every deprecation message to include, rather than the vague "may be removed in a future version".

I found this discussion of "since" in Rust's implementation, but don't find the arguments in favour particularly compelling: https://github.com/rust-lang/rfcs/pull/1270#issuecomment-138043714

Of interest, that discussion also linked to a related feature in Java, which could perhaps be added to a list in the RFC alongside the Rust and JetBrains ones already mentioned: https://openjdk.org/jeps/277

It's interesting to note, for instance, that both Java and Rust designers considered a specific "replacement" field, but decided that it was unlikely to be useful in practice. The Java proposal states this nicely:

> In practice, there is never a drop-in replacement API for
> any deprecated API; there are always tradeoffs and
> design considerations, or choices to be made among
> several possible replacements. All of these topics require
> discussion and are thus better suited for textual
> documentation.

The JetBrains attribute *does* include a "replacement" argument, but it's heavily tied into a specific use case: it contains a template used for code transformation in the IDE. Both it and "since" are explicitly marked "applicable only for PhpStorm stubs".

Regards,
Rowan Tommins
[IMSoP]

Stephen Reay

2 years ago

Sent from my iPhone

> On 25 Apr 2024, at 16:26, Rowan Tommins [IMSoP] <imsop.php@rwec.co.uk> wrote:
>
> ﻿On 24 April 2024 18:18:28 BST, Jorg Sowa <jorg.sowa@gmail.com> wrote:
>> What about setting this parameter vaguely as the boolean we can pass?
>> ...
>> #[Deprecated(since: $packageVersion > 5.5)]
>> #[Deprecated(since: PHP_VERSION_ID > 80100)]
>> #[Deprecated(since: date("Y-m-d") > "2024-01-21")]
>
>
> Even if these expressions were legal, as far as I know, standard reflection doesn't give any access to the source code or AST of how the attribute was written, so this would just end up with a meaningless "$since = true", and some source code that might as well be a comment.
>
> To be honest, I'm not really sure what I'd do with the information in a "since" field even if it was there. If you were running PHP 7.4, what difference would it make to know that create_function was deprecated in 7.2, rather than in 7.1 or 7.3? The two relevant facts are when the suggested replacement was introduced (in case you need to support multiple versions); and what is the soonest that the deprecated feature will be removed. The second in particular is something I would like every deprecation message to include, rather than the vague "may be removed in a future version".
>
> I found this discussion of "since" in Rust's implementation, but don't find the arguments in favour particularly compelling: https://github.com/rust-lang/rfcs/pull/1270#issuecomment-138043714
>
>
> Of interest, that discussion also linked to a related feature in Java, which could perhaps be added to a list in the RFC alongside the Rust and JetBrains ones already mentioned: https://openjdk.org/jeps/277
>
> It's interesting to note, for instance, that both Java and Rust designers considered a specific "replacement" field, but decided that it was unlikely to be useful in practice. The Java proposal states this nicely:
>
>> In practice, there is never a drop-in replacement API for
>> any deprecated API; there are always tradeoffs and
>> design considerations, or choices to be made among
>> several possible replacements. All of these topics require
>> discussion and are thus better suited for textual
>> documentation.
>
> The JetBrains attribute *does* include a "replacement" argument, but it's heavily tied into a specific use case: it contains a template used for code transformation in the IDE. Both it and "since" are explicitly marked "applicable only for PhpStorm stubs".
>
> Regards,
> Rowan Tommins
> [IMSoP]
>

I think it's worth pointing out that phpdoc has had a @deprecated [version] [description] attribute for a long time - since is, IMO a different name for "version" and indicates the version that element was deprecated. If you're on X.y and it says it was deprecated in X.w you know you don't need to worry about it being removed until at least Y.a.

Also, phpdoc *also* has a separate @since attribute, which documents the *introduction or modification* of an element.

I think suggestions of a "Since" attribute "beside" the deprecated attribute defeat the purpose of it being an attribute, on top of being confusing when compared to existing phpdoc attributes.

#[Deprecated, Since(1.9)] to me, means "currently deprecated; introduced (or significantly changed) in 1.9"

To mean what you're suggesting it would need to be
#[Deprecated, Since(1.9, 'Deprecated in favour of foo...')] which is just needless duplication.

For those who don't see the point of `since`, I think the obvious answer is: documentation. Rather than needing a separate docblock to detail the deprecation, it can be all in one, just as parameter types and return types are. This reduces duplication, and reduces possibility of mismatched values.

If you wanted it to be clearer I'd suggest maybe rename "since" to "version", but that's more to give a hint at intended use than anything.

Rowan Tommins [IMSoP]

2 years ago

On 25/04/2024 08:40, Stephen Reay wrote:

> If you're on X.y and it says it was deprecated in X.w you know you
> don't need to worry about it being removed until at least Y.a.

Yeah, that's the reasoning given in the Rust discussion, but I don't
find it convincing.

If the project's deprecation policy is that deprecations will be removed
in the next major version, the information is redundant: if you get the
deprecation message in 2.x, you know it will be removed in 3.0

If the project has some other deprecation policy, like "after 1 full
major version cycle", then you can work out that "since: 2.3" means
removal in 4.0; but the person adding the attribute also knows that, and
could save the reader some effort by writing "planned removal: 4.0"

If the project has no clear deprecation policy, the information is
useless anyway.

> If you wanted it to be clearer I'd suggest maybe rename "since" to
> "version", but that's more to give a hint at intended use than anything.

I don't think there's anything *unclear* about "since", I just don't
think it's very *useful*. But apparently it's common to write it, so I
guess I'm in the minority.

Naming it "version" would just make it less clear, and not resolve
anything from my point of view.


Regards,

--
Rowan Tommins
[IMSoP]

Mike Schinkel

2 years ago

> On Apr 25, 2024, at 11:28 AM, Rowan Tommins [IMSoP] <imsop.php@rwec.co.uk> wrote:
> If the project has no clear deprecation policy, the information is useless anyway.

Not true.

Having standardized notation for deprecation would allow tooling to analyze a codebase and determine if it contains deprecated code that needs to be remediated without having to run the code with full coverage.

Actually, `since:` doesn't go far enough because it doesn't indicate if the deprecation throws warnings, throws errors, or just no longer works.  The latter two would be especially helpful when deciding if code modifications are necessary before upgrading to a new version of PHP.

-Mike

Rowan Tommins [IMSoP]

2 years ago

On 25 April 2024 22:01:35 BST, Mike Schinkel <mike@newclarity.net> wrote:

>> On Apr 25, 2024, at 11:28 AM, Rowan Tommins [IMSoP] <imsop.php@rwec.co.uk> wrote:
>> If the project has no clear deprecation policy, the information is useless anyway.
>
>Not true.
>
>Having standardized notation for deprecation would allow tooling to analyze a codebase and determine if it contains deprecated code that needs to be remediated without having to run the code with full coverage.

I think you missed the context of that sentence - or I'm missing something in yours. I meant specifically that the "deprecated since" information is useless if there's no published policy on how long something will stay deprecated.

I think the "deprecated" attribute itself is definitely useful.

Regards,
Rowan Tommins
[IMSoP]

Mike Schinkel

2 years ago

> On Apr 25, 2024, at 5:47 PM, Rowan Tommins [IMSoP] <imsop.php@rwec.co.uk> wrote:
> I think you missed the context of that sentence - or I'm missing something in yours. I meant specifically that the "deprecated since" information is useless if there's no published policy on how long something will stay deprecated.

To paraphrase a former US President, "Depends on what the meaning of 'since' is."

Given a lack of agreed definition for 'since' it appears you are using narrow assumptions about the meaning of 'since' that led you to view 'since' as useless.

Conversely, I try not to make limiting assumptions and look for how a feature could be useful IF defined appropriately. And to me there is benefit to have a standardized way for tooling to recognize deprecation based on versions and other criteria, even if the specifics may need to be explored further.

Or at least that is how I see it.  But then, I have no vote, so ¯\_(ツ)_/¯.

-Mike

Nicolas Grekas

2 years ago

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.
- We should be able to add the attribute to a class.

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.

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.

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.

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.

Nicolas

Benjamin Eberlei

2 years ago

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.

Benjamin Eberlei

2 years ago

On Tue, Apr 23, 2024 at 3:27 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
>

We have updated the RFC and PR to include #[Deprecated] on class constants
and enum cases as this is something the engine already supports for
internal class constants.

This does not include support for non-class based constants, because they
don't have support for attributes at the moment and also only recently got
reflection support (for 8.4).

We are planning to work on adding $since next and get back to the list once
that is finished.

Benjamin Eberlei

2 years ago

On Thu, May 2, 2024 at 1:00 PM Benjamin Außenhofer <kontakt@beberlei.de>
wrote:

>
>
> On Tue, Apr 23, 2024 at 3:27 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
>>
>
> We have updated the RFC and PR to include #[Deprecated] on class constants
> and enum cases as this is something the engine already supports for
> internal class constants.
>
> This does not include support for non-class based constants, because they
> don't have support for attributes at the moment and also only recently got
> reflection support (for 8.4).
>
> We are planning to work on adding $since next and get back to the list
> once that is finished.
>

The $since implementation is now added to the RFC and to the PR.

We decided to make this a secondary voting choice, while there are
use-cases for it in php core for doc rendering, other ways of doing the
same thing are also available and given userland frameworks stated
different requirements maybe its better left for userland to implement.

Benjamin Eberlei

2 years ago

On Tue, Apr 23, 2024 at 3:27 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.
>

Feedback period is now nearing 4 weeks and the last 2 have not brought any
new significant discussions, as such we plan to open the vote next
Wednesday if no new roadblocks come up.

Thank you to all participants for the feedback.

Larry Garfield

2 years ago

On Fri, May 17, 2024, at 9:53 AM, Benjamin Außenhofer wrote:

> On Tue, Apr 23, 2024 at 3:27 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.
>
> Feedback period is now nearing 4 weeks and the last 2 have not brought
> any new significant discussions, as such we plan to open the vote next
> Wednesday if no new roadblocks come up.
>
> Thank you to all participants for the feedback.
>>
>> greetings
>> Benjamin

Why is the attribute not allowed on classes?  I think it would make sense on every language structure, unrestricted.  (Including classes, parameters, everything.)

The user-space definition of the attribute is invalid in the example: It declares properties AND promoted constructor args.  That can/should be combined.

Otherwise, this looks good to me.

--Larry Garfield

Tim Düsterhus

2 years ago

Hi

On 5/17/24 17:02, Larry Garfield wrote:

> Why is the attribute not allowed on classes?  I think it would make sense on every language structure, unrestricted.  (Including classes, parameters, everything.)

There is not pre-existing semantics of deprecating a class from the
engine PoV.

The only things that can be deprecated on the engine level as of now are
Functions, Methods, Class Constants (incl. Enum Cases), and regular
Constants (those do not support attributes).

Allowing to apply the attribute on classes is therefore left to future
scope (as mentioned in the “Future Scope” section of the RFC). Adjusting
the attribute to also apply to classes once the semantics are decided
should be straight-forward and thus be an implicit part of the process
of supporting the deprecation of classes.

> The user-space definition of the attribute is invalid in the example: It declares properties AND promoted constructor args.  That can/should be combined.

I don’t follow. Do you mean the top-most codeblock in the RFC? It does
not use constructor property promotion.

It’s the stub, intentionally omitting the method body. I’ve added a
placeholder comment in the constructor body to make that clearer.

Best regards
Tim Düsterhus

Larry Garfield

2 years ago

On Fri, May 17, 2024, at 3:49 PM, Tim Düsterhus wrote:

> Hi
>
> On 5/17/24 17:02, Larry Garfield wrote:
>> Why is the attribute not allowed on classes?  I think it would make sense on every language structure, unrestricted.  (Including classes, parameters, everything.)
>
> There is not pre-existing semantics of deprecating a class from the
> engine PoV.
>
> The only things that can be deprecated on the engine level as of now are
> Functions, Methods, Class Constants (incl. Enum Cases), and regular
> Constants (those do not support attributes).
>
> Allowing to apply the attribute on classes is therefore left to future
> scope (as mentioned in the “Future Scope” section of the RFC). Adjusting
> the attribute to also apply to classes once the semantics are decided
> should be straight-forward and thus be an implicit part of the process
> of supporting the deprecation of classes.

This would be helpful to include explicitly in the RFC, toward the top, because it's the first thing that came to mind when I saw the initial code block.

>> The user-space definition of the attribute is invalid in the example: It declares properties AND promoted constructor args.  That can/should be combined.
>
> I don’t follow. Do you mean the top-most codeblock in the RFC? It does
> not use constructor property promotion.
>
> It’s the stub, intentionally omitting the method body. I’ve added a
> placeholder comment in the constructor body to make that clearer.

Ah, I misread it.  You are correct, it's valid.  It's so rare I see a name in both the constructor and as a property these days. :-)

--Larry Garfield