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.

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

RFC: Add #[\Deprecated] Attribute #11293

Merged
merged 9 commits into from
Jul 2, 2024
Merged

Conversation

beberlei
Copy link
Contributor

@beberlei beberlei commented May 22, 2023

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.

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

@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

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

@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 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
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
This is in preparation for php#11293 and for consistency with
ReflectionConstant::isDeprecated() that was added in php#13669.
Zend/zend_execute.c Outdated Show resolved Hide resolved
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
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet