[RFC][Vote] Property Hooks

6 months ago by Rowan Tommins [IMSoP] — view source

unread

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'm somewhat conflicted on this one. On the one hand, I think the
feature will be very powerful, and it's clear a lot of effort has been
put into designing something that fits with the language. On the other
hand, however, I share the concerns some have expressed that it is a
very complex proposal.

I would have more enthusiastically supported one which left out a few
"bells and whistles", or moved them to Future Scope to be polished and
agreed separately. I hope I was consistent in expressing that during the
discussion phase.

For that reason, please consider this an "abstention": while I'm
reasonably happy for the RFC as written to pass, I am not going to cast
a Yes vote myself.

Regards,

--
Rowan Tommins
[IMSoP]

6 months ago by Larry Garfield — view source

unread

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.

The vote has now been closed. The final tally is 42 Yes and 2 No, meaning it has passed.

Thank you everyone for your participation and feedback. Ilija will try to get the PR merged ASAP.

--Larry Garfield

6 months ago by Bilge — view source

unread

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.
The vote has now been closed. The final tally is 42 Yes and 2 No, meaning it has passed.

Thank you everyone for your participation and feedback. Ilija will try to get the PR merged ASAP.

--Larry Garfield

Finally, a reason to upgrade this Winter! Congrats on the successful
RFC, Larry and Ilija, et al.

Cheers,
Bilge

5 months ago by Nikita Popov — view source

unread

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

5 months ago by Lanre — view source

unread

Personally I still think we should have used TS/JS syntax for computed
properties. PHP is much closer to typescript than Java/C# and we already
have support for modifiers. Slightly more verbose but these issues would be
non-existent.

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

5 months ago by Valentin Udaltsov — view source

unread

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.

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

Correct me if I'm wrong, but here's how I understand the short-hand
notation in setter hook in combination with
https://wiki.php.net/rfc/short-functions:

final class Foo
{
    // Before
    public string $x {
        set {
            $this->x = $this->transformX($value);
        }
    }

    private function transformX(string $value): string => trim($value);

    // After
    public string $x {
        set => trim($value);
    }
}

So it's kinda consistent if you say that only the right side of the
property assignment is replaced with short-hand notation, not the whole
assignment. And then $this->x = is implicit.

--
Regards,
Valentin

5 months ago by Larry Garfield — view source

unread

__

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.

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.

I'm a little surprised to hear you say that, since you voted against short-functions at the time. :-) I would love to try again on those if there's enough change in opinion to make it worthwhile, as I do think it's useful.

The reason we included short-set, aside from consistency, is that people really seemed to want a "return to assign" behavior, and that was the most logical way to do it. It's also more compact for when a set hook is inlined into a promoted constructor property, which I suspect will be not uncommon. For "validate or error" use cases, a ternary isn't ideal but it will get the job done, and there are several examples of that in the RFC.

If we find that "validate but don't modify" is an especially common case, then we should look into a more dedicated syntax for property Constraints / Guards. (I have some ideas there, but nothing ready for public consumption.)

I don't think the current hook syntax would preclude short-functions generally, though. The syntax is the same, the behavior is the same, all that's really different from the typical case is the extra "and the thing it evaluates to gets assigned." Which is exactly what several people asked for. So I don't see how it is really going to cause a conflict.

(Side note: If anyone has warmed to short-functions since it was last brought up let me know. I have no idea how to gauge if the zeitgeist has changed since then to make another attempt worthwhile.)

Regards,
Nikita

Correct me if I'm wrong, but here's how I understand the short-hand
notation in setter hook in combination with
https://wiki.php.net/rfc/short-functions:
final class Foo
{
    // Before
    public string $x {
        set {
            $this->x = $this->transformX($value);
        }
    }

    private function transformX(string $value): string => trim($value);

    // After
    public string $x {
        set => trim($value);
    }
}
So it's kinda consistent if you say that only the right side of the
property assignment is replaced with short-hand notation, not the whole
assignment. And then $this->x = is implicit.

Essentially that, yes.

--Larry Garfield