Hi Nick On Fri, Jul 11, 2025 at 6:31 AM Nick <p...@nicksdot.dev> wrote: > >> On 8. Jun 2025, at 11:16, Larry Garfield <la...@garfieldtech.com> wrote: >> >> https://wiki.php.net/rfc/readonly_hooks >> >> To not get this buried in individual answers to others: > > I came up with two alternative implementations which cache the computed `get` > hook value. > One leverages separate cache properties, the other writes directly to the > backing store. > > Links to the alternative branches can be found in the description of the > original PR. > https://github.com/php/php-src/pull/18757
I am not a fan of the caching approach. The implementation draft for this approach [^1] works by storing the assigned value in the property slot, and replacing it with the value returned from get one called for the first time. One of the issues here is that the backing value is observable without calling get. For example: ``` class C { public public(set) readonly string $prop { get => strtoupper($this->prop); } } $c = new C(); $c->prop = 'foo'; var_dump(((array)$c)['prop']); // foo $c->prop; var_dump(((array)$c)['prop']); // FOO ``` Here we can see that the underlying value changes, despite the readonly declaration. This is especially problematic for things like [un]serialize(), where calling serialize() before or after accessing the property will change which underlying value is serialized. Even worse, we don't actually know whether an unserialized property has already called the get hook. ``` class C { public public(set) readonly int $prop { get => $this->prop + 1; } } $c = new C(); $c->prop = 1; $s1 = serialize($c); $c->prop; $s2 = serialize($c); var_dump(unserialize($s1)->prop); // int(2) var_dump(unserialize($s2)->prop); // int(3) ``` Currently, get is always called after unserialize(). There may be similar issues for __clone(). For readable and writable properties, the straight-forward solution is to move the logic to set. ``` class C { public public(set) readonly int $prop { set => $value + 1; } } ``` This is slightly differently, semantically, in that it executes any potential side-effects on write rather than read, which seems reasonable. This also avoids the implicit mutation mentioned previously. At least in these cases, disallowing readonly + get seems reasonable to me. I will say that this doesn't solve all get+set cases. For example, proxies. Hopefully, lazy objects can mostly bridge this gap. Another case is lazy getters. ``` class C { public readonly int $magicNumber { get => expensiveComputation(); } } ``` This does not seem to work in the current implementation: > Fatal error: Hooked virtual properties cannot be declared readonly I presume it would be possible to fix this, e.g. by using readonly as a marker to add a backing value to the property. I'm personally not too fond of making the rules on which properties are backed more complicated, as this is already a common cause for confusion. I also fundamentally don't like that readonly changes whether get is called. Currently, if hooks are present, they are called. This adds more special cases to an already complex feature. To me it seems the primary motivator for this RFC are readonly classes, i.e. to prevent the addition of hooks from breaking readonly classes. However, as lazy-getters are de-facto read-only, given they are only writable from the extremely narrow scope of the hook itself, the modifier doesn't do much. Maybe an easier solution would be to provide an opt-out of readonly. Side note: Your implementation has a bug: ``` class C { public public(set) readonly int $prop { get => $this->prop + 1; } } function test($c) { var_dump($c->prop); } $c = new C(); $c->prop = 1; test($c); // int(2) test($c); // int(3) ``` You likely need to dodge the fast path in the VM by not marking the property as "SIMPLE_GET" [^2]. Sorry if this e-mail is a bit all over the place, I had trouble structuring it in a more sensible way. Ilija [^1]: https://github.com/php/php-src/compare/master...NickSdot:php-php-src:readonly-hooks-once [^2]: https://github.com/php/php-src/blob/4d9fc506df1131c630c530a0bfa6d0338cffa03c/Zend/zend_object_handlers.c#L864