Skip to content

[RFC] Allow Readonly Property Defaults - #22588

Open
NickSdot wants to merge 5 commits into
php:masterfrom
NickSdot:readonly-property-defaults
Open

[RFC] Allow Readonly Property Defaults#22588
NickSdot wants to merge 5 commits into
php:masterfrom
NickSdot:readonly-property-defaults

Conversation

@NickSdot

@NickSdot NickSdot commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@TimWolla TimWolla added the RFC label Jul 4, 2026
TimWolla
TimWolla requested changes Jul 4, 2026
View reviewed changes
Comment thread Zend/tests/readonly_classes/readonly_with_property_default.phpt Outdated
Comment thread Zend/tests/readonly_props/readonly_with_default_interface.phpt Outdated
Comment thread Zend/tests/readonly_props/serialization.phpt
@NickSdot NickSdot mentioned this pull request Jul 4, 2026
Open
NickSdot added a commit to NickSdot/php__php-src that referenced this pull request Jul 5, 2026
@NickSdot
Added abstract class test
470be2a 
The engine has a non-optimal error here. Will be addressed in a separate PR. Ref: php#22588 (comment)
This was referenced Jul 5, 2026
Open
Merged
@NickSdot
NickSdot requested review from bukka and dstogov as code owners July 9, 2026 17:44
NickSdot added a commit to NickSdot/php__php-src that referenced this pull request Jul 9, 2026
@NickSdot
Added abstract class test
5f637da 
The engine has a non-optimal error here. Will be addressed in a separate PR. Ref: php#22588 (comment)
@NickSdot
NickSdot force-pushed the readonly-property-defaults branch from 20031f3 to 6937610 Compare July 9, 2026 18:23
TimWolla
TimWolla reviewed Jul 13, 2026
View reviewed changes
Comment thread Zend/tests/readonly_props/readonly_with_default_interface_get_only.phpt Outdated
Comment thread Zend/tests/readonly_props/serialization.phpt
NickSdot added a commit to NickSdot/php__php-src that referenced this pull request Jul 13, 2026
@NickSdot
Added test to confirm relock of unmodified properties while unseriali…
7e366ff 
…sation

Ref: php#22588 (comment)
TimWolla
TimWolla approved these changes Jul 13, 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.

Implementation and tests now LGTM. Can you do the rebase and apply your new lock/unlock helper onto clone as well to keep this internally consistent. Ideally four commits:

  1. Introduction of new helper.
  2. Use of helper for clone.
  3. Serializer change.
  4. Allowing defaults on readonly + Tests.

@NickSdot
NickSdot force-pushed the readonly-property-defaults branch 2 times, most recently from 661dae8 to fd7a6d2 Compare July 13, 2026 19:46
@NickSdot

NickSdot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Implementation and tests now LGTM. Can you do the rebase and apply your new lock/unlock helper onto clone as well to keep this internally consistent. Ideally four commits:

  1. Introduction of new helper.
  2. Use of helper for clone.
  3. Serializer change.
  4. Allowing defaults on readonly + Tests.

That's done, @TimWolla. Actually, wouldn't it make sense to get the first two commits merged on their own? They are a nice cleanup. Could you cherry pickt them? Or should I PR them separately? Would also make the diff smaller here.

@TimWolla

TimWolla commented Jul 13, 2026

Copy link
Copy Markdown
Member

Actually, wouldn't it make sense to get the first two commits merged on their own? They are a nice cleanup. Could you cherry pickt them? Or should I PR them separately?

Yes, feel free to PR them separately. The introduction of the new helper should also get a UPGRADING.INTERNALS entry then. The request to split this into clean commits was both to ease review and to leave the option of picking them separately.

@NickSdot
NickSdot force-pushed the readonly-property-defaults branch from fd7a6d2 to fb02215 Compare July 13, 2026 19:58
@NickSdot NickSdot mentioned this pull request Jul 13, 2026
Merged
@NickSdot
NickSdot marked this pull request as draft August 7, 2026 14:42
@NickSdot
NickSdot marked this pull request as ready for review August 7, 2026 14:42
@NickSdot

NickSdot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

The RFC was accepted. I would appreciate if someone could help me to get this merged. Thank you.

@TimWolla

TimWolla commented Aug 7, 2026

Copy link
Copy Markdown
Member

@NickSdot Can you rebase onto the latest master and perhaps already add NEWS / UPGRADING? “Core” would be the correct category.

@TimWolla

TimWolla commented Aug 7, 2026

Copy link
Copy Markdown
Member

I also just asked for someone to take over the review of #22718, since I won't get to it before Beta 1.

@NickSdot
NickSdot force-pushed the readonly-property-defaults branch from fb02215 to 6892b60 Compare August 7, 2026 16:39
@NickSdot

NickSdot commented Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks @TimWolla, both PRs are rebased; NEWS / UPGRADING were added.

Edit: rebased again and #22718 is in.

@NickSdot
NickSdot force-pushed the readonly-property-defaults branch from 6892b60 to b9f901a Compare August 7, 2026 18:26
DanielEScherzer
DanielEScherzer reviewed Aug 8, 2026
View reviewed changes
Comment thread ext/standard/var_unserializer.re Outdated
@NickSdot @DanielEScherzer
review: reduced serialize lock scope
34e6a9e 
Co-authored-by: DanielEScherzer <daniel.e.scherzer@gmail.com>
DanielEScherzer
DanielEScherzer reviewed Aug 8, 2026
View reviewed changes

@DanielEScherzer DanielEScherzer 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 took a look at the reflection code, and it shouldn't need to be updated, but can please add

  • a test case for ReflectionProperty::__toString() with a readonly property with a default
  • a test case for ReflectionClass::__toString() with a readonly property with a default

?

Comment thread Zend/tests/readonly_classes/readonly_with_property_default_trait.phpt Outdated
Comment thread Zend/tests/readonly_props/readonly_with_default_asymmetric_visibility.phpt Outdated
Comment thread ext/reflection/tests/ReflectionProperty_readonly_with_default.phpt
Comment thread Zend/tests/readonly_props/serialization.phpt Outdated
NickSdot added a commit to NickSdot/php__php-src that referenced this pull request Aug 8, 2026
@NickSdot
Improved readonly errors
3be9a37 
Ref: php#22588 (comment)
DanielEScherzer
DanielEScherzer approved these changes Aug 8, 2026
View reviewed changes

@DanielEScherzer DanielEScherzer 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 good to me

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

Reviewers

@TimWolla TimWolla TimWolla approved these changes
@DanielEScherzer DanielEScherzer DanielEScherzer approved these changes
@dstogov dstogov Awaiting requested review from dstogov
@bukka bukka Awaiting requested review from bukka bukka is a code owner

Assignees

No one assigned

Labels

ABI break Category: Engine Extension: reflection Extension: standard RFC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@NickSdot @TimWolla @DanielEScherzer