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.
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
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
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.
Le 22/05/2024 à 09:33, Nicolas Grekas a écrit :
Hi Benjamin,
The vote for the RFC #[\Deprecated] attribute is now open:
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
On Wednesday, 22 May 2024 at 08:22, Benjamin Außenhofer <kontakt@beberlei.de> wrote:
The vote for the RFC #[\Deprecated] attribute is now open:
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
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
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:
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
Hi Matthew
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
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
On 6/5/24 10:16, Benjamin Außenhofer wrote:
Tim will finalize the implementation PR now and work on its merge in the
upcoming days.
During review Ilija pointed out that adding the internal ZEND_ACC_DEPRECATED flag was skipped for abstract methods and interface methods, questioning the decision. The decision dates back to Benjamin's initial PoC implementation and I've had a short discussion with him about this, with the result that it's probably best to ask the list, as it was not specified in the RFC.
Currently adding the #[\Deprecated] attribute on an abstract method or interface method does not do anything special. It only applies the attribute, which then can be accessed via Reflection.
It would be easy to change the implementation to also apply the ZEND_ACC_DEPRECATED flag. This would have the following consequences:
1. `ReflectionMethod::isDeprecated()` will return `true`.
2. No deprecation error will be emitted. Neither when implementing the method in a child class, nor when calling the method of the child class.
(1) is probably the obviously correct behavior. For (2) the reasoning is as follows:
- The implementation relies on the pre-existing behavior of ZEND_ACC_DEPRECATED, which does not emit a deprecation error for overridden methods / implemented abstract or interface methods.
- If the deprecation error would be emitted when implementing the method, then the error would not be actionable. Child classes are still forced to implement the abstract method / interface method, thus there is no way to change the code to not emit the error.
- It is not obviously correct to inherit the deprecation when overriding a non-abstract method of a parent class, just like attributes themselves are not inherited [1]. Abstract methods and interface methods should keep consistency with non-abstract methods. The reasoning why it's not obviously correct to inherit the deprecation is that due to not inheriting the Attribute, a custom deprecation message would not be applied.
It goes without saying that IDEs can opt to suggest adding the #[\Deprecated] attribute if the overridden method is deprecated itself by means of the existing code diagnosis functionality, making the attribute on abstract methods / interface methods useful by that means.
We plan to adjust the implementation to also apply to abstract and interface methods with the semantics outlined in (1) and (2). Does anyone have an opinion about this / does anyone disagree with that decision?
Best regards
Tim Düsterhus
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
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
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?
···
Matthew Weier O’Phinney
mweierophinney@gmail.com
https://mwop.net/
he/him
On Mon, Jun 24, 2024, 7:35 PM Stephen Reay <php-lists@koalephant.com> wrote:
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.
Hi
On 6/24/24 17:06, Nicolas Grekas wrote:
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: RFC: Add `#[\Deprecated]` Attribute by beberlei · Pull Request #11293 · php/php-src · GitHub
Best regards
Tim Düsterhus
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
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
Hi
On 6/27/24 13:10, Nicolas Grekas wrote:
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