Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:122613
Precedence: bulk
MIME-Version: 1.0
References: <fca97d6c-d4ec-4000-958a-72c32beb8421@app.fastmail.com>
 <a715dacb-9bac-4bea-a2bb-c623737fd81e@app.fastmail.com> <CABdc3WrxQx4GOgXdo9UeryXY+aFHB_+0hAjwT6z-=dS8jMet9g@mail.gmail.com>
 <84adc38a-210e-4f21-9145-0c99ae8c56f2@app.fastmail.com>
In-Reply-To: <84adc38a-210e-4f21-9145-0c99ae8c56f2@app.fastmail.com>
Date: Mon, 11 Mar 2024 17:58:26 +0100
Message-ID: <CABdc3Wr-rjDVXvc6ed4SoNe0JvJDQz=0DxxfKmRZdf85UZdCrw@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC[ Property accessor hooks, take 2
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000f74e710613657323"
From: michal.brzuchalski@gmail.com (=?UTF-8?Q?Micha=C5=82_Marcin_Brzuchalski?=)

--000000000000f74e710613657323
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

pon., 11 mar 2024 o 15:30 Larry Garfield <larry@garfieldtech.com>
napisa=C5=82(a):

> On Mon, Mar 11, 2024, at 8:35 AM, Micha=C5=82 Marcin Brzuchalski wrote:
> > Hi Larry,
> >
> > pt., 8 mar 2024 o 16:55 Larry Garfield <larry@garfieldtech.com>
> napisa=C5=82(a):
> >> Hi folks.  Based on earlier discussions, we've made a number of change=
s
> to the RFC that should address some of the concerns people raised.  We al=
so
> had some very fruitful discussions off-list with several developers from
> the Foundation, which led to what we feel are some solid improvements.
> >>
> >> https://wiki.php.net/rfc/property-hooks
> >>
> >
> > This RFC looks awesome, thanks Larry and Ilija I love the functionality
> > in its current shape.
> >
> >> Thank you everyone for the feedback so far, and if you still have some=
,
> please say so.  (Even if it's just to say that you're happy with the RFC
> now so we feel more comfortable bringing it to a vote.)
> >
> > The only thing I don't like and can still be worked on is the
> > reflection mechanism changes.
> > The proposed methods isVirtual and getRawValue, setRawValue pair
> > introduces a need to catch exceptions which could be eliminated by
> > subtyping ReflectionProperty.
> > Having these methods on a separate subtype allows returning a valid
> > value.
> > I realize this isn't trivial because for the last 2 days, I was
> > thinking about giving it a name and TBH cannot figure out anything
> > feasible.
> > If this is not possible to put in understandable words then at least
> > mention it in FAQ and why not.
> >
> > Cheers,
> > Micha=C5=82 Marcin Brzuchalski
>
> Hm, interesting.  I'll have to check with Ilija on feasibility.  My
> question is, how would it eliminate it?
>
> Suppose we hypothetically have a "ReflectionPropertyWithHooks" reflection
> object.  It has those three extra methods on it.  But that means
> $rObject->getProperty() could return a ReflectionProperty or
> ReflectionPropertyWithHooks, and you don't know which it is.  You'd have =
to
> do an instanceof check to know which type of property it is, and thus wha=
t
> methods are available.  That doesn't seem any less clumsy (nor more, to b=
e
> fair) than calling isVirtual().


It is similar when you work with ReflectionType or ReflectionEnum, you
always need to match against a certain type to ensure the code will behave
predictably.


> $rProp =3D $rObject->getProperty('foo', $obj);
> $rProp->getValue(); // works always.
>
> if (!$rProp->isVirtual()) {
>     $rProp->getRawValue(); // Works, may or may not be the same return as
> getValue()
> }
>
> vs.
>
> if (!$rProp instanceof VirtualProperty) {
>   $rProp->getRawValue(); // Works.
> }
>
> That doesn't seem to be an improvement.  If you omit the conditional, you
> still need a catch one way or the other.  It just changes what gets throw=
n
> (an Exception vs an Error).  What type of hierarchy were you thinking of
> that would help here?
>

My thinking was like:
1. if the property has hooks, only then calling getRawValue, setRawValue,
or isVirtual make sense,
2. if the property has no hooks and is static calling getRawValue, or
setRawValue always throws because of "On a static property, this method
will always throw an error."
3. if the property is "virtual", calling getRawValue, or setRawValue always
throws an error,
4. if the property is not "virtual", calling getValue, or setValue is safe
and never throws, otherwise it may throw under some conditions related to
certain hook presence.

In conclusion, I thought that the presence of hooks introduces 3 new
methods but some will always be thrown because of incorrect usage.
Normally, I'd model it with subtypes to completely avoid try-catch blocks
for the cost of a simple instanceof check which I consider much cleaner for
the reader than a bunch of try-catch blocks. Remember about static
analysis, each of them when checked will propose to handle possible
exceptions.

But as wrote before, I don't know how to model it well.

Cheers,
Micha=C5=82 Marcin Brzuchalski

--000000000000f74e710613657323
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">pon., 11 mar 2024 o 15:30=C2=A0Larry =
Garfield &lt;<a href=3D"mailto:larry@garfieldtech.com">larry@garfieldtech.c=
om</a>&gt; napisa=C5=82(a):<br></div><blockquote class=3D"gmail_quote" styl=
e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin=
g-left:1ex">On Mon, Mar 11, 2024, at 8:35 AM, Micha=C5=82 Marcin Brzuchalsk=
i wrote:<br>
&gt; Hi Larry,<br>
&gt;<br>
&gt; pt., 8 mar 2024 o 16:55 Larry Garfield &lt;<a href=3D"mailto:larry@gar=
fieldtech.com" target=3D"_blank">larry@garfieldtech.com</a>&gt; napisa=C5=
=82(a):<br>
&gt;&gt; Hi folks.=C2=A0 Based on earlier discussions, we&#39;ve made a num=
ber of changes to the RFC that should address some of the concerns people r=
aised.=C2=A0 We also had some very fruitful discussions off-list with sever=
al developers from the Foundation, which led to what we feel are some solid=
 improvements.<br>
&gt;&gt; <br>
&gt;&gt; <a href=3D"https://wiki.php.net/rfc/property-hooks" rel=3D"norefer=
rer" target=3D"_blank">https://wiki.php.net/rfc/property-hooks</a><br>
&gt;&gt; <br>
&gt;<br>
&gt; This RFC looks awesome, thanks Larry and Ilija I love the functionalit=
y <br>
&gt; in its current shape.<br>
&gt;<br>
&gt;&gt; Thank you everyone for the feedback so far, and if you still have =
some, please say so.=C2=A0 (Even if it&#39;s just to say that you&#39;re ha=
ppy with the RFC now so we feel more comfortable bringing it to a vote.)<br=
>
&gt;<br>
&gt; The only thing I don&#39;t like and can still be worked on is the <br>
&gt; reflection mechanism changes.<br>
&gt; The proposed methods isVirtual and getRawValue, setRawValue pair <br>
&gt; introduces a need to catch exceptions which could be eliminated by <br=
>
&gt; subtyping ReflectionProperty.<br>
&gt; Having these methods on a separate subtype allows returning a valid <b=
r>
&gt; value.<br>
&gt; I realize this isn&#39;t trivial because for the last 2 days, I was <b=
r>
&gt; thinking about giving it a name and TBH cannot figure out anything <br=
>
&gt; feasible.<br>
&gt; If this is not possible to put in understandable words then at least <=
br>
&gt; mention it in FAQ and why not.<br>
&gt;<br>
&gt; Cheers,<br>
&gt; Micha=C5=82 Marcin Brzuchalski<br>
<br>
Hm, interesting.=C2=A0 I&#39;ll have to check with Ilija on feasibility.=C2=
=A0 My question is, how would it eliminate it?<br>
<br>
Suppose we hypothetically have a &quot;ReflectionPropertyWithHooks&quot; re=
flection object.=C2=A0 It has those three extra methods on it.=C2=A0 But th=
at means $rObject-&gt;getProperty() could return a ReflectionProperty or Re=
flectionPropertyWithHooks, and you don&#39;t know which it is.=C2=A0 You=
9;d have to do an instanceof check to know which type of property it is, an=
d thus what methods are available.=C2=A0 That doesn&#39;t seem any less clu=
msy (nor more, to be fair) than calling isVirtual().=C2=A0</blockquote><div=
><br></div><div>It is similar when you work with ReflectionType or Reflecti=
onEnum, you always need to match against a certain type to ensure the code =
will behave predictably.</div><div>=C2=A0</div><blockquote class=3D"gmail_q=
uote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,2=
04);padding-left:1ex">
$rProp =3D $rObject-&gt;getProperty(&#39;foo&#39;, $obj);<br>
$rProp-&gt;getValue(); // works always.<br>
<br>
if (!$rProp-&gt;isVirtual()) {<br>
=C2=A0 =C2=A0 $rProp-&gt;getRawValue(); // Works, may or may not be the sam=
e return as getValue()<br>
}<br>
<br>
vs.<br>
<br>
if (!$rProp instanceof VirtualProperty) {<br>
=C2=A0 $rProp-&gt;getRawValue(); // Works.<br>
}<br>
<br>
That doesn&#39;t seem to be an improvement.=C2=A0 If you omit the condition=
al, you still need a catch one way or the other.=C2=A0 It just changes what=
 gets thrown (an Exception vs an Error).=C2=A0 What type of hierarchy were =
you thinking of that would help here?<br></blockquote><div><br></div><div>M=
y thinking was like:</div><div>1. if the property has hooks, only then call=
ing getRawValue, setRawValue, or isVirtual make sense,</div><div>2. if the =
property has no hooks and is static calling getRawValue, or setRawValue alw=
ays throws because of &quot;On a static property, this method will always t=
hrow an error.&quot;</div><div>3. if the property is &quot;virtual&quot;, c=
alling getRawValue, or setRawValue always throws an error,</div><div>4. if =
the property is not &quot;virtual&quot;, calling getValue, or setValue is s=
afe and never throws, otherwise it may throw under some conditions related =
to certain hook presence.<br></div><div><br></div><div>In conclusion, I tho=
ught that the presence of hooks introduces 3 new methods but some will alwa=
ys be thrown because of incorrect usage.</div><div>Normally,=C2=A0I&#39;d m=
odel it with subtypes to completely avoid try-catch blocks for the cost of =
a simple instanceof check which I consider much cleaner for the reader than=
 a bunch of try-catch blocks. Remember about static analysis, each of them =
when checked will propose to handle possible exceptions.</div><div><br></di=
v><div>But as wrote before, I don&#39;t know how to model it well.</div><di=
v><br></div><div>Cheers,</div><div>Micha=C5=82 Marcin Brzuchalski</div></di=
v></div>

--000000000000f74e710613657323--