Skip to content

[RFC] Allow #[\Override] on class constants#20478

Merged
DanielEScherzer merged 9 commits into
php:masterfrom
DanielEScherzer:override-constants
Jul 3, 2026
Merged

[RFC] Allow #[\Override] on class constants#20478
DanielEScherzer merged 9 commits into
php:masterfrom
DanielEScherzer:override-constants

Conversation

@DanielEScherzer

@DanielEScherzer DanielEScherzer commented Nov 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

@DanielEScherzer

DanielEScherzer commented Nov 14, 2025

Copy link
Copy Markdown
Member Author

Someone at MergePHP suggested this today, I'll write up the RFC when I have a chance

@TimWolla TimWolla requested a review from iluuu1994 May 24, 2026 10:22
TimWolla
TimWolla reviewed Jun 29, 2026
View reviewed changes

@TimWolla TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks correct, except for test remark. But please have someone else double-check the engine implementation.

Comment thread Zend/tests/attributes/override/constants/basic.phpt
Comment on lines +29 to +31
#[\Override]
public const C1 = 'C1';
public const C2 = 'C2';

@TimWolla TimWolla Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

C2 is not tested at all. The original Zend/tests/attributes/override/001.phpt test specifically had 4 cases to test all situations:

  • p1: Override in PP and C.
  • p2: Override in C, implementation in PP.
  • p3: Override in PP.
  • p4: Override in C, implementation in P.

@DanielEScherzer DanielEScherzer Jun 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added the cases from the origin test file which I think now covers everything

@DanielEScherzer

DanielEScherzer commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

Rebased onto latest master

iluuu1994
iluuu1994 approved these changes Jun 30, 2026
View reviewed changes

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm very sorry for the big delay. Some things got in the way yesterday.

Only a nit. The implementation looks correct. Too many tests for my taste.

Comment thread Zend/zend_compile.c Outdated
iluuu1994
iluuu1994 reviewed Jun 30, 2026
View reviewed changes
Comment thread Zend/zend_compile.c Outdated
iluuu1994
iluuu1994 approved these changes Jun 30, 2026
View reviewed changes

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@DanielEScherzer

DanielEScherzer commented Jul 3, 2026

Copy link
Copy Markdown
Member Author

Rebased and added UPGRADING, will merge once CI passes

@DanielEScherzer DanielEScherzer merged commit 4629b3e into php:master Jul 3, 2026
18 checks passed
@DanielEScherzer DanielEScherzer deleted the override-constants branch July 3, 2026 20:03
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
@iluuu1994 iluuu1994 iluuu1994 approved these changes
@kocsismate kocsismate Awaiting requested review from kocsismate kocsismate is a code owner
@dstogov dstogov Awaiting requested review from dstogov dstogov is a code owner

Assignees

No one assigned

Labels

ABI break Category: Engine RFC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DanielEScherzer @TimWolla @iluuu1994