Skip to content

cloning: visibility applies to all properties, readonly is orthogonal - #5854

Merged
lacatoire merged 1 commit into
php:masterfrom
lacatoire:fix/cloning-visibility-vs-readonly
Sep 10, 2026
Merged

cloning: visibility applies to all properties, readonly is orthogonal#5854
lacatoire merged 1 commit into
php:masterfrom
lacatoire:fix/cloning-visibility-vs-readonly

Conversation

@lacatoire

Copy link
Copy Markdown
Member
  • language/oop5/cloning.xml: visibility is checked for all properties (not just readonly); readonly is a separate orthogonal constraint

@lacatoire
lacatoire requested a review from TimWolla September 10, 2026 14:22

@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.

This is no longer wrong, but I still find it confusing: Why would it mention readonly for visibility? There is no reason for users to believe that readonly behave any different from other properties visibility-wise (for cloning).

The “Additionally” sentence just repeats the same visibility part again. Of course readonly can only be overridden when you are in the right scope.

The correct claims are:

  1. Visibility is checked. No ifs.
  2. Updating readonly properties is legal, even if they are already set on the original object.

@lacatoire
lacatoire force-pushed the fix/cloning-visibility-vs-readonly branch 2 times, most recently from 4679ece to d04d1ad Compare September 10, 2026 14:36
@lacatoire
lacatoire requested a review from TimWolla September 10, 2026 14:40
@lacatoire
lacatoire merged commit 09c6504 into php:master Sep 10, 2026
4 of 6 checks passed
@lacatoire
lacatoire deleted the fix/cloning-visibility-vs-readonly branch September 10, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants