Skip to content

Disallow indirect modification on readonly properties within __clone() #15012

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

Conversation

iluuu1994
Copy link
Member

Indirect modification isn't allowed in __construct() because it allows references to leak, so it doesn't make much sense to allow it in __clone().

@iluuu1994 iluuu1994 requested a review from kocsismate July 18, 2024 16:08
@iluuu1994 iluuu1994 requested a review from dstogov as a code owner July 18, 2024 16:08
@iluuu1994 iluuu1994 force-pushed the disallow-indirect-modification-in-readonly-clone branch from ad503d0 to 14be48b Compare July 18, 2024 16:08
@dstogov
Copy link
Member

dstogov commented Jul 23, 2024

It looks like this reverts the behavior introduced by https://wiki.php.net/rfc/readonly_amendments (that was voted and accepted). I have no idea about the desired behavior, but if something was introduced by mistake, please consider refactoring the whole https://wiki.php.net/rfc/readonly_amendments patch.

cc: @kocsismate

@kocsismate
Copy link
Member

Thanks for finding this inconsistency with #7942.

It looks like this reverts the behavior introduced by https://wiki.php.net/rfc/readonly_amendments (that was voted and accepted)

It only reverts some part of it where the property is modified indirectly.

@iluuu1994 iluuu1994 force-pushed the disallow-indirect-modification-in-readonly-clone branch 2 times, most recently from 624c061 to e6120aa Compare August 8, 2024 21:49
@iluuu1994 iluuu1994 force-pushed the disallow-indirect-modification-in-readonly-clone branch from e6120aa to ba5e67e Compare August 8, 2024 21:50
@iluuu1994
Copy link
Member Author

I didn't realize this required JIT changes. @dstogov Can you please check if they look correct to you?

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

The JIT part seems to reflect the VM changes.
I can't verify this in all details but I think this may be merged.

Indirect modification isn't allowed in __construct() because it allows
references to leak, so it doesn't make much sense to allow it in __clone().
@iluuu1994 iluuu1994 force-pushed the disallow-indirect-modification-in-readonly-clone branch from ba5e67e to 2cf7993 Compare August 9, 2024 09:55
@iluuu1994 iluuu1994 merged commit 46ee0fb into php:master Aug 9, 2024
7 of 11 checks passed
@iluuu1994
Copy link
Member Author

@dstogov Thank you for the review!

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