Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:128095
Precedence: bulk
MIME-Version: 1.0
References: <1e8634d7-ac1a-4025-b4e2-1948aabf5251@app.fastmail.com>
 <9D5043B2-1589-4FD5-B289-6E98FB1177BE@nicksdot.dev> <CAPyj-LDVi3ZYQuzzJz7d+cHdU6C-cxYJUSkuzdyvWpw+sWuCsg@mail.gmail.com>
 <0856c89f-2000-448a-bbbf-c145a8699f6a@app.fastmail.com> <CAOWwgpks5ksTn5Mj8UKYNDzhETUwU6vcHx9yEPfJupe5shPsPQ@mail.gmail.com>
 <4dee212b-55f9-4b7b-99c4-9fac4bc149cc@app.fastmail.com>
In-Reply-To: <4dee212b-55f9-4b7b-99c4-9fac4bc149cc@app.fastmail.com>
Reply-To: erictnorris@gmail.com
Date: Thu, 17 Jul 2025 09:10:28 -0400
Message-ID: <CA+NMNO2ERwZLBuUx3G6VqqpQb5edRyeqbDxi27+ht_JwF04bjA@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] Readonly property hooks
To: Rob Landers <rob@bottled.codes>
Cc: Nicolas Grekas <nicolas.grekas+php@gmail.com>, Larry Garfield <larry@garfieldtech.com>, 
	php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: eric.t.norris@gmail.com (Eric Norris)

On Thu, Jul 17, 2025 at 3:31=E2=80=AFAM Rob Landers <rob@bottled.codes> wro=
te:
>
> On Tue, Jul 15, 2025, at 19:27, Nicolas Grekas wrote:
>
>
>
> Le lun. 14 juil. 2025 =C3=A0 15:41, Larry Garfield <larry@garfieldtech.co=
m> a =C3=A9crit :
>
> On Sun, Jul 13, 2025, at 6:28 PM, Ilija Tovilo wrote:
> > Hi Nick
> >
> > On Fri, Jul 11, 2025 at 6:31=E2=80=AFAM Nick <php@nicksdot.dev> wrote:
> >>
> >>> On 8. Jun 2025, at 11:16, Larry Garfield <larry@garfieldtech.com> wro=
te:
> >>>
> >>> 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 compute=
d `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 t=
he 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 =3D> strtoupper($this->prop);
> >     }
> > }
> > $c =3D new C();
> > $c->prop =3D '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 =3D> $this->prop + 1;
> >     }
> > }
> > $c =3D new C();
> > $c->prop =3D 1;
> > $s1 =3D serialize($c);
> > $c->prop;
> > $s2 =3D 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 =3D> $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 =3D> 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 ind=
ividual properties readonly, then just don't mark the one that has a hook o=
n it (use aviz if needed) and there's no issue.
>
> Perhaps we're thinking about this the wrong way, though?  So far we've ta=
lked as though readonly makes the property write-once.  But... what if we t=
hink 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, mul=
tiple times.  Only writing to the actual value in the object table.  That g=
ives the exact same set of guarantees that a getX()/setX() method would giv=
e.  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 (l=
ike we already have with ??=3D), 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 =3D=3D=3D $foo->bar in all cases (though=
 that would likely hold in the 99% case in practice), but then, $foo->getBa=
r() =3D=3D=3D $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 be=
havior as see fit. This is especially useful to intercept accesses to said =
properties. Without readonly hooks, designing an abstract API that uses rea=
donly properties is a risky decision since it blocks any (future) implement=
ation 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 go=
od 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 ca=
n only be done via set/get hooks, but these hooks are not available on read=
only 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 obje=
cts 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`.