Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC: Add #[\Deprecated] Attribute #11293

Merged
merged 9 commits into from
Jul 2, 2024
Merged

RFC: Add #[\Deprecated] Attribute #11293

merged 9 commits into from
Jul 2, 2024

Conversation

beberlei
Copy link
Contributor

@beberlei beberlei commented May 22, 2023 •
edited by TimWolla
Loading

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

I made a mistake rebasing the previous PR #6521 and had to create this new one. Additional work is necessary to adapt this to new APIs and to move over the discussion and suggestions.

ToDo:

  • Apply the attribute to all internal functions that are deprecated.
  • Fix Opcache support for non-methods.

iluuu1994
iluuu1994 reviewed May 23, 2023
View reviewed changes
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
@TimWolla TimWolla added the RFC label May 23, 2023
@andypost
Copy link
Contributor

andypost commented Jan 9, 2024

Please update RFC with link to this PR

@pronskiy
Copy link

pronskiy commented Apr 12, 2024 •
edited
Loading

@beberlei, is there any chance to include a string ?replacement = null parameter?

It can be used by PhpStorm, @rectorphp, and other tools in the ecosystem to simplify migration for end users.
https://twitter.com/pronskiy/status/1778716339877613816

@beberlei
Copy link
Contributor Author

@pronskiy I can add it as a secondary vote, if i ever get around to finish the RFC for it. But it would be an essentially "useless" property from the engine POV that would be only useful for IDEs, and then without a standard, prone to conflicts.

@pronskiy
Copy link

@beberlei, how about adding a generic parameter? And letting tools to decide on the standard/convention, should there be one.

#[Attribute(Attribute::TARGET_METHOD|Attribute::TARGET_FUNCTION)]
final class Deprecated
{
    public function __construct(?string $message = null, ...$extra) {}
}

#[Deprecated('Message', replacement: 'bar')]
foo();

@pronskiy
Copy link

pronskiy commented Apr 13, 2024 •
edited
Loading

Well, giving it another thought, tools could just have their own attribute #[Replacement('bar')].

#[
    Deprecated('Message'),
    Replacement('bar'),
]
foo();

@bwoebi
Copy link
Member

bwoebi commented Apr 13, 2024

@beberlei Is there a specific reason #[Deprecated] is only targeting functions?

Why not classes, so that any reference of the class, be it instantiation, or static access, emits the deprecation?
Similarly why not consts etc.?

@beberlei
Copy link
Contributor Author

beberlei commented Apr 13, 2024 •
edited
Loading

@bwoebi no, i didnt get to this part yet due to my lack of engine skills :)

@Sysix
Copy link

Sysix commented Apr 13, 2024

Hello,

As a small lib maintainer I like this idea. At the moment I use PHPDoc + trigger_error, so it would help to simplify.
One question: why E_DEPRECATED, as I understood, this error_level should be only for php core.
Would love to see a possibility with E_USER_DEPRECATED :)

@TimWolla
Copy link
Member

I've checked with @beberlei and pushed some fixes into his branch.

One question: why E_DEPRECATED, as I understood, this error_level should be only for php core.
Would love to see a possibility with E_USER_DEPRECATED :)

@Sysix This is fixed now. Thank you.

@TimWolla
Copy link
Member

Similarly why not consts etc.?

@bwoebi Constants do not (yet) support attributes.

@TimWolla TimWolla mentioned this pull request Apr 15, 2024
Closed
@TimWolla TimWolla changed the title Add #[Deprecated] attribute RFC: Add #[\Deprecated] attribute Apr 19, 2024
@TimWolla TimWolla force-pushed the DeprecatedAttribute branch 2 times, most recently from 7dcafb2 to ce49812 Compare April 19, 2024 11:39
@TimWolla TimWolla changed the title RFC: Add #[\Deprecated] attribute RFC: Add #[\Deprecated] Attribute Apr 19, 2024
iluuu1994
iluuu1994 reviewed Apr 23, 2024
View reviewed changes
Zend/zend_attributes.c Outdated Show resolved Hide resolved
Zend/zend_execute.c Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
@TimWolla
Copy link
Member

Unfortunately, I'm unable to reproduce the ARM build failures. The tests within a Debian Docker container on a aarch64 VM appear to succeed:

=====================================================================
PHP         : /pwd/sapi/cli/php 
PHP_SAPI    : cli
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux f188114febdc 5.15.0-1058-aws #64~20.04.1-Ubuntu SMP Tue Apr 9 11:11:55 UTC 2024 aarch64
INI actual  : /pwd
More .INIs  :  
---------------------------------------------------------------------
PHP         : /pwd/sapi/cgi/php-cgi 
PHP_SAPI    : cgi-fcgi
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux f188114febdc 5.15.0-1058-aws #64~20.04.1-Ubuntu SMP Tue Apr 9 11:11:55 UTC 2024 aarch64
INI actual  : /pwd
More .INIs  : 
--------------------------------------------------------------------- 
---------------------------------------------------------------------
PHP         : /pwd/sapi/phpdbg/phpdbg 
PHP_SAPI    : phpdbg
PHP_VERSION : 8.4.0-dev
ZEND_VERSION: 4.4.0-dev
PHP_OS      : Linux - Linux f188114febdc 5.15.0-1058-aws #64~20.04.1-Ubuntu SMP Tue Apr 9 11:11:55 UTC 2024 aarch64
INI actual  : /pwd
More .INIs  : 
---------------------------------------------------------------------
CWD         : /pwd
Extra dirs  : 
VALGRIND    : Not used
=====================================================================
Running selected tests.
PASS #[\Deprecated] [Zend/tests/attributes/deprecated/001.phpt] 
PASS #[\Deprecated]: Instantiating via Reflection. [Zend/tests/attributes/deprecated/002.phpt] 
PASS #[\Deprecated]: Type validation of $message parameter with int. [Zend/tests/attributes/deprecated/003.phpt] 
PASS #[\Deprecated]: Throwing error handler. [Zend/tests/attributes/deprecated/004.phpt] 
PASS #[\Deprecated]: $message property is readonly. [Zend/tests/attributes/deprecated/005.phpt] 
PASS #[\Deprecated]: Code is E_USER_DEPRECATED. [Zend/tests/attributes/deprecated/006.phpt] 
PASS #[\Deprecated]: NUL bytes in message. [Zend/tests/attributes/deprecated/007.phpt] 
PASS #[\Deprecated]: Empty message [Zend/tests/attributes/deprecated/008.phpt] 
PASS #[\Deprecated]: Type validation of $message parameter with native enum case. [Zend/tests/attributes/deprecated/009.phpt] 
PASS #[\Deprecated]: Type validation of $message parameter with array. [Zend/tests/attributes/deprecated/010.phpt] 
PASS #[\Deprecated]: Type validation of $message parameter with int and strict types. [Zend/tests/attributes/deprecated/011.phpt] 
PASS #[\Deprecated]: ReflectionFunction::isDeprecated() returns true. [Zend/tests/attributes/deprecated/012.phpt] 
PASS #[\Deprecated]: Message from constant. [Zend/tests/attributes/deprecated/013.phpt] 
=====================================================================
Number of tests :    13                13
Tests skipped   :     0 (  0.0%) --------
Tests warned    :     0 (  0.0%) (  0.0%)
Tests failed    :     0 (  0.0%) (  0.0%)
Tests passed    :    13 (100.0%) (100.0%)
---------------------------------------------------------------------
Time taken      : 0.202 seconds
=====================================================================
root@f188114febdc:/pwd# sapi/cli/php run-tests.php --show-diff -P -q -d zend_extension=$(pwd)/modules/opcache.so -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.protect_memory=1 -d opcache.jit_buffer_size=64M -d zend_test.observer.enabled=1 -d zend_test.observer.show_output=0 Zend/tests/attributes/deprecated/

@beberlei
Copy link
Contributor Author

On my ARM Mac these also pass with ./configure --disable-all --enable-opcache --enable-opcache-jit --enable-zend_test and the command from @TimWolla

@iluuu1994
Copy link
Member

@TimWolla This is likely related to the --repeat 2 flag.

@TimWolla
Copy link
Member

This is likely related to the --repeat 2 flag.

Thanks, this reproduces the issue for me on x64 Linux:

sapi/cli/php run-tests.php --show-diff -d zend_extension=(pwd)/modules/opcache.so -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.protect_memory=1 --repeat 2 Zend/tests/attributes/deprecated/

JIT is required.

TimWolla added a commit to TimWolla/php-src that referenced this pull request Apr 30, 2024
@TimWolla
Add ReflectionClassConstant::isDeprecated()
9f0b64c 
This is in preparation for php#11293 and for consistency with
ReflectionConstant::isDeprecated() that was added in php#13669.
TimWolla
TimWolla reviewed Jul 1, 2024
View reviewed changes
Zend/zend_execute.c Outdated Show resolved Hide resolved
iluuu1994
iluuu1994 reviewed Jul 1, 2024
View reviewed changes
Zend/zend_attributes.c Show resolved Hide resolved
Zend/zend_execute.c Outdated Show resolved Hide resolved
Zend/zend_execute.c Outdated Show resolved Hide resolved
Zend/zend_vm_def.h Outdated Show resolved Hide resolved
@dstogov dstogov mentioned this pull request Jul 2, 2024
Merged
@TimWolla TimWolla merged commit 72c8746 into php:master Jul 2, 2024
11 checks passed
@TimWolla
Copy link
Member

TimWolla commented Jul 2, 2024

Now merged, thank you all for the review and assistance.

The follow-up PR to apply the Attribute is here: #14750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimWolla TimWolla TimWolla left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@iluuu1994 iluuu1994 iluuu1994 left review comments

@dstogov dstogov dstogov left review comments

@kocsismate kocsismate kocsismate left review comments

@Girgias Girgias Girgias left review comments

Assignees
No one assigned
Labels
Category: Build System Category: Engine Category: Optimizer Extension: opcache Extension: reflection Extension: zend_test RFC
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.