Skip to content

[RFC] Allow #[\Deprecated] on traits #19045

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DanielEScherzer
Copy link
Member

@DanielEScherzer DanielEScherzer commented Jul 5, 2025

@TimWolla
Copy link
Member

Is this waiting for #18817?

@DanielEScherzer
Copy link
Member Author

Yes, I was planning to have #18817 merged first

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only superficial comments, looks correct to me otherwise.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly remarks about the test organization. For my attribute RFCs, I've tried to very carefully maintain the test infrastructure with regard to titles and filenames.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this test 001.phpt would be consistent with the other folders. The title line could also be unified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a more informative name - here this is just a test for the basic functionality, i.e. "basic" is appropriate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep naming and title in sync.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming is a short version of the title - what would you suggest the naming be instead?

@@ -2006,6 +2006,27 @@ ZEND_API ZEND_COLD void ZEND_FASTCALL zend_deprecated_constant(const zend_consta
zend_string_release(message_suffix);
}

ZEND_API ZEND_COLD void zend_use_of_deprecated_trait(
zend_class_entry *trait,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
zend_class_entry *trait,
const zend_class_entry *trait,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't do that without more widespread changes, trait gets passed to get_deprecation_suffix_from_attribute() which does not const its parameter, because it gets passed to zend_get_attribute_object() where the parameter isn't const, etc.

@TimWolla
Copy link
Member

TimWolla commented Aug 21, 2025

Note: I've seen the new review request, but am currently busy. I'll try to get to that early next week at the latest.

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.

3 participants