Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123124
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-379-gabd37849b7-fm-20240408.001-gabd37849
Precedence: bulk
MIME-Version: 1.0
Message-ID: <4f5865b5-6ffd-40f2-aa6e-756b8e3bb6f7@app.fastmail.com>
In-Reply-To: <6618A2B1.70501@adviesenzo.nl>
References: 
 <CAPyj-LBMQEJ=2BF=xk_on4O2xTU+MM2XRAZ5g=uPwVOf=bh+sw@mail.gmail.com>
 <66154AA0.1040905@adviesenzo.nl> <66160A1D.4060409@adviesenzo.nl>
 <CAPyj-LDFKWut3cg0NzKAKZ6XFRnMd4YbC5TM2Fvb6Jop4mBB6g@mail.gmail.com>
 <6618A2B1.70501@adviesenzo.nl>
Date: Fri, 12 Apr 2024 16:25:53 +0000
To: "php internals" <internals@lists.php.net>
Subject: Re: [PHP-DEV] [RFC][Vote announcement] Property hooks
Content-Type: text/plain
From: larry@garfieldtech.com ("Larry Garfield")

On Fri, Apr 12, 2024, at 2:55 AM, Juliette Reinders Folmer wrote:

> The first part of my email (up to the "Furthermore") was mostly me 
> trying to summarize the complexity of the proposal and to compare it to 
> existing syntaxes and list those things which - to me - appeared 
> inconsistent with other PHP features. It was not intended as criticism 
> on individual choices made and I'm not sure whether, if I were 
> designing this feature, I would have made different choices, other than 
> to have just one syntax - the "full" syntax (with a `get();`/`set();` 
> variant - yes, always with parentheses - for abstract/interface 
> properties).
>
> This first part was meant as a summation to highlight how incredibly 
> complex this new feature is and how many opportunities it offers 
> developers to shoot themselves in the foot.
> Let alone, how many problems this can cause when developers use classes 
> in dependencies which use this new feature and they have limited to no 
> awareness of whether they are using virtual or backed properties on the 
> dependency.

The RFC itself is complex because PHP is complex, and the problem space is complex.  For instance, when hooks exist, what does that do for the return value of `=`?  Most devs probably don't even remember that there is one, but as a language design element it has to be considered.

Overall, the guiding design principle for this RFC was to make it as transparent as possible, so that devs who "use classes in dependencies which use this new feature and they have limited to no awareness of whether they are using virtual or backed properties on the dependency" do not need to care.  The whole point is that it should be a transparent change, or as transparent as possible.  Most of the complexity of the RFC is figuring out what the most transparent behavior would be, and ensuring that works.

As indicated in the introduction, a primary use case for hooks is to not need to use them at all; 90%+ of all Doctrine entities I've seen have dumb boilerplate do-nothing get/set methods, placed there as a "just in case we need it" or "because That's How It's Done In Java Beans."  Occasionally they do a little extra set validation, but not in the majority case.

Hooks allow eliminating that "just in case" boilerplate.  If hooks get almost no usage in the wild, but the option to use them eliminates hundreds of thousands of lines of effectively no-op getX() methods, I will consider it a resounding success.

> As the proposal purports to be a better alternative to the magic 
> `_get()`/`__set()` methods, I would expect it to prevent a lot of the 
> problems associated with those methods. However, what I found - and 
> what you seemingly confirmed in your reply - is that this new feature 
> does *not* in actual fact solve the problems magic methods have. It has 
> the same problems + more problems associated with the new feature 
> itself.
>
> Part of my concern is therefore that this feature adds significant 
> extra complexity to the engine, for little to no real benefit, other 
> than to have a nicer syntax for creating the same problems we already 
> had before.

I believe this is a highly disingenuous description of the RFC.

* Hooks are automatically static-analysis-friendly.  __get is not.
* Hooks are automatically IDE autocomplete-friendly.  __get is not.
* Hooks provide built-in type enforcement.  __get does not.
* Hooks allow properties to be self-documenting.  __get does not.
* Hooks avoid smooshing multiple different virtual properties into a single mega-method.  __get requires that.
* Hooks allow opting-in to reference semantics per-property.  __get is all-or-nothing.
* Hooks have clear, consistent integration with serialization (in that the behavior with each serialization mechanism is consistent with that mechanism's expectations).  __get doesn't interact with serialization at all, leaving you to do everything manually.
* Hooks allow properties to be declared without special behavior, and add special behavior later without a BC break.  __get... only kinda sorta does that, if you use it just right.
* Hooks cleanly allow pre-defining properties to get the substantial memory savings of well-defined properties.  __get may or may not, depending on how you impelement it.

From a usability perspective, hooks are vastly superior to using __get/__set directly.

However, both __get/__set and hooks operate in the same problem space: Inserting extra user-space code in between what otherwise seems like a boring variable read/write.  *That* concept naturally involves complexity, by definition, and has lots of edge cases, by definition.  Any attempt to insert user-space code into that operation would run into the exact same set of complexities; References exist, we have to deal with them somehow.  `=` evaluates to a value, we have to deal with that somehow.  Inheritance exists, we have to deal with that somehow.  isset() exists, we have to deal with that somehow.  This is all essential complexity.

In most cases, we chose to address issues in the same way that __get/__set do precisely to keep it as simple as possible and reduce cognitive overhead for developers.  The corner cases where the code insertion cannot be perfectly transparent already exist with __get/__set, and we therefore defaulted to "make them not-quite-transparent in the same way it's already not-quite-transparent," precisely to minimize the amount of thinking developers have to do.

> In that sense, my email was not to ask you to make changes to the 
> proposal [*], but far more to try and make sure that those people with 
> the power to vote and without the time to fully read and really grep 
> the (really really long) proposal, are put in a position to make an 
> informed choice.

Given the extensive feedback the RFC has gotten, both on list and off, I am confident that those who care to understand the level of complexity already do.

> [*] The part after the "Furthermore" did contain some criticism, where 
> my points marked with [2] and [3] are AFAICS just a matter of 
> clarifying things in the RFC, while [1] and [4] would possibly warrant 
> changes, though I would expect (hope) the impact of those in dev time 
> to be small.
> Oh and yes, my remark about `var` would also warrant a change in the 
> RFC text.
>
> I can see that those textual changes in the RFC have been made in the 
> mean time, so thank you for adding the additional clarifications for 
> those points to the RFC.

And we appreciate you calling out those inconsistencies.  We just wish you had done so 6 weeks ago, when it could certainly have been done in a polite and constructive way, as you said you wanted to do.

>>> [4] Why should ReflectionProperty::getRawValue() throw an error for static properties ? Instead of returning the value, identically to ReflectionProperty::getValue() ? This is surprising to me and I see no reason for it.
>> Because it's equivalent to `getValue`, and indicates the user is doing
>> something they don't fully understand. 
> Except that it isn't equivalent behaviour, as `getValue()` will return 
> the value for both static and non-static properties without any 
> problems.
> See: https://3v4l.org/v4F0S which is based on Example #1 in the 
> documentation of 
> https://www.php.net/manual/en/reflectionproperty.getvalue.php

Static properties have no concept of virtual or backed values, so anyone calling getRawValue() on a static property is already doing something illogical.  That said, if in practice we find this is problematic it is trivial to relax the throw to it being a pure alias of getValue() in the future; tightening it up in the future would be a BC break.

> Forgive me, but I had to chuckle at the mention of virtual properties 
> needing to access a dynamic property in the context of a property guard.
> My thoughts went straight to the following question "Does this mean 
> that support for dynamic properties could never be removed if this RFC 
> gets voted in ? And if so, can we please get rid of the deprecation 
> notice (and need for the attribute) ?"

As noted above, this was simply "what do __get/__set do?", which in some cases would result in a dynamic property.   Whether that is wise or not is a different question.  Based on this thread we've decided that there are no good use cases for this pathway, so we're going to deviate from __get/__set in this instance.

This is the text I just added to the RFC (in the Scoping section):

If a hook calls a method that in turn tries to read or write from the property again, that would normally result in an infinite loop. To prevent that, accessing the backing value of a property from a method called from a hook on that property will throw an Error. That is somewhat different than the existing behavior of __get and __set, where such sub-called methods would bypass the magic methods. However, as valid use cases for such circular logic are difficult to identify and there is added risk of confusion with dynamic properties, we have elected to simply block that access entirely. 


> I'll try to give you some idea, though I fear this is not really PHP 
> Internals mailinglist material as it is a bit too detailed and too 
> specific to one tool.

I am going to skip the weeds of the PHPCS implementation, and just ask you, and everyone, to provide this kind of feedback on future RFCs early, not at the literal last minute when everything was already polished.  If there are small tweaks to the syntax that would have made user-space meta tools (formatters, SA tools, etc.) substantially easier, that's something we would have been open to hearing 8 weeks ago, or a year ago when this was first proposed.  Addressing it now would require non-trivial design work from us, non-trivial implementation changes from Ilija, and non-trivial discussion and review from the half-dozen or so other reviewers that have taken an active interest in this RFC (which we very much appreciate!).  Trying to do that now would burn several weeks more time, with only one person involved being paid for it (Ilija works for the Foundation), and like every RFC it's all still spec work.

Even if you're not sure how to make it "polite", provide that kind of feedback early and often, in bite-sized pieces, so that it can save everyone's time, including yours.

This is a major structural failing of the current RFC process, something many have pointed out already.  It desperately needs to be addressed.  Until it does, though, there are ways we can make the process at least a little less painful for all involved.

--Larry Garfield