Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123563
Feedback-ID: i9169476b:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-497-g97f96844c-fm-20240526.001-g97f96844
Precedence: bulk
MIME-Version: 1.0
Message-ID: <834cb6c7-c6ae-4b59-b022-a43793f842a3@app.fastmail.com>
In-Reply-To: <caf753e7-99e4-486f-9983-dff83f8a40d7@app.fastmail.com>
References: <caf753e7-99e4-486f-9983-dff83f8a40d7@app.fastmail.com>
Date: Sun, 09 Jun 2024 16:46:16 +0200
To: "Larry Garfield" <larry@garfieldtech.com>,
 "PHP internals" <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC][Vote] Property Hooks
Content-Type: multipart/alternative;
 boundary=2cfcd4e645d946f5952a4142eb2bacc4
From: php@npopov.com ("Nikita Popov")

--2cfcd4e645d946f5952a4142eb2bacc4
Content-Type: text/plain

On Mon, Apr 15, 2024, at 18:43, Larry Garfield wrote:
> The vote for the Property Hooks RFC is now open:
> 
> https://wiki.php.net/rfc/property-hooks
> 
> Voting will close on Monday 29 April, afternoonish Chicago time.

I know I'm a few months late on this one, but I figured I'd still leave a couple of thoughts... Overall, the proposal is well thought out and does address many of the reservations I had about my original accessors proposal.

One of the more interesting choices in this proposal is to base whether the property is virtual or backed on the presence of a "$this->prop" reference in the accessor implementation. I think that, conceptually at least, this is a good choice.

What I find concerning though, is that the presence/absence of such a reference also affects the meaning of the get and set hooks. If the property is virtual and it only has get, then the property cannot be set. If the property is backed and only has get, then the property *can* be set. A no-op setter is implied. (Similar for only having a set hook.)

This has the unfortunate consequence that you actually have to look at the accessor implementation to determine the API of the class -- only looking at the "prototypes" (i.e. signatures without implementation bodies) is no longer sufficient! This seems both unfortunate and unprecedented.

This could have been avoided by still requiring an explicit no-op "set;" at the expensive of some additional verbosity.

The other thing that stood out to me are the short-hand notations using =>. There was a prior RFC on the topic (https://wiki.php.net/rfc/short-functions), which has been declined. That RFC would have introduced => ... as a general shorthand for { return ...; }.

The shorthand notation for get is compatible with that formulation. However, the shorthand notation for set is not. In that case => ... isn't short for { return ...; }, but rather for { $this->prop = ...; }.

This seems pretty unfortunate to me, and possibly closes the door on revisiting a general short function syntax in the future. Mostly I'm scratching my head at why this was included in the proposal at all, as I would not expect this use of the set hook to be common enough to justify a shorthand. The common case is a guard that checks the value without modifying it.

Putting this to the "would this shorthand have passed if it were introduced by a separate RFC on top of the base implementation" test, I think the answer would have been a clear "no".

Regards,
Nikita
--2cfcd4e645d946f5952a4142eb2bacc4
Content-Type: text/html
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}
p.MsoNormal,p.MsoNoSpacing{margin:0}</style></head><body><div>On Mon, Ap=
r 15, 2024, at 18:43, Larry Garfield wrote:<br></div><blockquote type=3D=
"cite" id=3D"qt" style=3D""><div>The vote for the Property Hooks RFC is =
now open:<br></div><div><br></div><div><a href=3D"https://wiki.php.net/r=
fc/property-hooks">https://wiki.php.net/rfc/property-hooks</a><br></div>=
<div><br></div><div>Voting will close on Monday 29 April, afternoonish C=
hicago time.<br></div></blockquote><div><br></div><div>I know I'm a few =
months late on this one, but I figured I'd still leave a couple of thoug=
hts... Overall, the proposal is well thought out and does address many o=
f the reservations I had about my original accessors proposal.<br></div>=
<div><br></div><div>One of the more interesting choices in this proposal=
 is to base whether the property is virtual or backed on the presence of=
 a "$this-&gt;prop" reference in the accessor implementation. I think th=
at, conceptually at least, this is a good choice.<br></div><div><br></di=
v><div>What I find concerning though, is that the presence/absence of su=
ch a reference also affects the meaning of the get and set hooks. If the=
 property is virtual and it only has get, then the property cannot be se=
t. If the property is backed and only has get, then the property *can* b=
e set. A no-op setter is implied. (Similar for only having a set hook.)<=
br></div><div><br></div><div>This has the unfortunate consequence that y=
ou actually have to look at the accessor implementation to determine the=
 API of the class -- only looking at the "prototypes" (i.e. signatures w=
ithout implementation bodies) is no longer sufficient! This seems both u=
nfortunate and unprecedented.<br></div><div><br></div><div>This could ha=
ve been avoided by still requiring an explicit no-op "set;" at the expen=
sive of some additional verbosity.<br></div><div><br></div><div>The othe=
r thing that stood out to me are the short-hand notations using =3D&gt;.=
 There was a prior RFC on the topic (<a href=3D"https://wiki.php.net/rfc=
/short-functions">https://wiki.php.net/rfc/short-functions</a>), which h=
as been declined. That RFC would have introduced =3D&gt; ... as a genera=
l shorthand for { return ...; }.<br></div><div><br></div><div>The shorth=
and notation for get is compatible with that formulation. However, the s=
horthand notation for set is not. In that case =3D&gt; ... isn't short f=
or { return ...; }, but rather for { $this-&gt;prop =3D ...; }.<br></div=
><div><br></div><div>This seems pretty unfortunate to me, and possibly c=
loses the door on revisiting a general short function syntax in the futu=
re. Mostly I'm scratching my head at why this was included in the propos=
al at all, as I would not expect this use of the set hook to be common e=
nough to justify a shorthand. The common case is a guard that checks the=
 value without modifying it.<br></div><div><br></div><div>Putting this t=
o the "would this shorthand have passed if it were introduced by a separ=
ate RFC on top of the base implementation" test, I think the answer woul=
d have been a clear "no".<br></div><div><br></div><div>Regards,<br></div=
><div>Nikita<br></div></body></html>
--2cfcd4e645d946f5952a4142eb2bacc4--