Re: [RFC] Readonly property hooks

On Thu, Jul 17, 2025 at 3:31 AM Rob Landers <rob@bottled.codes> wrote: > > On Tue, Jul 15, 2025, at 19:27, Nicolas Grekas wrote: > > > > Le lun. 14 juil. 2025 à 15:41, Larry Garfield <larry@garfieldtech.com> a écrit : > > On Sun, Jul 13, 2025, at 6:28 PM, Ilija Tovilo wrote: > > Hi Nick > > > > On Fri, Jul 11, 2025 at 6:31 AM Nick <php@nicksdot.dev> wrote: > >> > >>> On 8. Jun 2025, at 11:16, Larry Garfield <larry@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. > > Thanks, Ilija. You expressed my concerns as well. And yes, in practice, readonly classes > over-reaching is the main use case; if you're marking individual properties readonly, then just > don't mark the one that has a hook on it (use aviz if needed) and there's no issue. > > Perhaps we're thinking about this the wrong way, though? So far we've talked as > though readonly makes the property write-once. But... what if we think of it as applying to the > field, aka the backing value? > > So readonly doesn't limit calling the get hook, or even the set hook, multiple times. > Only writing to the actual value in the object table. That gives the exact same set of guarantees > that a getX()/setX() method would give. The methods can be called any number of times, but the > stored value can only be written once. > > That would allow conditional set hooks, conditional gets, caching gets (like we already have > with ??=), and so on. The mental model is simple and easy to explain/document. The behavior is the > same as with methods. But the identity of the stored value would be consistent. > > It would not guarantee $foo->bar === $foo->bar in all cases (though that would likely > hold in the 99% case in practice), but then, $foo->getBar() === $foo->getBar() has never been > guaranteed either. > > Would that way of looking at it be acceptable to folks? > > > It does to me: readonly applies to the backed property, then hooks add behavior as see fit. > This is especially useful to intercept accesses to said properties. Without readonly hooks, > designing an abstract API that uses readonly properties is a risky decision since it blocks any > (future) implementation that needs this interception capability. As a forward-thinking author, one > currently has two choices: not using readonly properties in abstract APIs, or falling back to using > getter/setters. That's a design failure for hooks IMHO. I'm glad this RFC exists to fill > this gap. > > Nicolas > > > To add to this, as I just mentioned on the Records thread, it would be good to get hooks on > readonly objects. With the new clone(), there is no way to rely on validation in constructors. The > most robust validation in 8.5 can only be done via set/get hooks, but these hooks are not available > on readonly classes. This means that it is remarkably easy to "break" objects that do > constructor validation + use public(set) -- or use clone() in inherited objects instead of the > parent constructor. In my experience, readonly objects typically only do constructor validation > (DRY). (shoot, double post, sorry Rob) I'm not sure I follow - do you actually need both set and get hooks for validation? I would think only set hooks would be necessary, and I don't yet think I've seen an objection to set hooks for readonly.