Hi Ilija,
On 12-4-2024 1:00, Ilija Tovilo wrote:
On 9-4-2024 16:03, Juliette Reinders Folmer wrote:
I realize it is late in the discussion period to speak up, but for months I've
been trying to find the words to express my concerns in a polite and
constructive way and have failed.
First of all, thank you for taking the time to analyze the RFC and
form your thoughts. I know how much time and effort it takes.
Nonetheless, I hope you understand that receiving feedback of this
sort literally minutes before a vote is planned is close to the worst
way to go about it, both from a practical standpoint, but also RFC
author and reviewer morale.
Many of the points you raise have been there for many months to a
year, and many have been raised and discussed many times over.
Thank you for your extensive response.
No matter how much time I spend trying to voice my concerns properly, I
clearly still failed.
My email was not a criticism of the work on property hooks as such. It
was, more than anything, an opinion piece.
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.
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.
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.
[*] 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.
As for [1] and [4], let me respond to your answers to those points:
[1] Additionally, it proposes for isset() to throw an Error for virtual
properties without a get hook. This is surprising behaviour as isset() by its
nature is expected to *not* throw errors, but return false for whatever is not
set or inaccessible from the current context: https://3v4l.org/3OlgM
Personally, I would rather know when checking `isset` on something
that can never return `true`. But I admit that this case is not
clear-cut. The throwing approach can be relaxed to return false
without a BC break though, but not the opposite.
In my opinion, a plain call to `isset()` should never have to be put in
a try-catch, but I understand your point about being conservative now
and being able to relax the approach later without BC-break.
I would definitely be in favour of relaxing the approach to make
`isset()` return false, but let's see what other have to say about it.
[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
Oh, and hang on, they *can* be read from a method in the same class as long as
that method is called from within the set hook, so now the method will need to
either do a backtrace or use Reflection to figure out whether it has access to
the property value.
Right. We use the same mechanism as `__get`/`__set` uses to protect
against recursion. In particular, when entering a magic method for a
particular property, PHP remembers this using a "property guard". It's
really just a marker in the object that sets the magic method type,
along with the property name.
When the same property is accessed again, the magic method or hook is
skipped, and PHP behaves as if the magic method or hook wasn't there
at all. For `__get`, `__set` and hooks of virtual properties, this
means accessing a dynamic property. For backed properties, this means
accessing its backing value.
After discussing it, we've decided to make virtual properties slightly
less surprising by throwing, instead of trying to create a dynamic
property. This would be consistent with how virtual read-, and
write-only properties are handled outside of hooks, when they are
written to or read, respectively.
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) ?"
But I see you've already answered that question by changing the
behaviour to throw now. ;-)
Now, as for the impact on PHP_CodeSniffer:
And not just one syntax, but five and the differences in the semantics of the
function syntaxes are significant.
Notably, `get` and `set` hooks don't actually have a different
grammar. Instead, hooks have names, optional parameter lists, and an
optional short (`=>`) or long (`{}`) body. Whether all possible
combinations are considered different syntax is open to
interpretation. It's true that additional rules apply to each hook,
e.g. whether they are allowed to declare a parameter list, and how the
return value is used. I could see an argument being made for allowing
an empty parameter list (`()`) for `get`, for consistency. Let us know
if you have any other ideas to make them more congruent.
In any case, I don't believe PHP_CodeSniffer would need to handle each
combination separately.
For the purposes of PHP_CodeSniffer, having all those different
variations of the syntax does actually make things exponentially more
complex.
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.
The Tokenizer layer in PHPCS will need extensive changes to accommodate
recognizing `set` and `get` as "function" keywords, but only when used
in the context of a property declaration.
Though, hang on, not only in the context of a property declaration, also
in the context of a function parameter declaration in the case of
constructor property promotion.
Next PHPCS will hit another problem (still in the Tokenizer layer),
which is that "function" keywords are expected to get markers for the
parentheses, which is now a problem.
Similarly, "function" keywords (for non-abstract, non-interface
functions) are expected to have markers for the start and end of the
scope, but PHPCS searches for those starting from the close parenthesis....
Only once all that is solved and stable, can we even start to look at
the sniffs themselves.
Let's take as an example a sniff trying to examine the contents of a
function. Such a sniff will typically first check if the function has a
body (based on the scope markers) and bow out if not, then gather the
declared parameters and associated information from between the
parentheses and then examine the contents of the function between the
curlies (or between the => and the "guessed" end of the expression for
arrow functions (as this is still not 100% solved)) for whatever the
sniff is looking for.
Now, for the property hooks, this would mean each of these sniffs will
need changes along the following lines:
- the hook function keywords would need to be added to indicate the
sniff should examine property hooks as well
- the "gathering of the parameters" part is broken as the hook function
may not have parentheses, which would typically be seen by sniffs as
indicative of use in an IDE (live coding) or a parse error and would be
a reason to bow out, so special handling would need to be put in place
for this.
- but even then the "gathering of the parameters" is broken as if there
are no parentheses, the `set` hook will automatically get the `$value`
parameter, but none of the other info about the parameter is available,
like the type, so now the sniff will need to examine the property itself
to figure out the type of `$value`.
- etc
Now, all of this is, of course, solvable, but it will take years to
update all actively used sniffs to account for all these different
situations.
What would your optimal solution look like? I feel this discussion
cannot progress unless we have more concrete discussion points.
From a PHPCS point of view, things would be much more straight forward
if the hooks would always take parentheses, the `set` hook would always
need a declared parameter, return types were allowed and only the syntax
with curlies was supported.
That way the syntax matches the pre-existing function syntaxes much more
closely.
And while writing this, I already know that limiting the RFC to only the
full syntax + parentheses and return types will be extremely unpopular
among the readers here, so **kind request to people here: no need to
flame me**. I know. I'm only providing an answer to the question posed,
I don't expect the RFC to be changed to accommodate PHPCS.
And last but not least, there is the ability to declare property hooks in
constructor property promotion ... where we can now basically have a function
declaration within the signature of a method declaration.... with all five
possible syntax types.
The grammar used for hooks in constructor property promotion is the
same as the one used for properties. Am I wrong to assume this
shouldn't cause any additional work for PHP_CodeSniffer?
Yes, your assumption is unfortunately wrong. See above.
I won't comment too much on this, because it's your area of expertise.
Of course, I sympathize. If there's a solution that works well for us
_and_ you, let me know. But it doesn't sound like some simple tweaks
can significantly improve this problem for you.
Agreed. But just to clarify: please don't see what I highlighted
regarding PHP_CodeSniffer as a "me" problem. This problem will affect
all _users_ of PHP_CodeSniffer who want to use the new syntax for years
to come.
To summarize: I understand your concerns and why you raise them.
However, none of the complexity in this RFC is arbitrary. Most of it
is inherent in the problem space, and the RFC’s approach is the result
of extensive experimentation to determine possible solutions. While
some superficial changes are possible, the actual complex parts
(reference handling, inheritance, interfaces, virtual properties,
etc.) are all necessary complexity, not accidental complexity. Absent
a novel alternative, which after over a year of work and extensive
discussions on this list and elsewhere we do not believe exists, we
believe this RFC represents "the best hooks implementation that PHP
can support." And that implementation deserves a final vote to
determine if the internals community wants hooks at all or not.
I fully agree. It does deserve a final vote and the main thing I was
trying to do with my mail was add another perspective to inform voters.
I realize it may not be a popular perspective and that was part of why I
struggled so much writing the email, as I know how much thought you both
have put into this and I so very much respect all that attention to
detail and all the careful considerations you have both made. It pains
me to know how much work you and Larry have put into this feature,
especially as I really kind of like the _idea_ of the feature.
Having said that, sometimes one has to take a step back and look at the
wider picture and ask oneself if something solves the original problem
or exacerbates it.
Unfortunately, my conclusion was the latter. We'll see via the vote,
what the conclusion drawn by the voters is.
With all my regards,
Juliette
