Allow readonly properties to be reinitialized once during cloning #10389
Conversation
kocsismate
force-pushed
the
readonly-clone-reinit
branch
from
01f03db to
d88f16b
Compare
kocsismate
force-pushed
the
readonly-clone-reinit
branch
from
d88f16b to
d6bbf1a
Compare
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
I'm only halfway through the review, the rest will follow 🙂
kocsismate
force-pushed
the
readonly-clone-reinit
branch
from
d6bbf1a to
1aa6fe5
Compare
|
I rebased to current master + pushed a new commit fixing some of the review comments |
kocsismate
force-pushed
the
readonly-clone-reinit
branch
from
c10c4cd to
75d4d9d
Compare
| variable_ptr = &EG(error_zval); | ||
| goto exit; | ||
| } | ||
| if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { |
There was a problem hiding this comment.
I just realized that this is actually wrong. if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { should be done before zend_verify_property_type, because otherwise side-effects like __toString() will run even if the property cannot be assigned. Z_PROP_FLAG_P(variable_ptr) &= ~IS_PROP_REINITABLE; however should be run after. @kocsismate Can you adjust that?
There was a problem hiding this comment.
@iluuu1994 Yes, sure. I actually had the very same implementation initially for a few minutes, just to switch it to the current implementation. :)
There was a problem hiding this comment.
Adding a test should be simple enough. Create a class with __toString and assign to a readonly property that's already initialized.
There was a problem hiding this comment.
Yep, I have already created a similar test case locally :)
3 participants
RFC: https://wiki.php.net/rfc/readonly_amendments
Split from #9497