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

8 months ago by Levi Morrison — view source

unread

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

8 months ago by kontakt@beberlei.de — view source

unread

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.

8 months ago by Larry Garfield — view source

unread

On Tue, Apr 23, 2024 at 7:27 PM Levi Morrison
levi.morrison@datadoghq.com 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":
#[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

8 months ago by kontakt@beberlei.de — view source

unread

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.

7 months ago by Alex Wells — view source

unread

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

8 months ago by Lynn — view source

unread

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

8 months ago by kontakt@beberlei.de — view source

unread

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.

8 months ago by Jorg Sowa — view source

unread

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

8 months ago by kontakt@beberlei.de — view source

unread

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.

Kind regards,
Jorg

8 months ago by Rowan Tommins [IMSoP] — view source

unread

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]

8 months ago by Stephen Reay — view source

unread

Sent from my iPhone

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.

8 months ago by Rowan Tommins [IMSoP] — view source

unread

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]

8 months ago by Mike Schinkel — view source

unread

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

8 months ago by Rowan Tommins [IMSoP] — view source

unread

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]

8 months ago by Mike Schinkel — view source

unread

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

8 months ago by Rowan Tommins [IMSoP] — view source

unread

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.

I can't see any ambiguity in the definition: "This function has been deprecated since version 7.2" seems a straightforward English sentence, meaning that before 7.2 it wasn't deprecated, and from that version onward it is.

If there's some alternative reading of it, it's not that I'm assuming it doesn't apply, it's that I'm completely unaware of what it might be.

Regards,
Rowan Tommins
[IMSoP]

8 months ago by Stephen Reay — view source

unread

Sent from my iPhone

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.

I can't see any ambiguity in the definition: "This function has been deprecated since version 7.2" seems a straightforward English sentence, meaning that before 7.2 it wasn't deprecated, and from that version onward it is.

If there's some alternative reading of it, it's not that I'm assuming it doesn't apply, it's that I'm completely unaware of what it might be.

Regards,
Rowan Tommins
[IMSoP]

I'd even go so far as to suggest that "since" is so commonly understood that we don't need the "This function has been " and "version " parts, and could simply write: #[Deprecated(since: '7.2')] and have it be understood by humans and tooling alike.

8 months ago by Nicolas Grekas — view source

unread

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

8 months ago by kontakt@beberlei.de — view source

unread

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

7 months ago by kontakt@beberlei.de — view source

unread

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.

7 months ago by kontakt@beberlei.de — view source

unread

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.

7 months ago by kontakt@beberlei.de — view source

unread

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

7 months ago by Larry Garfield — view source

unread

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

7 months ago by tim@bastelstu.be — view source

unread

Hi

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

7 months ago by Larry Garfield — view source

unread

Hi

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