On Mon, May 8, 2023, at 9:38 PM, Larry Garfield wrote:
> Ilija Tovilo and I would like to offer another RFC for your
> consideration. It's been a while in coming, and we've evolved the
> design quite a bit just in the last week so if you saw an earlier draft
> of it in the past few months, I would encourage you to read it over
> again to make sure we're all on the same page. I'm actually pretty
> happy with where it ended up, even if it's not the original design.
> This approach eliminates several hard-to-implement edge cases while
> 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 replies
into one message to reduce noise.
On Tue, May 9, 2023, at 7:57 AM, Tim Düsterhus wrote:
> (1) How does the set() hook interact with #[\SensitiveParameter]?
Internally, the hook is a method, so it should respect it the same way as any
other method. Viz, you should be able to write:
```
public string $foo {
set ($[\SensitiveParameter] string $value) {
$field = 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 include
> 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
> 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 single
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
> 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 nice to
> clarify (unless I missed it)
Conceptually, I believe it means set(TypeFromProp $value). Since at present
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 so.)
> - function setFullName => ucfirst is not found in the "set" version
Yes it is. Look at the end of the `set` line.
> - could parent::$x::set($x) be replaced by parent::$x = $x ? Same for
> the get variant? Why not if not?
There's a subtle but important distinction here. parent::$x would suggest
accessing the parent *property*, ie, its backing value, directly. What is
actually intended here is accessing the parent *hook*, ie, its equivalent to
parent::setFoo($foo). The property itself is already available as $field.
We're open to alternate syntaxes for accessing the parent accessor syntax 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 unset the
cache. In the end we decided that there were too many nooks and crannies to
readonly to try and make sense of. (The more I use it, the more I think
readonly was a mistake and we should have gone straight to 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 => $field ??= $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 important and
useful feature. :-) )
What a readonly flag on a set-only property means... I have no idea.
> - 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 Reflection
API, which tends to have both getFoo(): object and getFoos(): array options.
It's no more redundant than the rest of Reflection.
> - What does ReflectionMethod::getName return on a hook? And getClosure?
Either "get" or some hashed string that includes "get". I'm not sure off 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 hooks.
> 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 the hood
are just methods like anything else. And as Tim noted above, there are use
cases like #[SensitiveParameter] for attributes even down here.
> - 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 (array) /
> get_mangled_object_vars() / get_object_vars() shouldn't call any "get".
I don't follow. json_encode() is on the list.
> I have a bigger concern: the take on references contradicts with the intro
> 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 => Error when trying
> to get-by-ref. This could also help solve the $foo->array[] case (including
> 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 dance 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 virtual property?
Moreover, in practice, getting a reference to a property is extremely rare,
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 comfortable just
leaving it at is. In an earlier version of the RFC we had a way for properties
to just disable references without adding any hooks, 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
> limitations for arrays (same as for list properties in Python) or forbid
> the use of hooks for array properties? I'm leaning towards accepting the
> limitations, like Python does. Then it is up to the API designer to
> 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 hooks on arrays doesn't
really buy us anything but more engine code. So yes, Python-style "just deal
with it *sunglasses*" is the most logical approach.
On Tue, May 9, 2023, at 12:38 PM, Benjamin Außenhofer wrote:
> An error maybe, the "class C" example in "Detailed Proposal > set" uses 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 and why
> its not recommended to be used. Is $field a reference? or does the "compile
> time macro" part mean its replaced at compile time? Ifso, this feels
> different to anything else PHP, i am leaning towards $this->propertyName 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-paste 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 = someLogic($value)" applied to a dozen different properties; it
wouldn't work if it was "$this->specificProperty = someLogic($value)".
4. We're used to our eyes glossing over "$this->propName", as it's so common.
Having a separate name to mentally scan for to determine if a property 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. "Compile
time macro" means it translates to the same AST as if you'd used
$this->propName. There's precedent for that. Constructor Property Promotion
works basically the same way.
--Larry Garfield
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
