Repository navigation
PHP 8.2 readonly classes allow inheriting mutable properties from traits #9285
Description
Description
The following code:
<?php
trait T2{ public $prop; }
readonly class G { use T2; }
$g = new G();
$g->prop = 123;
$g->prop = 124;
var_export($g);Resulted in this output:
\G::__set_state(array(
'prop' => 124,
))
But I expected this output instead:
One of the following
- Making the inherited properties readonly (seems strange and makes trait harder to reason about without knowing about uses) - also, the
readonlykeyword can already be used - Making it an error to inherit a property from a trait at the time of inheritance when all trait properties are determined (including traits extending other traits)
EDIT: Or just non-readonly properties - Making it an error to use traits from readonly classes (similar to existing check that makes it an error to extend from non-readonly classes, including classes that have no properties)
readonlytrait support, which requires its own rfc. Ideas:
a. (can only be used by readonly classes and readonly traits?)
b. EDIT: A better restriction might be that they can have properties, but all properties declared within the trait or inherited by the trait must be readonly (allow them to modify properties declared elsewhere, which should rarely be needed)
So far, the only reference to traits in the implementation was just checking that the modifier couldn't be applied to traits (https://github.com/php/php-src/pull/7305/files#diff-3fa220fdd8047943b96515df0805331f7ae6d52a0228084d5e9b491e8f1b01a9)
PHP Version
PHP 8.2.0beta2
Operating System
No response
Activity
TysonAndre commentedon Aug 9, 2022
@kocsismate @nikic thoughts? Option 2 or 3 seem the most likely (change the least) after the php 8.2 feature freeze, and would make it easier to switch to other options in the future.
I guess there's technically a 5th option(the current status before noticing the bug) of leaving it as is for traits, and having readonly classes not actually being read only, but that (1) goes against the stated intent, and (2) seems like code would break in the likely case of restricting the behavior of readonly classes in the future
cmb69 commentedon Aug 9, 2022
I think we should go with option 3 for now; that appears to be the most consistent option, and indeed leaves as room for improvements.
kocsismate commentedon Aug 10, 2022
Thank you for finding this issue, and providing such a detailed summary! I've just submitted the implementation for option 3: #9291
I also think that this is the least bad solution for now :)
Fix phpGH-9285 Traits cannot be used in readonly classes
kocsismate commentedon Aug 21, 2022
I commited 0897266 to fix the issue. Also, I added an errata in the RFC itself: https://wiki.php.net/rfc/readonly_classes
drealecs commentedon Aug 22, 2022
Just leaving a note here as well in case someone does not read the PR details. The option 2 was the one actually implemented.