PHP 8.2 readonly classes allow inheriting mutable properties from traits

[TysonAndre]

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

1. Making the inherited properties readonly (seems strange and makes trait harder to reason about without knowing about uses) - also, the readonly keyword can already be used
2. 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
3. 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)
4. readonly trait 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

[TysonAndre]

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

• Adding a property to a trait that previously had no properties might become a breaking change (in composer libraries) with option 2 for readonly classes using a library, but that may be something to just accept given the convenience of letting readonly classes use traits to avoid boilerplate
• Option 3 would allow switching to any other options later

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

Any declared property of a readonly class is implicitly treated as readonly

[cmb69]

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]

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 :)

[kocsismate]

I commited 0897266 to fix the issue. Also, I added an errata in the RFC itself: https://wiki.php.net/rfc/readonly_classes

[drealecs]

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 :)

Just leaving a note here as well in case someone does not read the PR details. The option 2 was the one actually implemented.