Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:120222
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-415-gf2b17fe6c3-fm-20230503.001-gf2b17fe6
Mime-Version: 1.0
Message-ID: <57aed5a4-f5e5-4a47-bf7a-69249f1d0ef1@app.fastmail.com>
In-Reply-To: <641b1ca0-d33f-4f38-ae64-81b4abce24da@app.fastmail.com>
References: <641b1ca0-d33f-4f38-ae64-81b4abce24da@app.fastmail.com>
Date: Tue, 09 May 2023 17:46:50 +0000
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [RFC] Property hooks, nee accessors
From: larry@garfieldtech.com ("Larry Garfield")

On Mon, May 8, 2023, at 9:38 PM, Larry Garfield wrote:
> Ilija Tovilo and I would like to offer another RFC for your=20
> consideration.  It's been a while in coming, and we've evolved the=20
> design quite a bit just in the last week so if you saw an earlier draf=
t=20
> of it in the past few months, I would encourage you to read it over=20
> again to make sure we're all on the same page.  I'm actually pretty=20
> happy with where it ended up, even if it's not the original design. =20
> This approach eliminates several hard-to-implement edge cases while=20
> still providing a lot of functionality in one package.
>
> https://wiki.php.net/rfc/property-hooks

Thanks everyone for your feedback so far.  I'm going to collapse the rep=
lies into one message to reduce noise.

On Tue, May 9, 2023, at 7:57 AM, Tim D=C3=BCsterhus wrote:

> (1) How does the set() hook interact with #[\SensitiveParameter]?

Internally, the hook is a method, so it should respect it the same way a=
s any other method.  Viz, you should be able to write:

```
public string $foo {
  set ($[\SensitiveParameter] string $value) {
    $field =3D strtolower($foo);
  }
}
```

I'll ask Ilija to add a test to confirm that.

> (2) How will a Stack Trace emitted from a hook look like? Please inclu=
de=20
> an example of the Exception::__toString() output or so.

I'll ask Ilija to add a test to his branch and link it here.

> (3) ReflectionProperty::getHook(): Will this throw if an invalid hook=20
> name is given or will this return null?

Null if you ask for get/set and one doesn't exist.  Error if you ask for=
 something other than "get" or "set".  We're still keeping it as a singl=
e method, though, to make supporting other hooks easier in the future if=
 desired.  I've updated the RFC to clarify that.

> (4) The "Unaffected PHP Functionality" section is empty. It should=20
> either be filled in or removed.

Removed.  I don't quite understand the purpose of that section. :-)

On Tue, May 9, 2023, at 9:54 AM, Nicolas Grekas wrote:

>    - does set($value) { mean set(mixed $value) { or set(TypeFromProp
>    $value) { ? I understand it would be the latter but it might be nic=
e to
>    clarify (unless I missed it)

Conceptually, I believe it means set(TypeFromProp $value).  Since at pre=
sent that cannot be enforced, though, they're basically the same thing. =
 (If someone with more engine-fu than either of us wants to help enforce=
 contravariant types in set, please speak up because we'd prefer to do s=
o.)

>    - function setFullName =3D> ucfirst is not found in the "set" versi=
on

Yes it is.  Look at the end of the `set` line.

>    - could parent::$x::set($x) be replaced by parent::$x =3D $x ? Same=
 for
>    the get variant? Why not if not?

There's a subtle but important distinction here.  parent::$x would sugge=
st accessing the parent *property*, ie, its backing value, directly.  Wh=
at is actually intended here is accessing the parent *hook*, ie, its equ=
ivalent to parent::setFoo($foo).  The property itself is already availab=
le as $field.

We're open to alternate syntaxes for accessing the parent accessor synta=
x if people have good (and easily implementable) suggestions.

>    - readonly: could this be supported by materializing them, using the
>    storage for memoization after first access? that'd be quite powerful

That was one of my early ideas, as it then became a cheap form of cached=
 lazy property.  However, that then means there's no meaningful way to u=
nset the cache.  In the end we decided that there were too many nooks an=
d crannies to readonly to try and make sense of.  (The more I use it, th=
e more I think readonly was a mistake and we should have gone straight t=
o aviz.)

(As a side note, if we had aviz, then we could use the field itself as a=
 cache by making writes private, like so:

public string $name =3D> $field ??=3D $this->first . $this->last;

But right now that would also imply public-set, which we wouldn't want. =
 Aviz would solve that issue entirely.  Another reason aviz is an import=
ant and useful feature. :-) )

What a readonly flag on a set-only property means... I have no idea. =20

>    - I'm not sure we need ReflectionProperty::getHook('set')  when we =
can
>    do getHooks()['set'] ?? null

The double-methods were mainly for consistency with the rest of the Refl=
ection API, which tends to have both getFoo(): object and getFoos(): arr=
ay options.  It's no more redundant than the rest of Reflection.

>    - What does ReflectionMethod::getName return on a hook? And getClos=
ure?

Either "get" or some hashed string that includes "get".  I'm not sure of=
f hand.  I'll have to check with Ilija.

>    - Should we add ReflectionProperty::IS_VIRTUAL ?

Funny story.  I just asked Ilija this, and he said he already did add it=
 for testing purpose but forgot to tell me. :-)  RFC updated.

>    - I'm a bit surprised by the possibility of adding attributes to ho=
oks.
>    I'm not sure I see the use case and I think this might be confusing=
 in
>    addition to attributes on the property itself.

I think in practice it's actually more work not to, since hooks under th=
e hood are just methods like anything else.  And as Tim noted above, the=
re are use cases like #[SensitiveParameter] for attributes even down her=
e.

>    - For serialization: exclude them from var_export() and serialize(),
>    which don't need virtual props to rebuild the state (you mean
>    json_encode(), not JsonSerializable on that list), call get for
>    var_dump/print_r when it's defined (but ignore exceptions). But (ar=
ray) /
>    get_mangled_object_vars() / get_object_vars() shouldn't call any "g=
et".

I don't follow.  json_encode() is on the list.

> I have a bigger concern: the take on references contradicts with the i=
ntro
> about BC breaks: turning a materialized property into virtual one would
> break BC as far as refs are concerned. One idea to fix that: add a ref
> hook, that must return a reference. If not implemented =3D> Error when=
 trying
> to get-by-ref. This could also help solve the $foo->array[] case (incl=
uding
> the asym-visiblity issue).

That might be possible.  The issue there is that the returned ref would =
still be bypassing the get/set hooks, so you're just adding a way to dan=
ce around the existing hook implementations, which is exactly what we're=
 trying to avoid.  Also, what would that hook even mean if used on a vir=
tual property?

Moreover, in practice, getting a reference to a property is extremely ra=
re, with the exception of array writes.  I cannot recall the last time I=
 even saw some code get a reference to a property.  That's why we felt c=
omfortable just leaving it at is.  In an earlier version of the RFC we h=
ad a way for properties to just disable references without adding any ho=
oks, but removed it on the grounds that it was not worth the effort.

On Tue, May 9, 2023, at 11:52 AM, Dik Takken wrote:

> I'm not sure which solution would be best here: Just accept the=20
> limitations for arrays (same as for list properties in Python) or forb=
id=20
> the use of hooks for array properties? I'm leaning towards accepting t=
he=20
> limitations, like Python does. Then it is up to the API designer to=20
> decide if a property hook is the right tool for the job or not.

We considered disallowing it on array properties; the problem is, there'=
s also mixed and iterable properties that would have the same challenge,=
 but that couldn't be detected at compile time.  That means the runtime =
check has to be there anyway, so also having a compile-time check for ho=
oks on arrays doesn't really buy us anything but more engine code.  So y=
es, Python-style "just deal with it *sunglasses*" is the most logical ap=
proach.

On Tue, May 9, 2023, at 12:38 PM, Benjamin Au=C3=9Fenhofer wrote:

> An error maybe, the "class C" example in "Detailed Proposal > set" use=
s a
> "$this->_prop", but probably meant to use $this->_names, since private
> array $_names; is declared in the example.

Yep, typo, thanks.  Fixed now.

> I think $field should be its own "chapter". Its a central part of the
> proposal that should be clarified early and so that readers don't
> accidentally skip it.
>
> I am also confused why $field exists when $this->propertyName works an=
d why
> its not recommended to be used. Is $field a reference? or does the "co=
mpile
> time macro" part mean its replaced at compile time? Ifso, this feels
> different to anything else PHP, i am leaning towards $this->propertyNa=
me if
> there are no other compelling reasons why $field should be used.

Regarding $field vs. $this->propName, there's a few reasons we went that=
 route.

1. It's shorter and less typing for what will be a common pattern.
2. That makes it consistent between hook implementations.  In working on=
 examples, I found many cases where I was adding basically the same line=
 to multiple hooks on the same class.  Making the code easier to copy-pa=
ste seems like a win.
3. It also will be helpful if hook packages are added in the future, as =
they'll need a more "generic" way to access the backing property.  (See =
Future Scope.)  Eg, "$field =3D someLogic($value)" applied to a dozen di=
fferent properties; it wouldn't work if it was "$this->specificProperty =
=3D someLogic($value)".
4. We're used to our eyes glossing over "$this->propName", as it's so co=
mmon.  Having a separate name to mentally scan for to determine if a pro=
perty is virtual or not seems like it will be helpful in practice.
5. There's precedent for it: Kotlin has almost the same functionality as=
 we describe here, and uses a `field` variable in the exact same way.

So it's mainly an ergonomics argument rather than a technical one.  "Com=
pile time macro" means it translates to the same AST as if you'd used $t=
his->propName.  There's precedent for that.  Constructor Property Promot=
ion works basically the same way.

--Larry Garfield