[RFC] [Vote] #[\Deprecated] attribute

1 year ago by Nicolas Grekas — view source

unread

Hi Benjamin,

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

I voted "no" because I think this is better addressed in userland, as this
gives more flexibility.
I would better have an attribute that is made only for static analysis with
no runtime side effects at all.
Being forced to make the attribute final because the implementation in the
engine requires is an example of why the engine is not the correct place to
send this notice. Another example is not being able to add the attribute on
classes because [engine reasons].

trigger_error() is better fitted for the runtime side-effect when it's
desired.
In my opinion, #[Deprecated] should be only for static analysers /
reflection (although this would need another discussion - I'm not sure this
would benefit being in the engine vs in a userland package.

Thanks for the RFC anyway.

Nicolas

1 year ago by Pierre — view source

unread

Le 22/05/2024 à 09:33, Nicolas Grekas a écrit :

Hi Benjamin,
The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.
I voted "no" because I think this is better addressed in userland, as
this gives more flexibility.
I would better have an attribute that is made only for static analysis
with no runtime side effects at all.
Being forced to make the attribute final because the implementation in
the engine requires is an example of why the engine is not the correct
place to send this notice. Another example is not being able to add
the attribute on classes because [engine reasons].

trigger_error() is better fitted for the runtime side-effect when it's
desired.
In my opinion, #[Deprecated] should be only for static analysers /
reflection (although this would need another discussion - I'm not sure
this would benefit being in the engine vs in a userland package.

Thanks for the RFC anyway.

Nicolas

I'm always dubious when I read the "this can be addressed or better
addressed in userland": as soon as you need something as little as the
Deprecated attribute, if it's not in core but in userland, you encounter
two very serious issues: first one, fragmentation, every framework,
organisation, or static analysis tool will have its own variant, second
one if the community converges toward a single one, you then will be
dependent on a certain framework or huge tooling.

I can see many benefits in having this in core, one being that every
static analyzer will implement it using the core attribute, so any
project and developer using it can choose any static analysis tooling
without having to change or adapt its code everywhere, or learning a
different convention in order to use different tools.

That's my 2 cents, but the "better in userland" argument seems to me
almost always biased or wrong. Especially for this Deprecated argument,
this a very common need, and each project Doctrine, Symfony, any tool
implement it in a different manner. I don't want to be rude, but as a
developer using all of those, it's very annoying having to learn
different ways of doing something as simple and naive that saying that a
function is deprecated. One manner is sufficient, even if not perfect.

--

Pierre

1 year ago by Arvids Godjuks — view source

unread

Le 22/05/2024 à 09:33, Nicolas Grekas a écrit :

Hi Benjamin,

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

I voted "no" because I think this is better addressed in userland, as this
gives more flexibility.
I would better have an attribute that is made only for static analysis
with no runtime side effects at all.
Being forced to make the attribute final because the implementation in the
engine requires is an example of why the engine is not the correct place to
send this notice. Another example is not being able to add the attribute on
classes because [engine reasons].

trigger_error() is better fitted for the runtime side-effect when it's
desired.
In my opinion, #[Deprecated] should be only for static analysers /
reflection (although this would need another discussion - I'm not sure this
would benefit being in the engine vs in a userland package.

Thanks for the RFC anyway.

Nicolas

I'm always dubious when I read the "this can be addressed or better
addressed in userland": as soon as you need something as little as the
Deprecated attribute, if it's not in core but in userland, you encounter
two very serious issues: first one, fragmentation, every framework,
organisation, or static analysis tool will have its own variant, second one
if the community converges toward a single one, you then will be dependent
on a certain framework or huge tooling.

I can see many benefits in having this in core, one being that every
static analyzer will implement it using the core attribute, so any project
and developer using it can choose any static analysis tooling without
having to change or adapt its code everywhere, or learning a different
convention in order to use different tools.

That's my 2 cents, but the "better in userland" argument seems to me
almost always biased or wrong. Especially for this Deprecated argument,
this a very common need, and each project Doctrine, Symfony, any tool
implement it in a different manner. I don't want to be rude, but as a
developer using all of those, it's very annoying having to learn different
ways of doing something as simple and naive that saying that a function is
deprecated. One manner is sufficient, even if not perfect.

--

Pierre

Hello everyone.

I have to agree with Pierre on this one as a userland developer.
To give a good illustration: Guzzle. I'm trying to get rid of it in my code
base and use SF HttpClient (mostly for the sole reason that guzzle
truncates replies in their exceptions without any ability to configure that
and cutting off all actually useful information about why request failed)
and... I can't... Because some other package is hardwired to Guzzle and
refuses any attempts to inject a different HttpClient. Which forces me to
roll my own implementations, thankfully making API clients is fairly simple.
A Deprecated attribute belongs in the core.

--

Arvīds Godjuks
+371 26 851 664
arvids.godjuks@gmail.com
Telegram: @psihius https://t.me/psihius

1 year ago by kontakt@beberlei.de — view source

unread

On Wed, May 22, 2024 at 9:33 AM Nicolas Grekas nicolas.grekas+php@gmail.com
wrote:

Hi Benjamin,

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

I voted "no" because I think this is better addressed in userland, as this
gives more flexibility.
I would better have an attribute that is made only for static analysis
with no runtime side effects at all.
Being forced to make the attribute final because the implementation in the
engine requires is an example of why the engine is not the correct place to
send this notice. Another example is not being able to add the attribute on
classes because [engine reasons].

This is a misrepresentation of what this RFC is. There are no engine
reasons for not being able to set it on classes. It just has not been
implemented yet.

Allowing deprecations on classes is something that needs to be figured out
in a separate RFC in how these semantics work. Then adding Deprecated
attribute would just be a trivial matter.

You could argue about the order of things, we could have first made an RFC
for deprecated classes, but we should avoid a waterfall process of
dependencies in things to do in the language.

trigger_error() is better fitted for the runtime side-effect when it's
desired.
In my opinion, #[Deprecated] should be only for static analysers /
reflection (although this would need another discussion - I'm not sure this
would benefit being in the engine vs in a userland package.

This is the same as above, changing how deprecations work in the engine is
orthogonal to this attribute. These attributes are concerned with tagging
code elements with flags that the engine already provides, already makes
available for internal code elements.

Thanks for the RFC anyway.

Nicolas

1 year ago by Marco Pivetta — view source

unread

Hey Benjamin,

I voted against it:

I very much like having reflection methods that work on internal symbols
relying on #[\Deprecated]. I can rely on these in my static analysis
tooling, test suites, decorators, dependency injection tools, etc.
I am still disagree with abusing global runtime effects (E_DEPRECATED /
E_USER_DEPRECATED) to signal deprecations. Insert "PTSD Chihuaua" meme
here regarding PHP and library upgrades I had to work on over the last
decade. No: don't want. Don't add side-effects to otherwise functionally
pure code.

Marco Pivetta

https://mastodon.social/@ocramius

https://ocramius.github.io/

On Wed, 22 May 2024 at 09:23, Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

1 year ago by Gina P. Banyard — view source

unread

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

I have voted in favour of the RFC, I don't have any strong opinions about the "since" parameter, so I will let other people judge on this matter.
However, any discussion about how deprecations are currently being triggered is, IMHO, irrelevant to this discussion.
PHP, at this point in time, triggers a deprecation diagnostic via the same mechanism that notices, warnings, etc. diagnostics are triggered.
There might be value to separate the deprecation diagnostic mechanism from the other diagnostics to have them be globally toggled on/off and sent logs into a different file than the other diagnostics,
but like said by others, this is an orthogonal issue to this one, and one that has long existed in the language.

Every other internal attribute that has been added to PHP has some visible effects to the user, having it do nothing seems rather odd to me.

Best regards,
Gina P. Banyard

1 year ago by Matthew Weier O'Phinney — view source

unread

On Wed, May 22, 2024 at 2:24 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

I have voted no for a few reasons:

Ideally, I'd like to be able to mark anything as deprecated. In
particular, not being able to mark a class/interface/enum/etc as
deprecated makes this far less useful.
The "since" parameter is basically worthless to me. It's very easy to
find out the last version that wasn't deprecated. What would be far more
useful to a consumer is an argument indicating when something will be
removed (e.g. $toRemoveInVersion, $versionForRemoval, etc.). This helps me
as a user plan for the future.

--
Matthew Weier O'Phinney
mweierophinney@gmail.com
https://mwop.net/
he/him

1 year ago by Ilija Tovilo — view source

unread

Hi Matthew

On Mon, Jun 3, 2024 at 3:15 PM Matthew Weier O'Phinney
mweierophinney@gmail.com wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

I have voted no for a few reasons:

Ideally, I'd like to be able to mark anything as deprecated. In particular, not being able to mark a class/interface/enum/etc as deprecated makes this far less useful.

While it's true that extending #[Deprecated] to classes would be
useful, deprecation already exists as a language concept, and it can
be extended to class-like structures without a BC break.

The "since" parameter is basically worthless to me. It's very easy to find out the last version that wasn't deprecated. What would be far more useful to a consumer is an argument indicating when something will be removed (e.g. $toRemoveInVersion, $versionForRemoval, etc.). This helps me as a user plan for the future.

Did you vote yes in the secondary vote by accident? I voted no on the
$since parameter for the same reason:

"How long have I not fixed this?" is not a particularly useful
question to ask. "When do I have to fix this?" is more relevant.
The format of $since is intentionally left unstandardized, and it's
unclear (to me?) what it refers to. For example, some packages are
split into multiple, smaller ones (e.g. Doctrine) with diverging
version numbers. The sub-package version number may not be useful to
the end-user, who never requires it directly. Similarly, referencing
the main package version may be confusing, especially if the ranges of
recent main and sub-package versions overlap.

Ilija

The "since" parameter is basically worthless to me. It's very easy to find out the last version that wasn't deprecated. What would be far more useful to a consumer is an argument indicating when something will be removed (e.g. $toRemoveInVersion, $versionForRemoval, etc.). This helps me as a user plan for the future.

--
Matthew Weier O'Phinney
mweierophinney@gmail.com
https://mwop.net/
he/him

1 year ago by Matthew Weier O'Phinney — view source

unread

Hi Matthew

On Mon, Jun 3, 2024 at 3:15 PM Matthew Weier O'Phinney
mweierophinney@gmail.com wrote:

On Wed, May 22, 2024 at 2:24 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

I have voted no for a few reasons:

Ideally, I'd like to be able to mark anything as deprecated. In
particular, not being able to mark a class/interface/enum/etc as
deprecated makes this far less useful.

While it's true that extending #[Deprecated] to classes would be
useful, deprecation already exists as a language concept, and it can
be extended to class-like structures without a BC break.

Right, but I'd rather have the ability to have it now. It's far less
often the case that I'm deprecating a single method or constant, and more
often the case that I'm deprecating a class or interface. Not having
support for those immediately makes the feature "meh" for me.

The "since" parameter is basically worthless to me. It's very easy to
find out the last version that wasn't deprecated. What would be far more
useful to a consumer is an argument indicating when something will be
removed (e.g. $toRemoveInVersion, $versionForRemoval, etc.). This helps me
as a user plan for the future.

Did you vote yes in the secondary vote by accident? I voted no on the
$since parameter for the same reason:

"How long have I not fixed this?" is not a particularly useful
question to ask. "When do I have to fix this?" is more relevant.

The format of $since is intentionally left unstandardized, and it's
unclear (to me?) what it refers to. For example, some packages are
split into multiple, smaller ones (e.g. Doctrine) with diverging
version numbers. The sub-package version number may not be useful to
the end-user, who never requires it directly. Similarly, referencing
the main package version may be confusing, especially if the ranges of
recent main and sub-package versions overlap.

I voted yes on it because it's not worthless, and if the RFC passes, I'd
rather have it than not have it. But I agree that the lack of a standard
format for it, and the lack of a parallel "will remove by" argument make it
far less useful.

--
Matthew Weier O'Phinney
mweierophinney@gmail.com
https://mwop.net/
he/him

1 year ago by kontakt@beberlei.de — view source

unread

On Wed, May 22, 2024 at 9:22 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

1 year ago by tim@bastelstu.be — view source

unread

Hi

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

During review Ilija pointed out that adding the internal
ZEND_ACC_DEPRECATED flag was skipped for abstract methods and interface
methods, questioning the decision. The decision dates back to Benjamin's
initial PoC implementation and I've had a short discussion with him
about this, with the result that it's probably best to ask the list, as
it was not specified in the RFC.

Currently adding the #[\Deprecated] attribute on an abstract method or
interface method does not do anything special. It only applies the
attribute, which then can be accessed via Reflection.

It would be easy to change the implementation to also apply the
ZEND_ACC_DEPRECATED flag. This would have the following consequences:

ReflectionMethod::isDeprecated() will return true.
No deprecation error will be emitted. Neither when implementing the
method in a child class, nor when calling the method of the child class.

(1) is probably the obviously correct behavior. For (2) the reasoning is
as follows:

The implementation relies on the pre-existing behavior of
ZEND_ACC_DEPRECATED, which does not emit a deprecation error for
overridden methods / implemented abstract or interface methods.
If the deprecation error would be emitted when implementing the
method, then the error would not be actionable. Child classes are still
forced to implement the abstract method / interface method, thus there
is no way to change the code to not emit the error.
It is not obviously correct to inherit the deprecation when overriding
a non-abstract method of a parent class, just like attributes themselves
are not inherited [1]. Abstract methods and interface methods should
keep consistency with non-abstract methods. The reasoning why it's not
obviously correct to inherit the deprecation is that due to not
inheriting the Attribute, a custom deprecation message would not be applied.

It goes without saying that IDEs can opt to suggest adding the
#[\Deprecated] attribute if the overridden method is deprecated itself
by means of the existing code diagnosis functionality, making the
attribute on abstract methods / interface methods useful by that means.

We plan to adjust the implementation to also apply to abstract and
interface methods with the semantics outlined in (1) and (2). Does
anyone have an opinion about this / does anyone disagree with that decision?

Best regards
Tim Düsterhus

[1] https://3v4l.org/GIZRj

1 year ago by Nicolas Grekas — view source

unread

Le mer. 5 juin 2024 à 10:18, Benjamin Außenhofer kontakt@beberlei.de a
écrit :

On Wed, May 22, 2024 at 9:22 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Hi Benjamin,

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Since the vote passed, we're discussing how we might use the attribute in
Symfony.
2 things on the topic:

1/ We're wondering about using it at the class level despite the missing
Attribute::TARGET_CLASS. ReflectionAttribute does allow reading attributes
without checking these flags and we might leverage this capability to make
our DebugClassLoader able to inspect those at the class level.

Would you consider adding Attribute::TARGET_CLASS in 8.4 already? It would
just make sense to me. We don't need the engine to do anything about
deprecated classes ever since all we need in userland is a class-loader
that checks for the attribute. Keeping the engine simpler and empowering
userland with maximum flexiblity FTW. I'm up for a quick RFC if the
consensus is this needs one.

2/ About the "since" parameter, we're going to standardize around a
package+version string, e.g.
#[Deprecated(since: "symfony/yaml v7.2"]

I'm sharing this hoping this can spread outside of Symfony's boundary
because I think this would be a very useful practice that'd allow building
nice reporting tools.

Nicolas

1 year ago by Matthew Weier O'Phinney — view source

unread

On Mon, Jun 24, 2024 at 10:08 AM Nicolas Grekas <
nicolas.grekas+php@gmail.com> wrote:

Le mer. 5 juin 2024 à 10:18, Benjamin Außenhofer kontakt@beberlei.de a
écrit :

On Wed, May 22, 2024 at 9:22 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Hi Benjamin,

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Since the vote passed, we're discussing how we might use the attribute in
Symfony.
2 things on the topic:

1/ We're wondering about using it at the class level despite the missing
Attribute::TARGET_CLASS. ReflectionAttribute does allow reading
attributes without checking these flags and we might leverage this
capability to make our DebugClassLoader able to inspect those at the class
level.

Would you consider adding Attribute::TARGET_CLASS in 8.4 already? It
would just make sense to me. We don't need the engine to do anything about
deprecated classes ever since all we need in userland is a class-loader
that checks for the attribute. Keeping the engine simpler and empowering
userland with maximum flexiblity FTW. I'm up for a quick RFC if the
consensus is this needs one.

2/ About the "since" parameter, we're going to standardize around a
package+version string, e.g.
#[Deprecated(since: "symfony/yaml v7.2"]

I'm sharing this hoping this can spread outside of Symfony's boundary
because I think this would be a very useful practice that'd allow building
nice reporting tools.

Personally, I'd prefer the "since" format to mimic the notation that
Composer uses on the CLI when specifying a package with a constraint:
"symfony/yaml:7.2.0". This can be parsed easily, and won't suffer from
having arbitrary spacing and version naming prefixes.

(Still would prefer a "scheduledRemoval" field, as knowing when it was
deprecated is far less interesting than knowing when it will be removed.
Yes, I can assume the next major version, but what if it's major version +
1? What if a project allows removals during minor releases? Knowing what
version it will be removed in makes it far easier to understand what will
happen when I upgrade next.)

--
Matthew Weier O'Phinney
mweierophinney@gmail.com
https://mwop.net/
he/him

1 year ago by Stephen Reay — view source

unread

1 year ago by Matthew Weier O'Phinney — view source

unread

Sent from my iPhone

On 24 Jun 2024, at 23:43, Matthew Weier O'Phinney <
mweierophinney@gmail.com> wrote:

On Mon, Jun 24, 2024 at 10:08 AM Nicolas Grekas <
nicolas.grekas+php@gmail.com> wrote:

Le mer. 5 juin 2024 à 10:18, Benjamin Außenhofer kontakt@beberlei.de a
écrit :

On Wed, May 22, 2024 at 9:22 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Hi Benjamin,

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Since the vote passed, we're discussing how we might use the attribute in
Symfony.
2 things on the topic:

1/ We're wondering about using it at the class level despite the missing
Attribute::TARGET_CLASS. ReflectionAttribute does allow reading
attributes without checking these flags and we might leverage this
capability to make our DebugClassLoader able to inspect those at the class
level.

Would you consider adding Attribute::TARGET_CLASS in 8.4 already? It
would just make sense to me. We don't need the engine to do anything about
deprecated classes ever since all we need in userland is a class-loader
that checks for the attribute. Keeping the engine simpler and empowering
userland with maximum flexiblity FTW. I'm up for a quick RFC if the
consensus is this needs one.

2/ About the "since" parameter, we're going to standardize around a
package+version string, e.g.
#[Deprecated(since: "symfony/yaml v7.2"]

I'm sharing this hoping this can spread outside of Symfony's boundary
because I think this would be a very useful practice that'd allow building
nice reporting tools.

Personally, I'd prefer the "since" format to mimic the notation that
Composer uses on the CLI when specifying a package with a constraint:
"symfony/yaml:7.2.0". This can be parsed easily, and won't suffer from
having arbitrary spacing and version naming prefixes.

(Still would prefer a "scheduledRemoval" field, as knowing when it was
deprecated is far less interesting than knowing when it will be removed.
Yes, I can assume the next major version, but what if it's major version +
1? What if a project allows removals during minor releases? Knowing what
version it will be removed in makes it far easier to understand what will
happen when I upgrade next.)

If you're marking a piece of code in a project as deprecated, why does
the deprecation notice need to re-specify the package the code is in?
Wouldn't a regular version string be sufficient?

Because the consumer of the code might not know what package supplied it,
particularly if multiple packages share a namespace.

1 year ago by tim@bastelstu.be — view source

unread

Hi

Since the vote passed, we're discussing how we might use the attribute in
Symfony.
2 things on the topic:

1/ We're wondering about using it at the class level despite the missing
Attribute::TARGET_CLASS. ReflectionAttribute does allow reading attributes
without checking these flags and we might leverage this capability to make
our DebugClassLoader able to inspect those at the class level.

Would you consider adding Attribute::TARGET_CLASS in 8.4 already? It would
just make sense to me. We don't need the engine to do anything about
deprecated classes ever since all we need in userland is a class-loader
that checks for the attribute. Keeping the engine simpler and empowering
userland with maximum flexiblity FTW. I'm up for a quick RFC if the
consensus is this needs one.

I didn't consult with Benjamin and thus I'm not speaking for him, but
for me this definitely requires an additional RFC:

A main point of our RFC was to make the existing deprecation
functionality available to userland. There is no existing deprecation
functionality for classes (e.g. no ReflectionClass::isDeprecated()) and
thus supporting the attribute on classes was out of scope / left for
future scope.

Personally I would against such an RFC, because it adds to the
inconsistency of the language by not emitting a deprecation warning.

2/ About the "since" parameter, we're going to standardize around a
package+version string, e.g.
#[Deprecated(since: "symfony/yaml v7.2"]

I'm sharing this hoping this can spread outside of Symfony's boundary
because I think this would be a very useful practice that'd allow building
nice reporting tools.

You might be interested in this thread on the implementation PR then:
https://github.com/php/php-src/pull/11293#discussion_r1647339607

Best regards
Tim Düsterhus

1 year ago by kontakt@beberlei.de — view source

unread

On Mon, Jun 24, 2024 at 5:07 PM Nicolas Grekas nicolas.grekas+php@gmail.com
wrote:

Le mer. 5 juin 2024 à 10:18, Benjamin Außenhofer kontakt@beberlei.de a
écrit :

On Wed, May 22, 2024 at 9:22 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Hi Benjamin,

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Since the vote passed, we're discussing how we might use the attribute in
Symfony.
2 things on the topic:

1/ We're wondering about using it at the class level despite the missing
Attribute::TARGET_CLASS. ReflectionAttribute does allow reading
attributes without checking these flags and we might leverage this
capability to make our DebugClassLoader able to inspect those at the class
level.

Would you consider adding Attribute::TARGET_CLASS in 8.4 already? It
would just make sense to me. We don't need the engine to do anything about
deprecated classes ever since all we need in userland is a class-loader
that checks for the attribute. Keeping the engine simpler and empowering
userland with maximum flexiblity FTW. I'm up for a quick RFC if the
consensus is this needs one.

We have started loosly thinking about the behavior of
Attribute::TARGET_CLASS for a next PHP RFC already, and allowing this
before the behavior is introduced seems like a bad precedent to me, so I
agree with Tim that we probably shouldn't do that.

2/ About the "since" parameter, we're going to standardize around a
package+version string, e.g.
#[Deprecated(since: "symfony/yaml v7.2"]

I'm sharing this hoping this can spread outside of Symfony's boundary
because I think this would be a very useful practice that'd allow building
nice reporting tools.

Nicolas

1 year ago by Nicolas Grekas — view source

unread

Le mar. 25 juin 2024 à 22:01, Benjamin Außenhofer kontakt@beberlei.de a
écrit :

On Mon, Jun 24, 2024 at 5:07 PM Nicolas Grekas <
nicolas.grekas+php@gmail.com> wrote:

Le mer. 5 juin 2024 à 10:18, Benjamin Außenhofer kontakt@beberlei.de a
écrit :

On Wed, May 22, 2024 at 9:22 AM Benjamin Außenhofer kontakt@beberlei.de
wrote:

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Hi Benjamin,

The vote for the RFC #[\Deprecated] attribute is now open:

https://wiki.php.net/rfc/deprecated_attribute

Voting will close on Wednesday 5th June, 08:00 GMT.

The #[\Deprecated] attribute has been accepted with 23 (Yes) to 6 (No)
votes, 79%.

The secondary vote to include Deprecated::$since has also been accepted
with 22 (Yes) to 1 (No) votes, 96%.

Thank you to everyone for voting!

Tim will finalize the implementation PR now and work on its merge in the
upcoming days.

Since the vote passed, we're discussing how we might use the attribute in
Symfony.
2 things on the topic:

1/ We're wondering about using it at the class level despite the missing
Attribute::TARGET_CLASS. ReflectionAttribute does allow reading
attributes without checking these flags and we might leverage this
capability to make our DebugClassLoader able to inspect those at the class
level.

Would you consider adding Attribute::TARGET_CLASS in 8.4 already? It
would just make sense to me. We don't need the engine to do anything about
deprecated classes ever since all we need in userland is a class-loader
that checks for the attribute. Keeping the engine simpler and empowering
userland with maximum flexiblity FTW. I'm up for a quick RFC if the
consensus is this needs one.

We have started loosly thinking about the behavior of
Attribute::TARGET_CLASS for a next PHP RFC already, and allowing this
before the behavior is introduced seems like a bad precedent to me, so I
agree with Tim that we probably shouldn't do that.

That's sad news, because I keep explaining why engine-triggered runtime
notices are a terrible idea, yet you're planning to add more of them. The
consistency argument Tim wrote in another email isn't sound to me:
consistency with a bad idea doesn't make a good idea.

In the hope it may be used as food for thoughts: when we use @deprecated
on a class, Symfony's DebugClassLoader throws a deprecation ONLY when a
class extends one of those @deprecated classes.
For the runtime notice itself, we decide case by case among two
possibilities: either triggering the notice just before the class
declaration, or in the constructor. The reason is that sometimes we have to
load the class but we don't want to trigger the deprecation because loading
the class doesn't trigger any side-effects that users should be warned
about in relation to the deprecation. In such situations, we trigger in the
constructor.

About Attribute::TARGET_CLASS itself, not adding it right now will lead to
a situation where userland will have a hard time writing code that's
compatible with both 8.4 and 8.5 (assuming 8.5 is the moment where the flag
is added): using #[Deprecated] will be "illegal" when running on 8.4, yet
legal when running in 8.5?

That's another reason to allow the flag right away: smooth upgrades.

My suggestion is quite restricted, and that'd solve all my concerns: it is
to enable the flag right now, and to add ReflectionClass::isDeprecated()
right now also if you want. But don't plan anything more on the topic,
except maybe a deprecation notice when extending a #[Deprecated] class.
But nothing more please. No runtime notice when loading the class.

Nicolas

1 year ago by tim@bastelstu.be — view source

unread

Hi

That's sad news, because I keep explaining why engine-triggered runtime
notices are a terrible idea, yet you're planning to add more of them. The
consistency argument Tim wrote in another email isn't sound to me:
consistency with a bad idea doesn't make a good idea.

I've yet to hear a better solution for deprecation handling that does
not involve third-party tooling. Being able to deprecate functionality
is a core part of being able to evolve a programming language and just a
note in the documentation no one reads anyways does not suffice.

In the hope it may be used as food for thoughts: when we use @deprecated
on a class, Symfony's DebugClassLoader throws a deprecation ONLY when a
class extends one of those @deprecated classes.

Thank you. I'll keep that in mind if/when we'll actually start working
on an RFC adding support for deprecating classes.

For the runtime notice itself, we decide case by case among two
possibilities: either triggering the notice just before the class
declaration, or in the constructor. The reason is that sometimes we have to
load the class but we don't want to trigger the deprecation because loading
the class doesn't trigger any side-effects that users should be warned
about in relation to the deprecation. In such situations, we trigger in the
constructor.

I agree that just loading or defining a class should not emit a
deprecation warning, just like loading or defining a function does not
emit a deprecation warning.

About Attribute::TARGET_CLASS itself, not adding it right now will lead to
a situation where userland will have a hard time writing code that's
compatible with both 8.4 and 8.5 (assuming 8.5 is the moment where the flag
is added): using #[Deprecated] will be "illegal" when running on 8.4, yet
legal when running in 8.5?

That's another reason to allow the flag right away: smooth upgrades.

That is a fair concern.

I could imagine backporting the Attribute::TARGET_CLASS to all supported
version if/when support for deprecating classes is actually added to the
language and semantics are clear and considering that to be effectively
a bugfix then.

My suggestion is quite restricted, and that'd solve all my concerns: it is
to enable the flag right now, and to add ReflectionClass::isDeprecated()
right now also if you want. But don't plan anything more on the topic,
except maybe a deprecation notice when extending a #[Deprecated] class.
But nothing more please. No runtime notice when loading the class.

The PR implementing the #[\Deprecated] attribute will implement exactly
the semantics that were decided in the RFC. The implementation is
already be complete and is currently pending JIT review by Dmitry.
Assuming no change requests during the review I'll make a final cleanup
pass and then it should be ready to merge.

Anything that is not in the RFC will (need to) be part of a separate PR.

Best regards
Tim Düsterhus

1 year ago by tim@bastelstu.be — view source

unread

Hi

I'm specifically replying to Nicolas, but this email is directed to the
general public, so please feel free to reply even if you are not called
Nicolas :-)

2/ About the "since" parameter, we're going to standardize around a
package+version string, e.g.
#[Deprecated(since: "symfony/yaml v7.2"]

I'm sharing this hoping this can spread outside of Symfony's boundary
because I think this would be a very useful practice that'd allow building
nice reporting tools.

I have merged the follow-up PR to apply the attribute earlier today:

https://github.com/php/php-src/commit/29f98e748568ebd66aaae061c0dcefbba92ca058

For now I've filled in the $since attribute with a bare version
number, since this most closely matches what was described in the RFC.

I'm happy to do another follow-up on the format of the $since property
for internal functions if we can reach a simple consensus here on the
list. I think it should also be acceptable to do some basic processing
of the $since property for the display in the human readable error
message, making it easier to consume it programmatically without looking
ugly to the human reader.

For PHP internal attributes the obvious choice would be:

 #[\Deprecated(since: 'PHP 8.4')]
 function foo() { }

which would result in an error message similar to:

 Deprecated: Function foo() is deprecated since PHP 8.4

The space probably is easy enough to understand for programmatic
consumption in frameworks / error tracking libraries and the 'PHP'
matches how the PHP requirements are defined in composer dependencies
(once lowercased). I don't like the 'v' prefix for the version number,
because to the best of my knowledge nothing else uses that (except for
git tags).

You can find the discussion about the $since property in the first PR
at https://github.com/php/php-src/pull/11293#discussion_r1647339607, if
you want additional context.

Best regards
Tim Düsterhus