[RFC][Vote announcement] Property hooks

php.internals

Ilija Tovilo

2 years ago
Hi everyone Heads-up: Larry and I would like to start the vote of the property hooks RFC tomorrow: https://wiki.php.net/rfc/property-hooks We have worked long and hard on this RFC, and hope that we have found some middle-ground that works for the majority. One last concern we have not officially clarified on the list: https://externals.io/message/122445#122667
>> I personally do not feel strongly about whether asymmetric types make it into the initial implementation. Larry does, however, and I think it is not fair to exclude them without providing any concrete reasons not to. [snip] > > My concern is more about the external impact of what is effectively a change to the type system of the language: [snip] will tools like PhpStan and Psalm require complex changes to analyse code using such properties?
In particular, this paragraph is referencing the ability to widen the accepted $value parameter type of the set hook, described at the bottom of https://wiki.php.net/rfc/property-hooks#set. I have talked to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that this should not be complex to implement in PHPStan. In fact, PHPStan already offers the @property-read and @property-write class annotations, which can be used to describe "virtual" properties handled within __get/__set, already providing asymmetric types of sorts. Hence, this concern should be a non-issue. Thank you to everybody who has contributed to the discussion! Ilija

Unnamed Person

2 years ago
On 8-4-2024 23:39, Ilija Tovilo wrote:
> Hi everyone > > Heads-up: Larry and I would like to start the vote of the property > hooks RFC tomorrow: > https://wiki.php.net/rfc/property-hooks > > We have worked long and hard on this RFC, and hope that we have found > some middle-ground that works for the majority. One last concern we > have not officially clarified on the list: > > https://externals.io/message/122445#122667 > >>> I personally do not feel strongly about whether asymmetric types make it into the initial implementation. Larry does, however, and I think it is not fair to exclude them without providing any concrete reasons not to. [snip] >> My concern is more about the external impact of what is effectively a change to the type system of the language: [snip] will tools like PhpStan and Psalm require complex changes to analyse code using such properties? > In particular, this paragraph is referencing the ability to widen the > accepted $value parameter type of the set hook, described at the > bottom of https://wiki.php.net/rfc/property-hooks#set. I have talked > to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that > this should not be complex to implement in PHPStan. In fact, PHPStan > already offers the @property-read and @property-write class > annotations, which can be used to describe "virtual" properties > handled within __get/__set, already providing asymmetric types of > sorts. Hence, this concern should be a non-issue. > > Thank you to everybody who has contributed to the discussion! > > Ilija >
Ilija, Heads-up: I'm still writing up an opinion and intend to send it to the list before end of day (CET). I know I'm late to the party, but I've been having trouble finding the words to express myself properly regarding this RFC (and have been struggling to find the right words for months). Smile, Juliette

Unnamed Person

2 years ago
On 9-4-2024 16:03, Juliette Reinders Folmer wrote:
> On 8-4-2024 23:39, Ilija Tovilo wrote: >> Hi everyone >> >> Heads-up: Larry and I would like to start the vote of the property >> hooks RFC tomorrow: >> https://wiki.php.net/rfc/property-hooks >> >> We have worked long and hard on this RFC, and hope that we have found >> some middle-ground that works for the majority. One last concern we >> have not officially clarified on the list: >> >> https://externals.io/message/122445#122667 >> >>>> I personally do not feel strongly about whether asymmetric types make it into the initial implementation. Larry does, however, and I think it is not fair to exclude them without providing any concrete reasons not to. [snip] >>> My concern is more about the external impact of what is effectively a change to the type system of the language: [snip] will tools like PhpStan and Psalm require complex changes to analyse code using such properties? >> In particular, this paragraph is referencing the ability to widen the >> accepted $value parameter type of the set hook, described at the >> bottom ofhttps://wiki.php.net/rfc/property-hooks#set. I have talked >> to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that >> this should not be complex to implement in PHPStan. In fact, PHPStan >> already offers the @property-read and @property-write class >> annotations, which can be used to describe "virtual" properties >> handled within __get/__set, already providing asymmetric types of >> sorts. Hence, this concern should be a non-issue. >> >> Thank you to everybody who has contributed to the discussion! >> >> Ilija >> > > Ilija, > > Heads-up: I'm still writing up an opinion and intend to send it to the > list before end of day (CET). I know I'm late to the party, but I've > been having trouble finding the words to express myself properly > regarding this RFC (and have been struggling to find the right words > for months). > > Smile, > Juliette
Later than intended, but here goes.... If there is one RFC which has been giving me nightmares since I first heard of it, it's this one. 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. I am going to try now anyway (before it is too late), so please bear with me. Also, as I'm not a C-developer, please forgive me if I get the internals wrong. I'm writing this from a PHP-dev/user perspective, with my perspective being heavily influenced by my role as maintainer of PHP_CodeSniffer. --- TL;DR: this RFC tries to do too much in one go and introduces a huge amount of cognitive complexity with all the exceptions and the differences in behaviour between virtual and backed properties. This cognitive complexity is so high that I expect that the feature will catch most developers out a lot of the time. --- I can definitely see the use case and desirability of the property hooks functionality proposed in the RFC and compared to the initial RFC I read last year, the current RFC is, IMO, much improved. Huge kudos to Ilija and Larry for all the work they have put in to this! I applaud the intention of this RFC to make it easier to avoid the magic __get()/__set() et al methods. What I have a problem with is the implementation details. Over the last few years, we've seen a movement to get rid of more and more of the _surprising_ behaviour of PHP, with subtle exceptions being deprecated and slated for removal and (most) new syntaxes trying to use the principle of least surprise by design. This RFC, in my view, is in stark contrast to this as it introduces a plethora of exceptions and subtle different behaviour in a way that will catch developers out for years to come. At this moment (excluding property hooks), from a user perspective, there are three different function syntaxes in PHP: named functions (and methods), anonymous functions and arrow functions. The semantics of these are very similar with subtle differences: * Can be static or non-static. * Take parameters, which can be typed, references, variadic, optional etc. * Can have a return type. * Can return by reference. * Have a function "body". The differences between the current syntaxes - from a user perspective - are as follows: = Named functions: * When declared in a class, can have visibility attached, can be abstract, can be final. * When declared in an interface or declared as abstract, will not have a function "body". = Anonymous functions: * Can import plain variables from outside its scope with a `use()` clause. * Are declared as an expression (can be assigned to a variable etc). = Arrow functions: * Have access to variables in the same scope. * Are declared as an expression. * Body of the function starts with a => instead of being enclosed in curlies and can end on a range of characters. * Can only take one statement in the body. * Automagically returns. The property hooks RFC introduces a fourth flavour of function syntax. And not just one syntax, but five and the differences in the semantics of the function syntaxes are significant. The differences in semantics I see for "full" property hook functions compared to other function syntaxes are as follows: * `get` cannot take parameters (and doesn't have the parentheses typically expected for a function declaration). * `get` cannot take return type (inherits this from the property, which is logically sound, but does create a syntactic difference). * `set` can take one (typed or untyped) parameter. * `set` cannot take return type (silently set to void, which is logically sound, but does create a syntactic difference). * `set` magically creates a $value variable for the "new" value, though that variable _may_ have an arbitrary different name depending on whether or not the parameter was explicitly specified. * Has a function body. Then there are multiple short hand syntaxes, which each have yet more syntactic differences: * The arrow function variant for `get` with all the above differences + the differences inherent to arrow functions, combined, with the exception of the access to variables in the same scope and a more clearly defined end of the expression. * The implicit "set" parameter, which does not have a explicit parameter, does not have parentheses and has the magically created $value variable. * The arrow function variant for `set` with all the above differences + the differences inherent to arrow functions with the above mentioned exceptions + the implicit assignment, which breaks the expected behaviour of arrow functions by assigning the result of the expression instead of returning it (totally understandable, but still a difference). * The abstract/interface variants, which don't have a function body. Next there are the differences in semantics which are now being introduced for properties: * Aside from the hooks themselves.... * Properties _may_ or _may not_ have a default value anymore, depending on whether the hooks cause it to be a backed or a virtual property. * Properties with hooks can _be_ readonly, but cannot be declared as readonly. And then there are the new features for properties (not just hooked ones): * Properties can now be declared as final. * Properties can now be declared as abstract, but only with explicit hook requirements, otherwise the abstract keyword is not allowed. * Properties can now be declared on interfaces, but only with explicit hook requirements, otherwise they are not allowed. * Abstract/Interface properties don't comply with the same covariance/contravariance rules as other properties 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. Additionally, when reading the RFC (in its current state), I see the following exceptions being put in place, which impact the semantics for properties: * Only available for object properties, static properties are excluded. * `var` for property declarations is not supported according to the RFC. While I 100% agree using the var keyword is old-school, using `var` effectively makes a property a public property (which would satisfy the visibility requirement for an interface/abstract class) and the `var` keyword is *not* deprecated, even though the RFC states it is: https://3v4l.org/3o50a I'd fully support formally deprecating the `var` keyword for properties and eventually removing it, but not supporting it for this RFC, even though it is still a supported language feature seems opinionated and arbitrary. Note: when testing the RFC code, declaring a property using the var keyword on an interface does not currently throw an error (while if it is not supported, I would expect one): https://3v4l.org/KaLqe/rfc#vrfc.property-hooks , same goes for using the var keyword with property hooks in a class: https://3v4l.org/mQ6pG/rfc#vrfc.property-hooks * Disallows references to properties, but only when there is a set hook. * Disallows indirect modification of a property, but only when there is a set hook. * Write-only (virtual) properties, which cannot be read and you need to study the hook declaration in detail to figure out if a property is or is not write-only. 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 (now why does that remind me of the magic __...() methods ?). * Readonly properties which are not denoted as readonly, but still are due to their virtual nature (get without access to $this->prop), which can only be figured out by, again, studying the contents of the hook function. * Whether a type can be specified on the parameter on `set` depends on whether the property is typed. You cannot declare `set(mixed $value)` for an untyped property, even though it would effectively be compatible. This is inconsistent with the behaviour for, for instance method overloads, where this is acceptable: https://3v4l.org/hbCor/rfc#vrfc.property-hooks , though it is consistent with the behaviour of property overloads, where this is not acceptable: https://3v4l.org/seDWM (anyone up for an RFC to fix this inconsistency ?) * Changes the meaning of `$this->property` read/write access for all methods called from within a hook while executing the hook. * Creates two different flavour of properties: "backed" properties and "virtual" properties with significantly different behaviours, raising cognitive complexity. This includes, but is not limited to the fact that set hooks can be bypassed on virtual properties via a get reference. * The range of different behaviours for various forms of serialization. Furthermore: [1] The RFC also states in "Interaction with isset() and unset()" that the behaviour of property hooks is consistent with how `isset()` interacts with `__get()` today. This is not true, as in: the behaviour as described for isset with magic methods is different than what the RFC states it is: https://3v4l.org/2Arkh/rfc#vrfc.property-hooks 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 [2] PHP supports "multi-property declarations", but I see no mention of these in the RFC, which makes it unclear if and if so, how property hooks would work with multi-property declarations. class Foo { public string $bar, $baz, $bab; } [3] If a `set` hook gets a named parameter, is the magic `$value` variable still available or not ? I'd expect not, but it is unclear from the RFC. [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. All in all, I really do see the use case for this feature and I most definitely appreciate all the work Ilija and Larry have put into the RFC and the implementation. However, in its current state, I'm concerned that the feature introduces so many exceptional situations and WTF moments that it is a worse solution than the magic methods which are already available. (and no, I'm definitely not a fan of those magic methods either). I think property hooks as currently proposed with all its catches will be hard to teach, will raise the barrier of entry to the language, can catch people out in dozens of different ways and introduces way too many peculiarities, while the tendency of the language has been to try to eliminate those kind of surprising behaviours. Irrelevant side-note: I kind of feel like I could create a completely new "Why Equal doesn't Equal" Quiz just and only based on property hooks and people will still not get it afterwards. --- As for the impact on static analysis tools.... I can't speak for other tools, but I can tell you that for PHP_CodeSniffer the impact of this RFC will be horrendous. Aside from the impact on the syntax recognition layer (Tokenizer), my rough estimate is that about 70% of all sniffs ever written will need to be adjusted for these five syntaxes and all the peculiarities around them - either to ignore hooks, work around hooks, skip over hooks, or to take hooks into account and examine them in all their variations. I estimate it will take a good 5-6 years before all actively used sniffs will have been adjusted and that in the mean time, maintainers of both PHPCS itself as well as the popular external standards will have to deal with a significant number of extra support requests/bug reports related to this. The sheer amount of new syntaxes which have been introduced starting with PHP 7.4, has already brought innovation in a large part of the PHP_CodeSniffer field to a near complete halt for the past four years as I'm continuously trying to play catch-up. This RFC will mean that innovation, like new PSR-5/19 Docs standards, QA sniffs for tests, significantly improving the Security standard etc, will all be put on hold for another few years while I have to deal with this change. While I fully recognize that the impact on userland tooling is not a reason to reject a new PHP feature, nor should it be, I do believe that the impact of this RFC could be far less and easier to mitigate if different design choices were made or if the new syntaxes were introduced in a far slower, more staged approach across multiple PHP versions. Smile, Juliette P.S.: nitpick: in the first two code examples under "Detailed Implementation" - "Set" the `$u` variable is missing the dollar sign for the `new User` line.

Lynn

2 years ago
On Wed, Apr 10, 2024 at 5:47 AM Juliette Reinders Folmer < php-internals_nospam@adviesenzo.nl> wrote:
> On 9-4-2024 16:03, Juliette Reinders Folmer wrote: > > On 8-4-2024 23:39, Ilija Tovilo wrote: > > Hi everyone > > Heads-up: Larry and I would like to start the vote of the property > hooks RFC tomorrow:https://wiki.php.net/rfc/property-hooks > > We have worked long and hard on this RFC, and hope that we have found > some middle-ground that works for the majority. One last concern we > have not officially clarified on the list: > https://externals.io/message/122445#122667 > > I personally do not feel strongly about whether asymmetric types make it into the initial implementation. Larry does, however, and I think it is not fair to exclude them without providing any concrete reasons not to. [snip] > > My concern is more about the external impact of what is effectively a change to the type system of the language: [snip] will tools like PhpStan and Psalm require complex changes to analyse code using such properties? > > In particular, this paragraph is referencing the ability to widen the > accepted $value parameter type of the set hook, described at the > bottom of https://wiki.php.net/rfc/property-hooks#set. I have talked > to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that > this should not be complex to implement in PHPStan. In fact, PHPStan > already offers the @property-read and @property-write class > annotations, which can be used to describe "virtual" properties > handled within __get/__set, already providing asymmetric types of > sorts. Hence, this concern should be a non-issue. > > Thank you to everybody who has contributed to the discussion! > > Ilija > > > > Ilija, > > Heads-up: I'm still writing up an opinion and intend to send it to the > list before end of day (CET). I know I'm late to the party, but I've been > having trouble finding the words to express myself properly regarding this > RFC (and have been struggling to find the right words for months). > > Smile, > Juliette > > > Later than intended, but here goes.... > > If there is one RFC which has been giving me nightmares since I first > heard of it, it's this one. > > 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. > > I am going to try now anyway (before it is too late), so please bear with > me. Also, as I'm not a C-developer, please forgive me if I get the > internals wrong. I'm writing this from a PHP-dev/user perspective, with my > perspective being heavily influenced by my role as maintainer of > PHP_CodeSniffer. > > --- > TL;DR: this RFC tries to do too much in one go and introduces a huge > amount of cognitive complexity with all the exceptions and the differences > in behaviour between virtual and backed properties. This cognitive > complexity is so high that I expect that the feature will catch most > developers out a lot of the time. > --- > > I can definitely see the use case and desirability of the property hooks > functionality proposed in the RFC and compared to the initial RFC I read > last year, the current RFC is, IMO, much improved. > Huge kudos to Ilija and Larry for all the work they have put in to this! > > I applaud the intention of this RFC to make it easier to avoid the magic > __get()/__set() et al methods. What I have a problem with is the > implementation details. > > Over the last few years, we've seen a movement to get rid of more and more > of the _surprising_ behaviour of PHP, with subtle exceptions being > deprecated and slated for removal and (most) new syntaxes trying to use the > principle of least surprise by design. > > This RFC, in my view, is in stark contrast to this as it introduces a > plethora of exceptions and subtle different behaviour in a way that will > catch developers out for years to come. > > At this moment (excluding property hooks), from a user perspective, there > are three different function syntaxes in PHP: named functions (and > methods), anonymous functions and arrow functions. > > The semantics of these are very similar with subtle differences: > * Can be static or non-static. > * Take parameters, which can be typed, references, variadic, optional etc. > * Can have a return type. > * Can return by reference. > * Have a function "body". > > The differences between the current syntaxes - from a user perspective - > are as follows: > = Named functions: > * When declared in a class, can have visibility attached, can be abstract, > can be final. > * When declared in an interface or declared as abstract, will not have a > function "body". > > = Anonymous functions: > * Can import plain variables from outside its scope with a `use()` clause. > * Are declared as an expression (can be assigned to a variable etc). > > = Arrow functions: > * Have access to variables in the same scope. > * Are declared as an expression. > * Body of the function starts with a => instead of being enclosed in > curlies and can end on a range of characters. > * Can only take one statement in the body. > * Automagically returns. > > The property hooks RFC introduces a fourth flavour of function syntax. And > not just one syntax, but five and the differences in the semantics of the > function syntaxes are significant. > > The differences in semantics I see for "full" property hook functions > compared to other function syntaxes are as follows: > * `get` cannot take parameters (and doesn't have the parentheses typically > expected for a function declaration). > * `get` cannot take return type (inherits this from the property, which is > logically sound, but does create a syntactic difference). > * `set` can take one (typed or untyped) parameter. > * `set` cannot take return type (silently set to void, which is logically > sound, but does create a syntactic difference). > * `set` magically creates a $value variable for the "new" value, though > that variable _may_ have an arbitrary different name depending on whether > or not the parameter was explicitly specified. > * Has a function body. > > Then there are multiple short hand syntaxes, which each have yet more > syntactic differences: > * The arrow function variant for `get` with all the above differences + > the differences inherent to arrow functions, combined, with the exception > of the access to variables in the same scope and a more clearly defined end > of the expression. > * The implicit "set" parameter, which does not have a explicit parameter, > does not have parentheses and has the magically created $value variable. > * The arrow function variant for `set` with all the above differences + > the differences inherent to arrow functions with the above mentioned > exceptions + the implicit assignment, which breaks the expected behaviour > of arrow functions by assigning the result of the expression instead of > returning it (totally understandable, but still a difference). > * The abstract/interface variants, which don't have a function body. > > Next there are the differences in semantics which are now being introduced > for properties: > * Aside from the hooks themselves.... > * Properties _may_ or _may not_ have a default value anymore, depending on > whether the hooks cause it to be a backed or a virtual property. > * Properties with hooks can _be_ readonly, but cannot be declared as > readonly. > > And then there are the new features for properties (not just hooked ones): > * Properties can now be declared as final. > * Properties can now be declared as abstract, but only with explicit hook > requirements, otherwise the abstract keyword is not allowed. > * Properties can now be declared on interfaces, but only with explicit > hook requirements, otherwise they are not allowed. > * Abstract/Interface properties don't comply with the same > covariance/contravariance rules as other properties >
This complexity describes what I've been unconsciously worried about though I really like having the proposed feature in PHP. I'm not sure if there's a solution to reduce the complexity to a point where glancing at something will tell you how it works when you don't know exactly how all variants work. There are already a lot of gotches with something as simple as adding a `string|null` to a property without setting its default value to `null`. It's one of those things you won't find out until you run that code because technically speaking it's not incorrect. I worry that a bunch of those gotchas will bite me in the behind because I don't fully understand how it works and mistakes are easily made. It also adds a lot of complexity when reviewing someone else's code where bugs might slip through.
> 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. >
This is what I've been afraid of. Constructor method signatures are already exploding and I hope that one day we'll revise this notation and head towards `__construct($this->theProperty)` instead. One could argue that property promotion is fully optional, except that in its most basic design it does save a lot of lines of code and is a net-gain in terms of maintainability and readability in its minimum form for properly written code. The benefits fall apart the moment you don't have static code analysis available, or if the class is big/old and it just takes time for the tool to be done. If I forget to add the visibility keyword in the constructor I have to rely on my IDE telling me that the argument is unused, which results in extra overhead of trying to find out why it's telling me it's unused, just to realize I forgot to add `public` or `private`. Some of my worries would be removed if hooks are not allowed in property promotion scenarios, but this doesn't solve the complexity mentioned for the hooks themselves.

Arvids Godjuks

2 years ago
On Wed, 10 Apr 2024 at 06:43, Juliette Reinders Folmer < php-internals_nospam@adviesenzo.nl> wrote:
> On 9-4-2024 16:03, Juliette Reinders Folmer wrote: > > On 8-4-2024 23:39, Ilija Tovilo wrote: > > Hi everyone > > Heads-up: Larry and I would like to start the vote of the property > hooks RFC tomorrow:https://wiki.php.net/rfc/property-hooks > > We have worked long and hard on this RFC, and hope that we have found > some middle-ground that works for the majority. One last concern we > have not officially clarified on the list: > https://externals.io/message/122445#122667 > > I personally do not feel strongly about whether asymmetric types make it into the initial implementation. Larry does, however, and I think it is not fair to exclude them without providing any concrete reasons not to. [snip] > > My concern is more about the external impact of what is effectively a change to the type system of the language: [snip] will tools like PhpStan and Psalm require complex changes to analyse code using such properties? > > In particular, this paragraph is referencing the ability to widen the > accepted $value parameter type of the set hook, described at the > bottom of https://wiki.php.net/rfc/property-hooks#set. I have talked > to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that > this should not be complex to implement in PHPStan. In fact, PHPStan > already offers the @property-read and @property-write class > annotations, which can be used to describe "virtual" properties > handled within __get/__set, already providing asymmetric types of > sorts. Hence, this concern should be a non-issue. > > Thank you to everybody who has contributed to the discussion! > > Ilija > > > > Ilija, > > Heads-up: I'm still writing up an opinion and intend to send it to the > list before end of day (CET). I know I'm late to the party, but I've been > having trouble finding the words to express myself properly regarding this > RFC (and have been struggling to find the right words for months). > > Smile, > Juliette > > > Later than intended, but here goes.... > > If there is one RFC which has been giving me nightmares since I first > heard of it, it's this one. > > 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. > > I am going to try now anyway (before it is too late), so please bear with > me. Also, as I'm not a C-developer, please forgive me if I get the > internals wrong. I'm writing this from a PHP-dev/user perspective, with my > perspective being heavily influenced by my role as maintainer of > PHP_CodeSniffer. > > --- > TL;DR: this RFC tries to do too much in one go and introduces a huge > amount of cognitive complexity with all the exceptions and the differences > in behaviour between virtual and backed properties. This cognitive > complexity is so high that I expect that the feature will catch most > developers out a lot of the time. > --- > > I can definitely see the use case and desirability of the property hooks > functionality proposed in the RFC and compared to the initial RFC I read > last year, the current RFC is, IMO, much improved. > Huge kudos to Ilija and Larry for all the work they have put in to this! > > I applaud the intention of this RFC to make it easier to avoid the magic > __get()/__set() et al methods. What I have a problem with is the > implementation details. > > Over the last few years, we've seen a movement to get rid of more and more > of the _surprising_ behaviour of PHP, with subtle exceptions being > deprecated and slated for removal and (most) new syntaxes trying to use the > principle of least surprise by design. > > This RFC, in my view, is in stark contrast to this as it introduces a > plethora of exceptions and subtle different behaviour in a way that will > catch developers out for years to come. > > At this moment (excluding property hooks), from a user perspective, there > are three different function syntaxes in PHP: named functions (and > methods), anonymous functions and arrow functions. > > The semantics of these are very similar with subtle differences: > * Can be static or non-static. > * Take parameters, which can be typed, references, variadic, optional etc. > * Can have a return type. > * Can return by reference. > * Have a function "body". > > The differences between the current syntaxes - from a user perspective - > are as follows: > = Named functions: > * When declared in a class, can have visibility attached, can be abstract, > can be final. > * When declared in an interface or declared as abstract, will not have a > function "body". > > = Anonymous functions: > * Can import plain variables from outside its scope with a `use()` clause. > * Are declared as an expression (can be assigned to a variable etc). > > = Arrow functions: > * Have access to variables in the same scope. > * Are declared as an expression. > * Body of the function starts with a => instead of being enclosed in > curlies and can end on a range of characters. > * Can only take one statement in the body. > * Automagically returns. > > The property hooks RFC introduces a fourth flavour of function syntax. And > not just one syntax, but five and the differences in the semantics of the > function syntaxes are significant. > > The differences in semantics I see for "full" property hook functions > compared to other function syntaxes are as follows: > * `get` cannot take parameters (and doesn't have the parentheses typically > expected for a function declaration). > * `get` cannot take return type (inherits this from the property, which is > logically sound, but does create a syntactic difference). > * `set` can take one (typed or untyped) parameter. > * `set` cannot take return type (silently set to void, which is logically > sound, but does create a syntactic difference). > * `set` magically creates a $value variable for the "new" value, though > that variable _may_ have an arbitrary different name depending on whether > or not the parameter was explicitly specified. > * Has a function body. > > Then there are multiple short hand syntaxes, which each have yet more > syntactic differences: > * The arrow function variant for `get` with all the above differences + > the differences inherent to arrow functions, combined, with the exception > of the access to variables in the same scope and a more clearly defined end > of the expression. > * The implicit "set" parameter, which does not have a explicit parameter, > does not have parentheses and has the magically created $value variable. > * The arrow function variant for `set` with all the above differences + > the differences inherent to arrow functions with the above mentioned > exceptions + the implicit assignment, which breaks the expected behaviour > of arrow functions by assigning the result of the expression instead of > returning it (totally understandable, but still a difference). > * The abstract/interface variants, which don't have a function body. > > Next there are the differences in semantics which are now being introduced > for properties: > * Aside from the hooks themselves.... > * Properties _may_ or _may not_ have a default value anymore, depending on > whether the hooks cause it to be a backed or a virtual property. > * Properties with hooks can _be_ readonly, but cannot be declared as > readonly. > > And then there are the new features for properties (not just hooked ones): > * Properties can now be declared as final. > * Properties can now be declared as abstract, but only with explicit hook > requirements, otherwise the abstract keyword is not allowed. > * Properties can now be declared on interfaces, but only with explicit > hook requirements, otherwise they are not allowed. > * Abstract/Interface properties don't comply with the same > covariance/contravariance rules as other properties > > 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. > > Additionally, when reading the RFC (in its current state), I see the > following exceptions being put in place, which impact the semantics for > properties: > * Only available for object properties, static properties are excluded. > * `var` for property declarations is not supported according to the RFC. > While I 100% agree using the var keyword is old-school, using `var` > effectively makes a property a public property (which would satisfy the > visibility requirement for an interface/abstract class) and the `var` > keyword is *not* deprecated, even though the RFC states it is: > https://3v4l.org/3o50a > I'd fully support formally deprecating the `var` keyword for properties > and eventually removing it, but not supporting it for this RFC, even though > it is still a supported language feature seems opinionated and arbitrary. > Note: when testing the RFC code, declaring a property using the var > keyword on an interface does not currently throw an error (while if it is > not supported, I would expect one): > <https://3v4l.org/KaLqe/rfc#vrfc.property-hooks> > https://3v4l.org/KaLqe/rfc#vrfc.property-hooks , same goes for using the > var keyword with property hooks in a class: > https://3v4l.org/mQ6pG/rfc#vrfc.property-hooks > * Disallows references to properties, but only when there is a set hook. > * Disallows indirect modification of a property, but only when there is a > set hook. > * Write-only (virtual) properties, which cannot be read and you need to > study the hook declaration in detail to figure out if a property is or is > not write-only. > 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 (now why does that remind me of the > magic __...() methods ?). > * Readonly properties which are not denoted as readonly, but still are due > to their virtual nature (get without access to $this->prop), which can only > be figured out by, again, studying the contents of the hook function. > * Whether a type can be specified on the parameter on `set` depends on > whether the property is typed. You cannot declare `set(mixed $value)` for > an untyped property, even though it would effectively be compatible. This > is inconsistent with the behaviour for, for instance method overloads, > where this is acceptable: <https://3v4l.org/hbCor/rfc#vrfc.property-hooks> > https://3v4l.org/hbCor/rfc#vrfc.property-hooks , though it is consistent > with the behaviour of property overloads, where this is not acceptable: > https://3v4l.org/seDWM (anyone up for an RFC to fix this inconsistency ?) > * Changes the meaning of `$this->property` read/write access for all > methods called from within a hook while executing the hook. > * Creates two different flavour of properties: "backed" properties and > "virtual" properties with significantly different behaviours, raising > cognitive complexity. > This includes, but is not limited to the fact that set hooks can be > bypassed on virtual properties via a get reference. > * The range of different behaviours for various forms of serialization. > > Furthermore: > > [1] The RFC also states in "Interaction with isset() and unset()" that the > behaviour of property hooks is consistent with how `isset()` interacts with > `__get()` today. This is not true, as in: the behaviour as described for > isset with magic methods is different than what the RFC states it is: > https://3v4l.org/2Arkh/rfc#vrfc.property-hooks > > 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 > > [2] PHP supports "multi-property declarations", but I see no mention of > these in the RFC, which makes it unclear if and if so, how property hooks > would work with multi-property declarations. > class Foo { > public string $bar, $baz, $bab; > } > > [3] If a `set` hook gets a named parameter, is the magic `$value` variable > still available or not ? I'd expect not, but it is unclear from the RFC. > > [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. > > > All in all, I really do see the use case for this feature and I most > definitely appreciate all the work Ilija and Larry have put into the RFC > and the implementation. > > However, in its current state, I'm concerned that the feature introduces > so many exceptional situations and WTF moments that it is a worse solution > than the magic methods which are already available. (and no, I'm definitely > not a fan of those magic methods either). > > I think property hooks as currently proposed with all its catches will be > hard to teach, will raise the barrier of entry to the language, can catch > people out in dozens of different ways and introduces way too many > peculiarities, while the tendency of the language has been to try to > eliminate those kind of surprising behaviours. > > Irrelevant side-note: I kind of feel like I could create a completely new > "Why Equal doesn't Equal" Quiz just and only based on property hooks and > people will still not get it afterwards. > > --- > > As for the impact on static analysis tools.... > > I can't speak for other tools, but I can tell you that for PHP_CodeSniffer > the impact of this RFC will be horrendous. Aside from the impact on the > syntax recognition layer (Tokenizer), my rough estimate is that about 70% > of all sniffs ever written will need to be adjusted for these five syntaxes > and all the peculiarities around them - either to ignore hooks, work around > hooks, skip over hooks, or to take hooks into account and examine them in > all their variations. > I estimate it will take a good 5-6 years before all actively used sniffs > will have been adjusted and that in the mean time, maintainers of both > PHPCS itself as well as the popular external standards will have to deal > with a significant number of extra support requests/bug reports related to > this. > > The sheer amount of new syntaxes which have been introduced starting with > PHP 7.4, has already brought innovation in a large part of the > PHP_CodeSniffer field to a near complete halt for the past four years as > I'm continuously trying to play catch-up. This RFC will mean that > innovation, like new PSR-5/19 Docs standards, QA sniffs for tests, > significantly improving the Security standard etc, will all be put on hold > for another few years while I have to deal with this change. > > While I fully recognize that the impact on userland tooling is not a > reason to reject a new PHP feature, nor should it be, I do believe that the > impact of this RFC could be far less and easier to mitigate if different > design choices were made or if the new syntaxes were introduced in a far > slower, more staged approach across multiple PHP versions. > > Smile, > Juliette > > P.S.: nitpick: in the first two code examples under "Detailed > Implementation" - "Set" the `$u` variable is missing the dollar sign for > the `new User` line. >
So seeing how big this feedback was, I gave the RFC a proper read before I read this feedback message and frankly, as a userland developer, I checked out at the "Here is a exhaustive list of possible hook combinations and their supported operations." and just stopped reading because at that point my brain seized up and I understand nothing. What's more damming - I do not want to understand it and I'm looking at it as "I'm gonna have a .git hook that prohibits hooks period - it's so complicated and convoluted that I do not trust with it myself and not even talking about less experienced developers in my teams". The amount of complexity in these two hooks is on the level of the whole PHP OOP model with the number of footguns and WTF cases to rival magic quotes, register_globals=on and every other infamous gotcha we had in the engine that got deleted out of existence. And then there's the whole "chained methods" part that's missing at all. If I want chained methods, that means the setter hooks are useless for me, meaning we are back to just using get*() and set*() methods anyway, making the hooks kind'a irrelevant.
-- Arvīds Godjuks +371 26 851 664 arvids.godjuks@gmail.com Telegram: @psihius https://t.me/psihius

Tim Düsterhus

2 years ago
Hi On 4/10/24 10:28, Arvids Godjuks wrote:
> The amount of complexity in these two hooks is on the level of the whole > PHP OOP model with the number of footguns and WTF cases to rival magic > quotes, register_globals=on and every other infamous gotcha we had in the > engine that got deleted out of existence.
As someone who read the RFC like 20 times while it evolved (and just read it once more before submitting this email): Most of what the RFC spells out explicitly, because that's what RFCs need to, is a pretty much natural consequence of how the existing PHP features work. You cannot really shorten the RFC without either making it underspecified or useless. Is there some syntax that could technically be omitted? Sure. Would doing so meaningfully simplify the RFC? No, because most of the text length comes from the interaction of existing syntax and semantics with hooks and thus is an inherent property of property hooks (pun not intended). It's not the RFCs fault that references exist, or that casting an object to an array is legal, or whatever else needs to be explicitly spelled out. I also believe that the chosen syntax fits nicely with the existing syntax, borrowing syntax where useful and inventing new syntax where the existing syntax does not provide anything. It's a clear improvement over the status quo of __get() and __set(), especially from a third-party tooling perspective. I'm in favor. Best regards Tim Düsterhus

Arvids Godjuks

2 years ago
On Wed, 10 Apr 2024 at 22:31, Tim Düsterhus <tim@bastelstu.be> wrote:
> Hi > > On 4/10/24 10:28, Arvids Godjuks wrote: > > The amount of complexity in these two hooks is on the level of the whole > > PHP OOP model with the number of footguns and WTF cases to rival magic > > quotes, register_globals=on and every other infamous gotcha we had in the > > engine that got deleted out of existence. > > As someone who read the RFC like 20 times while it evolved (and just > read it once more before submitting this email): > > Most of what the RFC spells out explicitly, because that's what RFCs > need to, is a pretty much natural consequence of how the existing PHP > features work. You cannot really shorten the RFC without either making > it underspecified or useless. > > Is there some syntax that could technically be omitted? Sure. Would > doing so meaningfully simplify the RFC? No, because most of the text > length comes from the interaction of existing syntax and semantics with > hooks and thus is an inherent property of property hooks (pun not > intended). It's not the RFCs fault that references exist, or that > casting an object to an array is legal, or whatever else needs to be > explicitly spelled out. > > I also believe that the chosen syntax fits nicely with the existing > syntax, borrowing syntax where useful and inventing new syntax where the > existing syntax does not provide anything. > > It's a clear improvement over the status quo of __get() and __set(), > especially from a third-party tooling perspective. I'm in favor. > > Best regards > Tim Düsterhus >
Yeah we had a discussion with Larry on SF Slack, he made some things clear that it's either all or nothing, because once they dove in, it became clear that there's no way to gradually to introduce it. So, if it passes, the community is just gonna have to deal with the massive scope of it if they want the hooks or forget about them.
-- Arvīds Godjuks +371 26 851 664 arvids.godjuks@gmail.com Telegram: @psihius https://t.me/psihius

Robert Landers

2 years ago
On Wed, Apr 10, 2024 at 5:49 AM Juliette Reinders Folmer <php-internals_nospam@adviesenzo.nl> wrote:
> > On 9-4-2024 16:03, Juliette Reinders Folmer wrote: > > On 8-4-2024 23:39, Ilija Tovilo wrote: > > Hi everyone > > Heads-up: Larry and I would like to start the vote of the property > hooks RFC tomorrow: > https://wiki.php.net/rfc/property-hooks > > We have worked long and hard on this RFC, and hope that we have found > some middle-ground that works for the majority. One last concern we > have not officially clarified on the list: > > https://externals.io/message/122445#122667 > > I personally do not feel strongly about whether asymmetric types make it into the initial implementation. Larry does, however, and I think it is not fair to exclude them without providing any concrete reasons not to. [snip] > > My concern is more about the external impact of what is effectively a change to the type system of the language: [snip] will tools like PhpStan and Psalm require complex changes to analyse code using such properties? > > In particular, this paragraph is referencing the ability to widen the > accepted $value parameter type of the set hook, described at the > bottom of https://wiki.php.net/rfc/property-hooks#set. I have talked > to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that > this should not be complex to implement in PHPStan. In fact, PHPStan > already offers the @property-read and @property-write class > annotations, which can be used to describe "virtual" properties > handled within __get/__set, already providing asymmetric types of > sorts. Hence, this concern should be a non-issue. > > Thank you to everybody who has contributed to the discussion! > > Ilija > > > Ilija, > > Heads-up: I'm still writing up an opinion and intend to send it to the list before end of day (CET). I know I'm late to the party, but I've been having trouble finding the words to express myself properly regarding this RFC (and have been struggling to find the right words for months). > > Smile, > Juliette > > > Later than intended, but here goes.... > > If there is one RFC which has been giving me nightmares since I first heard of it, it's this one. > > 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. > > I am going to try now anyway (before it is too late), so please bear with me. Also, as I'm not a C-developer, please forgive me if I get the internals wrong. I'm writing this from a PHP-dev/user perspective, with my perspective being heavily influenced by my role as maintainer of PHP_CodeSniffer. > > --- > TL;DR: this RFC tries to do too much in one go and introduces a huge amount of cognitive complexity with all the exceptions and the differences in behaviour between virtual and backed properties. This cognitive complexity is so high that I expect that the feature will catch most developers out a lot of the time. > --- > > I can definitely see the use case and desirability of the property hooks functionality proposed in the RFC and compared to the initial RFC I read last year, the current RFC is, IMO, much improved. > Huge kudos to Ilija and Larry for all the work they have put in to this! > > I applaud the intention of this RFC to make it easier to avoid the magic __get()/__set() et al methods. What I have a problem with is the implementation details. > > Over the last few years, we've seen a movement to get rid of more and more of the _surprising_ behaviour of PHP, with subtle exceptions being deprecated and slated for removal and (most) new syntaxes trying to use the principle of least surprise by design. > > This RFC, in my view, is in stark contrast to this as it introduces a plethora of exceptions and subtle different behaviour in a way that will catch developers out for years to come. > > At this moment (excluding property hooks), from a user perspective, there are three different function syntaxes in PHP: named functions (and methods), anonymous functions and arrow functions. > > The semantics of these are very similar with subtle differences: > * Can be static or non-static. > * Take parameters, which can be typed, references, variadic, optional etc. > * Can have a return type. > * Can return by reference. > * Have a function "body". > > The differences between the current syntaxes - from a user perspective - are as follows: > = Named functions: > * When declared in a class, can have visibility attached, can be abstract, can be final. > * When declared in an interface or declared as abstract, will not have a function "body". > > = Anonymous functions: > * Can import plain variables from outside its scope with a `use()` clause. > * Are declared as an expression (can be assigned to a variable etc). > > = Arrow functions: > * Have access to variables in the same scope. > * Are declared as an expression. > * Body of the function starts with a => instead of being enclosed in curlies and can end on a range of characters. > * Can only take one statement in the body. > * Automagically returns. > > The property hooks RFC introduces a fourth flavour of function syntax. And not just one syntax, but five and the differences in the semantics of the function syntaxes are significant. > > The differences in semantics I see for "full" property hook functions compared to other function syntaxes are as follows: > * `get` cannot take parameters (and doesn't have the parentheses typically expected for a function declaration). > * `get` cannot take return type (inherits this from the property, which is logically sound, but does create a syntactic difference). > * `set` can take one (typed or untyped) parameter. > * `set` cannot take return type (silently set to void, which is logically sound, but does create a syntactic difference). > * `set` magically creates a $value variable for the "new" value, though that variable _may_ have an arbitrary different name depending on whether or not the parameter was explicitly specified. > * Has a function body. > > Then there are multiple short hand syntaxes, which each have yet more syntactic differences: > * The arrow function variant for `get` with all the above differences + the differences inherent to arrow functions, combined, with the exception of the access to variables in the same scope and a more clearly defined end of the expression. > * The implicit "set" parameter, which does not have a explicit parameter, does not have parentheses and has the magically created $value variable. > * The arrow function variant for `set` with all the above differences + the differences inherent to arrow functions with the above mentioned exceptions + the implicit assignment, which breaks the expected behaviour of arrow functions by assigning the result of the expression instead of returning it (totally understandable, but still a difference). > * The abstract/interface variants, which don't have a function body. > > Next there are the differences in semantics which are now being introduced for properties: > * Aside from the hooks themselves.... > * Properties _may_ or _may not_ have a default value anymore, depending on whether the hooks cause it to be a backed or a virtual property. > * Properties with hooks can _be_ readonly, but cannot be declared as readonly. > > And then there are the new features for properties (not just hooked ones): > * Properties can now be declared as final. > * Properties can now be declared as abstract, but only with explicit hook requirements, otherwise the abstract keyword is not allowed. > * Properties can now be declared on interfaces, but only with explicit hook requirements, otherwise they are not allowed. > * Abstract/Interface properties don't comply with the same covariance/contravariance rules as other properties > > 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. > > Additionally, when reading the RFC (in its current state), I see the following exceptions being put in place, which impact the semantics for properties: > * Only available for object properties, static properties are excluded. > * `var` for property declarations is not supported according to the RFC. While I 100% agree using the var keyword is old-school, using `var` effectively makes a property a public property (which would satisfy the visibility requirement for an interface/abstract class) and the `var` keyword is *not* deprecated, even though the RFC states it is: https://3v4l.org/3o50a > I'd fully support formally deprecating the `var` keyword for properties and eventually removing it, but not supporting it for this RFC, even though it is still a supported language feature seems opinionated and arbitrary. > Note: when testing the RFC code, declaring a property using the var keyword on an interface does not currently throw an error (while if it is not supported, I would expect one): https://3v4l.org/KaLqe/rfc#vrfc.property-hooks , same goes for using the var keyword with property hooks in a class: https://3v4l.org/mQ6pG/rfc#vrfc.property-hooks > * Disallows references to properties, but only when there is a set hook. > * Disallows indirect modification of a property, but only when there is a set hook. > * Write-only (virtual) properties, which cannot be read and you need to study the hook declaration in detail to figure out if a property is or is not write-only. > 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 (now why does that remind me of the magic __...() methods ?). > * Readonly properties which are not denoted as readonly, but still are due to their virtual nature (get without access to $this->prop), which can only be figured out by, again, studying the contents of the hook function. > * Whether a type can be specified on the parameter on `set` depends on whether the property is typed. You cannot declare `set(mixed $value)` for an untyped property, even though it would effectively be compatible. This is inconsistent with the behaviour for, for instance method overloads, where this is acceptable: https://3v4l.org/hbCor/rfc#vrfc.property-hooks , though it is consistent with the behaviour of property overloads, where this is not acceptable: https://3v4l.org/seDWM (anyone up for an RFC to fix this inconsistency ?) > * Changes the meaning of `$this->property` read/write access for all methods called from within a hook while executing the hook. > * Creates two different flavour of properties: "backed" properties and "virtual" properties with significantly different behaviours, raising cognitive complexity. > This includes, but is not limited to the fact that set hooks can be bypassed on virtual properties via a get reference. > * The range of different behaviours for various forms of serialization. > > Furthermore: > > [1] The RFC also states in "Interaction with isset() and unset()" that the behaviour of property hooks is consistent with how `isset()` interacts with `__get()` today. This is not true, as in: the behaviour as described for isset with magic methods is different than what the RFC states it is: https://3v4l.org/2Arkh/rfc#vrfc.property-hooks > > 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 > > [2] PHP supports "multi-property declarations", but I see no mention of these in the RFC, which makes it unclear if and if so, how property hooks would work with multi-property declarations. > class Foo { > public string $bar, $baz, $bab; > } > > [3] If a `set` hook gets a named parameter, is the magic `$value` variable still available or not ? I'd expect not, but it is unclear from the RFC. > > [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. > > > All in all, I really do see the use case for this feature and I most definitely appreciate all the work Ilija and Larry have put into the RFC and the implementation. > > However, in its current state, I'm concerned that the feature introduces so many exceptional situations and WTF moments that it is a worse solution than the magic methods which are already available. (and no, I'm definitely not a fan of those magic methods either). > > I think property hooks as currently proposed with all its catches will be hard to teach, will raise the barrier of entry to the language, can catch people out in dozens of different ways and introduces way too many peculiarities, while the tendency of the language has been to try to eliminate those kind of surprising behaviours. > > Irrelevant side-note: I kind of feel like I could create a completely new "Why Equal doesn't Equal" Quiz just and only based on property hooks and people will still not get it afterwards. > > --- > > As for the impact on static analysis tools.... > > I can't speak for other tools, but I can tell you that for PHP_CodeSniffer the impact of this RFC will be horrendous. Aside from the impact on the syntax recognition layer (Tokenizer), my rough estimate is that about 70% of all sniffs ever written will need to be adjusted for these five syntaxes and all the peculiarities around them - either to ignore hooks, work around hooks, skip over hooks, or to take hooks into account and examine them in all their variations. > I estimate it will take a good 5-6 years before all actively used sniffs will have been adjusted and that in the mean time, maintainers of both PHPCS itself as well as the popular external standards will have to deal with a significant number of extra support requests/bug reports related to this. > > The sheer amount of new syntaxes which have been introduced starting with PHP 7.4, has already brought innovation in a large part of the PHP_CodeSniffer field to a near complete halt for the past four years as I'm continuously trying to play catch-up. This RFC will mean that innovation, like new PSR-5/19 Docs standards, QA sniffs for tests, significantly improving the Security standard etc, will all be put on hold for another few years while I have to deal with this change. > > While I fully recognize that the impact on userland tooling is not a reason to reject a new PHP feature, nor should it be, I do believe that the impact of this RFC could be far less and easier to mitigate if different design choices were made or if the new syntaxes were introduced in a far slower, more staged approach across multiple PHP versions. > > Smile, > Juliette > > P.S.: nitpick: in the first two code examples under "Detailed Implementation" - "Set" the `$u` variable is missing the dollar sign for the `new User` line.
As someone who has spent a number of years writing C#, the RFC looks quite sane. I do agree that it does quite a lot in one go though. If you look at how fields were introduced into C#, the "short syntax" version didn't come along until years after the long version and people had a good grasp of what they wanted: a shorter way to write fields because writing it out all the time was annoying af. Getting it from the start will be really nice! Personally, I'm not a big fan of calling arbitrary methods from inside setters/getters. I'm pretty sure this is allowed in C#, however, it is an anti-pattern and avoided except for very narrow edge cases. I'm sure we'll see some of that here, where people will get bitten, and a consensus of anti-patterns will emerge. However, having that power available is immensely useful /when you need it/! Removing it because "it complicates things" isn't really a good reason, IMHO. To me, as a non-voting outsider, it smells like the RFC process breaks down past a certain point. I've watched operator overloading fail ... and I am implementing an RFC that could have been done 1000% in user-space if operator overloading had passed. But it didn't, so now we need to literally change the language to support it. Then watching the feedback here, at the 11th hour, is super saddening. Will we never be able to make great changes to the language due to a tragedy of the commons? Robert Landers Software Engineer Utrecht NL

Matteo Beccati

2 years ago
Hi, Il 10/04/2024 05:40, Juliette Reinders Folmer ha scritto:
> --- > TL;DR: this RFC tries to do too much in one go and introduces a huge > amount of cognitive complexity with all the exceptions and the > differences in behaviour between virtual and backed properties. This > cognitive complexity is so high that I expect that the feature will > catch most developers out a lot of the time. > ---
Thanks Juliette for the excellent write-up (I quoted the TL;DR just for brevity). At first what the RFC promises looks incredibly shiny and got me very excited. The work done by Larry and Ilija is certainly brilliant, but the complexity is so big that I feel we should aim for a trimmed down version to make it easier to grasp and to work with, e.g. less syntax variants (no short or constructor promotion), and perhaps no virtual properties. Cheers
-- Matteo Beccati Development & Consulting - http://www.beccati.com/

Erick de Azevedo Lima

2 years ago
> As someone who has spent a number of years writing C#, the RFC look>
quite sane. Same here.
> Removing it because "it complicates things" isn't really a good reason,
IMHO. I think the same. FFI, for instance, is powerful but one can make a mess with it.
> Then watching the feedback here, at the 11th hour, is super saddening.
Again, same here . If this feedback was given during the "heat of the discussion", maybe it could lead the discussion to a place where less variants would be introduced on this first implementation and a shorter syntax could be introduced later on, like the arrow functions were introduced after anonymous functions. Maybe if such a feedback was given before and it was decided to go for a trimmed version of the feature, maybe Ilija/Larry could have had less work to implement and test all the variantes they've done. I think if a person has such numerous concerns, it should be exposed ASAP even if your thoughts are not totally clear. If you had a chat directly with Ilija/Larry, they would be aware of such concerns and would expend efforts to address these concerns. Regards, Erick

Alex Wells

2 years ago
On Wed, Apr 10, 2024 at 4:01 PM Erick de Azevedo Lima < ericklima.comp@gmail.com> wrote:
> Maybe if such a feedback was given before and it was decided to go for a > trimmed version of the feature, maybe Ilija/Larry could have had less work > to implement and test all the variantes they've done. I think if a person > has such numerous concerns, it should be exposed ASAP even if your thoughts > are not totally clear. If you had a chat directly with Ilija/Larry, they > would be aware of such concerns and would expend efforts to address these > concerns. >
To an outsider, it looks wild when feedback starts coming in right before the vote starts. What's even more startling is that there are people with voting rights who have never participated in the discussion at all, yet have a right to wordlessly affect the vote's outcome. I sincerely hope Ilija and Larry's work don't go to waste here.

Bilge

2 years ago
On 10/04/2024 15:56, Alex Wells wrote:
> To an outsider, it looks wild when feedback starts coming in right > before the vote starts. What's even more startling is that there are > people with voting rights who have never participated in the > discussion at all, yet have a right to wordlessly affect the vote's > outcome. I sincerely hope Ilija and Larry's work don't go to waste here.
There are always people afraid of change. I just hope they don't stand in the way of progress.

Unnamed Person

2 years ago
On 4/10/24 07:56, Alex Wells wrote:
> To an outsider, it looks wild when feedback starts coming in right > before the vote starts. What's even more startling is that there are > people with voting rights who have never participated in the > discussion at all, yet have a right to wordlessly affect the vote's > outcome. I sincerely hope Ilija and Larry's work don't go to waste here.
-- Clearly those of us who agreed with the approach and wanted the RFC and planned to vote to approve things should've been more vocal about that. Don't take silence to be apathy - email is often the worst medium for a deep discussion and "ditto" doesn't add to the discourse. I for one will try to be more vocal since apparently what I thought was noise is apparently necessary signal here. Is the feature complex? Yes. Is it overly complex? No. As a developer who has hand-rolled custom getter/setter behavior for years this will save a mountain of time (and would've removed nearly half of the code I once had to write for a very complicated WordPress feature). I love the approach and wholeheartedly support the direction here. The similarity to C# and Kotlin structures should lower the cognitive burden here as well.

Saki Takamachi

2 years ago
Hi,
> To an outsider, it looks wild when feedback starts coming in right before the vote starts. What's even more startling is that there are people with voting rights who have never participated in the discussion at all, yet have a right to wordlessly affect the vote's outcome. I sincerely hope Ilija and Larry's work don't go to waste here.
I was also one of those who did not speak out because I was in favor of this RFC. The syntax is new and may look unfamiliar, but people should soon get used to it. Also, I personally don't think the functionality of property hooks is complicated. Since this is a new feature, the amount of information may be a little large during initial learning, but the feature itself is not that complicated. Regards, Saki

Marco Pivetta

2 years ago
On Wed, 10 Apr 2024 at 16:56, Alex Wells <autaut03@gmail.com> wrote:
> On Wed, Apr 10, 2024 at 4:01 PM Erick de Azevedo Lima < > ericklima.comp@gmail.com> wrote: > >> Maybe if such a feedback was given before and it was decided to go for a >> trimmed version of the feature, maybe Ilija/Larry could have had less work >> to implement and test all the variantes they've done. I think if a person >> has such numerous concerns, it should be exposed ASAP even if your thoughts >> are not totally clear. If you had a chat directly with Ilija/Larry, they >> would be aware of such concerns and would expend efforts to address these >> concerns. >> > > To an outsider, it looks wild when feedback starts coming in right before > the vote starts. What's even more startling is that there are people with > voting rights who have never participated in the discussion at all, yet > have a right to wordlessly affect the vote's outcome. I sincerely hope > Ilija and Larry's work don't go to waste here. >
Reminder to not run into a sunk cost fallacy. I totally understand the sentiment around the work done, but the outcome is something very impactful. Work may or may not be wasted, but that's the nature of RFCs. The current RFC makes a lot of people uneasy, and JRF's comment resonates strongly with my opinion. To me, further overloading the arrow (`->`) operator with this much added context is a mistake, regardless of how much detail was added in handling all the weird edge cases of the language: in fact, going as far as exploring by-ref semantics (instead of just saying "no" to them) is also a big problem. Marco Pivetta https://mastodon.social/@ocramius https://ocramius.github.io/

Erick de Azevedo Lima

2 years ago
> Reminder to not run into a sunk cost fallacy.
My main point was not the cost/work itself, but the fact that big concerns should be raised ASAP, and not on the 11th hour, as Robert said. Whether we like it or not, the voting right beholders' concerns should be taken into account with "special attention", because, well... They are the ones that, in the end, decide if your RFC will make it to the language or not. So having no negative feedback or having it only at the last minute before voting is not good at all. I think doing this and voting for a "no" for a feature that was so well received and discussed extensively by the internalians discourages OSS developers from contributing to the language we care so much about. Regards, Erick

Marco Pivetta

2 years ago
On Wed, 10 Apr 2024 at 18:52, Erick de Azevedo Lima < ericklima.comp@gmail.com> wrote:
> > Reminder to not run into a sunk cost fallacy. > > My main point was not the cost/work itself, but the fact that big concerns > should be raised ASAP, and not on the 11th hour, as Robert said. > Whether we like it or not, the voting right beholders' concerns should be > taken into account with "special attention", because, well... > They are the ones that, in the end, decide if your RFC will make it to the > language or not. > So having no negative feedback or having it only at the last minute before > voting is not good at all. > I think doing this and voting for a "no" for a feature that was so well > received and discussed extensively by the internalians discourages > OSS developers from contributing to the language we care so much about. >
Agreed, but to give you some more context: * I know JRF has been holding off feedback about it for a long time, waiting for the RFC to be more "stable" (i.e. not being a moving flubber) before posting thoughts * I personally usually only get into RFCs very early on (when I have a clear idea/opinion formed on a topic upfront) or at the end, when the RFC has gone through the ironing out a lot: the in-between is a brawl for which 24h/day aren't sufficient. The nature of email threads in a mailing list makes RFCs extremely hard to approach anything that is "in-flight". Marco Pivetta https://mastodon.social/@ocramius https://ocramius.github.io/

Jeffrey Dafoe

2 years ago
> On Wed, Apr 10, 2024 at 4:01 PM Erick de Azevedo Lima <mailto:ericklima.comp@gmail.com> wrote: > Maybe if such a feedback was given before and it was decided to go for a trimmed version of the feature, maybe Ilija/Larry could have had less work to implement and test all the variantes they've done. I think if a person has such numerous concerns, it should be exposed ASAP even if your thoughts are not > totally clear. If you had a chat directly with Ilija/Larry, they would be aware of such concerns and would expend efforts to address these concerns.
> To an outsider, it looks wild when feedback starts coming in right before the vote starts. What's even more startling is that there are people with voting rights who have never participated in the discussion at all, yet have a right to wordlessly affect the vote's outcome. I sincerely hope Ilija and Larry's work don't > go to waste here.
I hope that the property hooks feature goes through. I've used it in C# and it reads nice and saves time. Waiting until the last minute to express a significant design disagreement is disrespectful of the time the RFC authors have spent working on this proposal. It's also counterproductive to the PHP project overall. -Jeff

Mark Trapp

2 years ago
On Wed, Apr 10, 2024 at 9:59 AM Jeffrey Dafoe <JDafoe@fsx.com> wrote:
> > > On Wed, Apr 10, 2024 at 4:01 PM Erick de Azevedo Lima <mailto:ericklima.comp@gmail.com> wrote: > > Maybe if such a feedback was given before and it was decided to go for a trimmed version of the feature, maybe Ilija/Larry could have had less work to implement and test all the variantes they've done. I think if a person has such numerous concerns, it should be exposed ASAP even if your thoughts are not > > totally clear. If you had a chat directly with Ilija/Larry, they would be aware of such concerns and would expend efforts to address these concerns. > > > To an outsider, it looks wild when feedback starts coming in right before the vote starts. What's even more startling is that there are people with voting rights who have never participated in the discussion at all, yet have a right to wordlessly affect the vote's outcome. I sincerely hope Ilija and Larry's work don't > > go to waste here. > > I hope that the property hooks feature goes through. I've used it in C# and it reads nice and saves time. > > Waiting until the last minute to express a significant design disagreement is disrespectful of the time the RFC authors have spent working on this proposal. It's also counterproductive to the PHP project overall. > > -Jeff
Just a friendly reminder that feedback on why people vote against RFCs has been desperately sought after and requested each time a seemingly popular RFC is rejected, and the feedback provided by others for this RFC was done so in good faith, before voting started, allowing the RFC authors to address said feedback should they choose to acknowledge it. And in general, the wider PHP community does not pay attention to RFCs until/unless they come up for a vote. Admonishing people for providing unexpected or negative feedback will have a chilling effect for future RFC attempts, as it's much easier to just silently vote no than to be attacked simply for providing a dissenting opinion at an inconvenient time. - Mark

Robert Landers

2 years ago
On Wed, Apr 10, 2024 at 10:41 PM Mark Trapp <mark@itafroma.com> wrote:
> > On Wed, Apr 10, 2024 at 9:59 AM Jeffrey Dafoe <JDafoe@fsx.com> wrote: > > > > > On Wed, Apr 10, 2024 at 4:01 PM Erick de Azevedo Lima <mailto:ericklima.comp@gmail.com> wrote: > > > Maybe if such a feedback was given before and it was decided to go for a trimmed version of the feature, maybe Ilija/Larry could have had less work to implement and test all the variantes they've done. I think if a person has such numerous concerns, it should be exposed ASAP even if your thoughts are not > > > totally clear. If you had a chat directly with Ilija/Larry, they would be aware of such concerns and would expend efforts to address these concerns. > > > > > To an outsider, it looks wild when feedback starts coming in right before the vote starts. What's even more startling is that there are people with voting rights who have never participated in the discussion at all, yet have a right to wordlessly affect the vote's outcome. I sincerely hope Ilija and Larry's work don't > > > go to waste here. > > > > I hope that the property hooks feature goes through. I've used it in C# and it reads nice and saves time. > > > > Waiting until the last minute to express a significant design disagreement is disrespectful of the time the RFC authors have spent working on this proposal. It's also counterproductive to the PHP project overall. > > > > -Jeff > > Just a friendly reminder that feedback on why people vote against RFCs > has been desperately sought after and requested each time a seemingly > popular RFC is rejected, and the feedback provided by others for this > RFC was done so in good faith, before voting started, allowing the RFC > authors to address said feedback should they choose to acknowledge it. > > And in general, the wider PHP community does not pay attention to RFCs > until/unless they come up for a vote. > > Admonishing people for providing unexpected or negative feedback will > have a chilling effect for future RFC attempts, as it's much easier to > just silently vote no than to be attacked simply for providing a > dissenting opinion at an inconvenient time. > > - Mark
Hey Mark, I think that is fair, but I'd also point out that RFCs have been rejected in the past simply because voters felt it was "too last minute" before a release. I don't think it's fair to decline RFCs for being too last minute, but when the inverse happens to an RFC: a huge dissenting opinion at the last second, call foul. Largely, I agree with you but it's worth pointing out the incongruity here. Robert Landers Software Engineer Utrecht NL

Ilija Tovilo

2 years ago
Hi Juliette On 9-4-2024 16:03, Juliette Reinders Folmer wrote:
> On 8-4-2024 23:39, Ilija Tovilo wrote: >> >> https://wiki.php.net/rfc/property-hooks >> > > 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. I will try to address each, and give people some time to respond.
> And not just one syntax, but five and the differences in the semantics of the function syntaxes are significant.
I think this is somewhat of a misrepresentation. For reference, you can find the grammar here. It's only about <50 lines. https://github.com/php/php-src/blob/bf390b47c7522fd4d130be26b8ee97520f985275/Zend/zend_language_parser.y#L1088-L1131 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. What would your optimal solution look like? I feel this discussion cannot progress unless we have more concrete discussion points.
> * The implicit "set" parameter, which does not have a explicit parameter, does not have parentheses and has the magically created $value variable.
To be a bit more exact, the parameter list is simply inferred to `(<property type> $value)` so that the user does not need to spell it out. This is not akin to other magic variables we had previously, like `$http_response_header`.
> TL;DR: this RFC tries to do too much in one go and introduces a huge amount of cognitive complexity with all the exceptions and the differences in behaviour between virtual and backed properties. This cognitive complexity is so high that I expect that the feature will catch most developers out a lot of the time.
Many people still say that this RFC doesn't do enough because it doesn't support x/y/z. This is including you:
> * The arrow function variant for `set` with all the above differences + the differences inherent to arrow functions with the above mentioned exceptions + the implicit assignment, which breaks the expected behaviour of arrow functions by assigning the result of the expression instead of returning it (totally understandable, but still a difference). > * Properties _may_ or _may not_ have a default value anymore, depending on whether the hooks cause it to be a backed or a virtual property. > * Properties with hooks can _be_ readonly, but cannot be declared as readonly. > * Only available for object properties, static properties are excluded. > * Readonly properties which are not denoted as readonly, but still are due to their virtual nature (get without access to $this->prop), which can only be figured out by, again, studying the contents of the hook function.
There are more below that I will be commenting on in more detail. I understand that, in a perfect world, each language feature would automatically work with every other. In reality, this is either not possible, or complex and time consuming. We opted for the middle ground, allowing things that were workable and omitting things that weren't, or required unrelated changes. If this is not acceptable, then I genuinely don't know how to approach non-trivial RFCs.
> * Properties can now be declared as abstract, but only with explicit hook requirements, otherwise the abstract keyword is not allowed. > * Properties can now be declared on interfaces, but only with explicit hook requirements, otherwise they are not allowed.
The semantics of plain interface properties are not clear. Intuitively, `public $prop;` looks equivalent to `public $prop { get; set; }`, but due to reference semantics it is not. It's not even equivalent to `{ &get; set; }`, because by-reference hooked properties still have some inherent limitations (https://wiki.php.net/rfc/property-hooks#assignment_by_reference). Again, this could be addressed, but only at the cost of complexity.
> * Abstract/Interface properties don't comply with the same covariance/contravariance rules as other properties
Do you mean that they are not invariant as other properties? I think it's generally accepted that LSP rules should be as lax as possible, while being sound. Note that this rule isn't unique to read-/write-only abstract properties either, but also applies to read-/write-only virtual properties. ```php class P { public string|int $readonly { get => 'foo'; } } class C extends P { public int $readonly { get => 42; } } ```
> 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?
> * `var` for property declarations is not supported according to the RFC.
This is a misunderstanding between Larry and me. I personally never intended to deprecate `var` in this RFC. We've briefly discussed this and decided to treat `var` as normal, meaning as an alias of `public`.
> * Disallows references to properties, but only when there is a set hook. > * Disallows indirect modification of a property, but only when there is a set hook.
The RFC spends much time explaining these limitations. If the RFC is ever accepted, in any form, then we must accept either of these approaches: 1. The engine places limitations on by-reference usage of properties when there is a `set` hook. 2. The user can trivially circumvent the `set` hook. This limitation is not unique to PHP either.
> * Write-only (virtual) properties, which cannot be read and you need to study the hook declaration in detail to figure out if a property is or is not write-only.
Indeed. But this has also been discussed in much detail. One proposed solution was explicitly marking properties as virtual. This comes with the downside of changing the property signature, making conversion of a backed parent property to virtual (and vice versa) a BC break. If you have a different solution in mind, please share.
> 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.
> now why does that remind me of the magic __...() methods ?
That's no secret. We have mentioned many times that we use the same mechanism for accessing backing value as `__get`/`__set` uses for accessing dynamic properties.
> * Whether a type can be specified on the parameter on `set` depends on whether the property is typed. You cannot declare `set(mixed $value)` for an untyped property, even though it would effectively be compatible. This is inconsistent with the behaviour for, for instance method overloads, where this is acceptable: https://3v4l.org/hbCor/rfc#vrfc.property-hooks , though it is consistent with the behaviour of property overloads, where this is not acceptable: https://3v4l.org/seDWM (anyone up for an RFC to fix this inconsistency ?)
For child properties, it is not legal to add `mixed` to a previously untyped property. https://3v4l.org/mScQS#v8.3.5 I was honestly not aware of the case you outlined. Anyway, either approach we chose will be inconsistent with the other.
> * Changes the meaning of `$this->property` read/write access for all methods called from within a hook while executing the hook.
What is the right solution to this in your opinion? Recursion surely isn't it. Throwing when accessing the backing value outside of its corresponding hook? I will investigate whether this is possible in an efficient way, and whether it's worth considering.
> * Creates two different flavour of properties: "backed" properties and "virtual" properties with significantly different behaviours, raising cognitive complexity. > This includes, but is not limited to the fact that set hooks can be bypassed on virtual properties via a get reference.
Sure, but is there actually a solution to this? Making all properties backed means that serialization includes backing values that aren't used, and that the property supports useless reads/writes for properties that are only read or only written to. Only supporting virtual properties means that there's an inevitable BC break when converting a backed property to a virtual one, because the explicit backing property cannot share the same name as the virtual one, which will once again affect serialization.
> * The range of different behaviours for various forms of serialization.
Indeed, we carefully picked the behavior that should make sense for each case. Isn't that better than the user having to remember that they need to handle serialization/JSON themselves, depending on the case, which they also need to identify themselves?
> [1] The RFC also states in "Interaction with isset() and unset()" that the behaviour of property hooks is consistent with how `isset()` interacts with `__get()` today. This is not true, as in: the behaviour as described for isset with magic methods is different than what the RFC states it is: https://3v4l.org/2Arkh/rfc#vrfc.property-hooks
Right, this paragraph is a bit misleading. In essence, `isset` triggers both `__isset` and `__get` to 1. verify that the property even exists and 2. if it does, that it is non-null. For hooked properties, we don't need to verify the former, because the property is explicitly declared. So we only call `get` and make sure its value is non-null. We'll make sure to clarify this better.
> 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.
> [2] PHP supports "multi-property declarations", but I see no mention of these in the RFC, which makes it unclear if and if so, how property hooks would work with multi-property declarations. > class Foo { > public string $bar, $baz, $bab; > }
The implementation does not allow mixing multi-property declarations with hooks. I find them confusing (e.g. are `public int $foo, $bar;` two typed properties, or one typed and one untyped?). The same applies for hooks. We'll make sure to spell this out explicitly.
> [3] If a `set` hook gets a named parameter, is the magic `$value` variable still available or not ? I'd expect not, but it is unclear from the RFC.
No. As mentioned above, on omission of the parameter list, it is simply inferred. There's no "truly" magic `$value` variable.
> [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.
> As for the impact on static analysis tools....
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. Here are the things we're changing: * `var` continues to be supported. * We will change accessing a virtual property with the same name from within a method called from a hook to throw, rather than create a dynamic property. * We will change accessing a backed property from within a method called from a hook to throw as well, assuming it can be done performantly. (We need to verify this.) 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. Ilija

Unnamed Person

2 years ago
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

Marc

2 years ago
Hi, On 12.04.24 04:55, Juliette Reinders Folmer wrote:
> Hi Ilija, > > On 12-4-2024 1:00, Ilija Tovilo wrote: > <snip> > >>> 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. ;-) >
This was something I was confused by as well but wasn't sure at all if this would actually be an issue in a real world. So I tried to come up with a real world example. If I understand correctly you already changed the behavior to throw now but I still want to share anyway .... Imaging, as library author, you have a property with hooks that contain a typo and you want to rename it in a BC way. Let's say you want to rename "oldProp" to "newProp" but both should work the same as if it would be the same property. class Test {     const PREFIX = 'backed:';     public string $newProp {         set => $this->prefixize($value);         get => $this->getProp();     }     public string $oldProp {         set {             $this->newProp = $value;         }         get => $this->getProp();     }     /** Complex value transformer */     private function prefixize($value) {         return self::PREFIX . $value;     }     /** Complex value reverse transformer */     private function unprefixize($prefixed) {         return substr($prefixed, strlen(self::PREFIX));     }     private function getProp() {         return $this->unprefixize($this->newProp);     } } $t = new Test(); $t->newProp = 'foo'; var_dump($t); // object(Test)#1 (1) { ["newProp"]=> string(10) "backed:foo" } var_dump($t->newProp); // string(3) "foo" var_dump($t->oldProp); // string(0) "" $t->oldProp = 'bar'; var_dump($t); // { ["newProp"]=> string(10) "backed:bar" } var_dump($t->newProp); // string(3) "bar" var_dump($t->oldProp); // string(0) "" Kind regards, Marc

Larry Garfield

2 years ago
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

Rowan Tommins [IMSoP]

2 years ago
On 10 April 2024 04:40:13 BST, Juliette Reinders Folmer <php-internals_nospam@adviesenzo.nl> wrote: * Whether a type can be specified on the parameter on `set` depends on whether the property is typed. You cannot declare `set(mixed $value)` for an untyped property, even though it would effectively be compatible. This is inconsistent with the behaviour for, for instance method overloads, where this is acceptable: https://3v4l.org/hbCor/rfc#vrfc.property-hooks , though it is consistent with the behaviour of property overloads, where this is not acceptable: https://3v4l.org/seDWM (anyone up for an RFC to fix this inconsistency ?) Just picking up on this point, because it's a bit of a tangle: PHP currently makes a hard distinction between "typed properties" and "untyped properties". For instance, unset() works differently, and the "readonly" attribute can only be added to a typed property. That's actually rather relevant to your point, because if this RFC passes we would probably need to consider that PHP has at least 4 types of properties: - dynamic properties (deprecated by default, but allowed with an attribute) - declared but untyped properties - typed properties - virtual properties But maybe 6, with: - untyped properties with hooks - typed properties with hooks Of course, most of the time, users aren't aware of the current 3-way split, and they won't need to think about all 6 of these variations. But there are going to be cases where documentation or a future RFC has to cover edge cases of each. I do think there is scope for removing some features from the RFC which are nice but not essential, and reducing these combinations. For instance, if we limit the access to the underlying property, we might be able to treat "virtual properties" as just an optimisation: the engine doesn't allocate a property it knows will never be accessed, and accesses to it, e.g. via reflection, just return "uninitialized". I am however conscious that RFCs have failed in the past for being "not complete enough" as well as for being "too complex". Regards, Rowan Tommins [IMSoP]

Levi Morrison

2 years ago
On Mon, Apr 8, 2024 at 3:42 PM Ilija Tovilo <tovilo.ilija@gmail.com> wrote:
> > Hi everyone > > Heads-up: Larry and I would like to start the vote of the property > hooks RFC tomorrow: > https://wiki.php.net/rfc/property-hooks > > We have worked long and hard on this RFC, and hope that we have found > some middle-ground that works for the majority. One last concern we > have not officially clarified on the list: > > https://externals.io/message/122445#122667 > > >> I personally do not feel strongly about whether asymmetric types make it into the initial implementation. Larry does, however, and I think it is not fair to exclude them without providing any concrete reasons not to. [snip] > > > > My concern is more about the external impact of what is effectively a change to the type system of the language: [snip] will tools like PhpStan and Psalm require complex changes to analyse code using such properties? > > In particular, this paragraph is referencing the ability to widen the > accepted $value parameter type of the set hook, described at the > bottom of https://wiki.php.net/rfc/property-hooks#set. I have talked > to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that > this should not be complex to implement in PHPStan. In fact, PHPStan > already offers the @property-read and @property-write class > annotations, which can be used to describe "virtual" properties > handled within __get/__set, already providing asymmetric types of > sorts. Hence, this concern should be a non-issue. > > Thank you to everybody who has contributed to the discussion! > > Ilija
I was playing around with 3v4l.org. Is the implementation up-to-date there? I don't have any hard objections at the moment, but after playing with it for a while, I do kind of wonder if it's a lot of complexity for what is effectively a niche feature because: 1. It does not support asymmetric visibility for get/set. Having a public getter and private setter seems really natural. 2. You can't access accessors for "siblings". 3. You can't do by-reference set (important for arrays). 4. You can't satisfy a parent's readonly property with a getter in a child. Now, points 2 through 4 are fairly minor and niche by themselves, but if we take all these restrictions as a whole... I'm a bit worried. I also don't like the syntax. I can ignore this for the vote and still vote yes because I don't like the syntax, but with these other things... I'm worried.

Larry Garfield

2 years ago
On Tue, Apr 9, 2024, at 4:07 PM, Levi Morrison wrote:
> I was playing around with 3v4l.org. Is the implementation up-to-date there? > > I don't have any hard objections at the moment, but after playing with > it for a while, I do kind of wonder if it's a lot of complexity for > what is effectively a niche feature because: > > 1. It does not support asymmetric visibility for get/set. Having a > public getter and private setter seems really natural.
Aviz is a separate RFC, for reasons we've covered before: They're actually two separate features that do not depend on each other, so we split them up to help reduce RFC size (something RFC authors are often encouraged to do). The Aviz RFC was put to a vote last year but didn't pass. If hooks pass, we do want to revisit aviz and bring it to another vote, as the argument may be stronger now with hooks in place.
> 2. You can't access accessors for "siblings".
If a use case for this is found, that should be addable in a future RFC with no notable BC break. (Assuming a syntax of self::$otherProp::get() or similar, it would only have the same slight edge case as the parent::$thisProp::get() has, as documented in the RFC. As we've decided that is too edge-casey to care about here, presumably the same would hold true for such an follow-up.)
> 3. You can't do by-reference set (important for arrays).
This is due to logical limitations in the context. By-ref set means set hooks don't get called. So we can either block by-ref manipulation when a set hook is defined, or we can make set hooks easily-bypassable suggestions at best. We have not found any way around that decision. (And as noted in the RFC, this problem is already present if just using methods. It's not a new issue.) Please note that *the scope of what is blocked has been reduced considerably from earlier versions of the RFC!* The only invalid combination is &get/set on a backed property, because of the issue above. Writing to an array returned from a hook is allowed in certain circumstances, namely, if it was returned by &get and there is no set. So we're only preventing the narrowest set of operations we reasonably can, without hamstringing the concept of a set hook in the first place, and we don't believe there is any logical approach that would allow more. If in the future someone can come up with an approach that would work, it would likely be addable in a BC way.
> 4. You can't satisfy a parent's readonly property with a getter in a child.
In the last week or two we've figured out a situation where we may be able to allow this; iff the child getter works by reading the parent's value, then it would *probably* still be able to satisfy readonly's guarantees. We haven't tried implementing it yet as the RFC is large enough as is, but it's possible that could be added in the future. The main issue is that there's no way to fully guarantee that a get hook on a readonly property is idemopotent, the way a bare readonly property is. (It's not a lack of desire to support it, just the logical issues in ensuring different features' invariants are maintained.) Note that it's still unclear what set from a child set hook should do, given that readonly properties are also silently private-set, even if the property itself is protected or public. Potentially we could mandate that a child set MUST use parent::$prop::set(), so the actual write is technically in the parent class. I've noted this before as a design flaw in readonly that really needs to be corrected, but that's not a job for this RFC. (We actually had a solution in the aviz RFC, but people pushed back on it so we had to remove it.)
> Now, points 2 through 4 are fairly minor and niche by themselves, but > if we take all these restrictions as a whole... I'm a bit worried.
Hopefully the above comments make you less worried. :-) --Larry Garfield

Robert Landers

2 years ago
On Tue, Apr 9, 2024 at 8:56 PM Larry Garfield <larry@garfieldtech.com> wrote:
> > On Tue, Apr 9, 2024, at 4:07 PM, Levi Morrison wrote: > > > I was playing around with 3v4l.org. Is the implementation up-to-date there? > > > > I don't have any hard objections at the moment, but after playing with > > it for a while, I do kind of wonder if it's a lot of complexity for > > what is effectively a niche feature because: > > > > 1. It does not support asymmetric visibility for get/set. Having a > > public getter and private setter seems really natural. > > Aviz is a separate RFC, for reasons we've covered before: They're actually two separate features that do not depend on each other, so we split them up to help reduce RFC size (something RFC authors are often encouraged to do). The Aviz RFC was put to a vote last year but didn't pass. If hooks pass, we do want to revisit aviz and bring it to another vote, as the argument may be stronger now with hooks in place. > > > 2. You can't access accessors for "siblings". > > If a use case for this is found, that should be addable in a future RFC with no notable BC break. (Assuming a syntax of self::$otherProp::get() or similar, it would only have the same slight edge case as the parent::$thisProp::get() has, as documented in the RFC. As we've decided that is too edge-casey to care about here, presumably the same would hold true for such an follow-up.) > > > 3. You can't do by-reference set (important for arrays). > > This is due to logical limitations in the context. By-ref set means set hooks don't get called. So we can either block by-ref manipulation when a set hook is defined, or we can make set hooks easily-bypassable suggestions at best. We have not found any way around that decision. (And as noted in the RFC, this problem is already present if just using methods. It's not a new issue.) > > Please note that *the scope of what is blocked has been reduced considerably from earlier versions of the RFC!* The only invalid combination is &get/set on a backed property, because of the issue above. Writing to an array returned from a hook is allowed in certain circumstances, namely, if it was returned by &get and there is no set. > > So we're only preventing the narrowest set of operations we reasonably can, without hamstringing the concept of a set hook in the first place, and we don't believe there is any logical approach that would allow more. If in the future someone can come up with an approach that would work, it would likely be addable in a BC way. > > > 4. You can't satisfy a parent's readonly property with a getter in a child. > > In the last week or two we've figured out a situation where we may be able to allow this; iff the child getter works by reading the parent's value, then it would *probably* still be able to satisfy readonly's guarantees. We haven't tried implementing it yet as the RFC is large enough as is, but it's possible that could be added in the future. The main issue is that there's no way to fully guarantee that a get hook on a readonly property is idemopotent, the way a bare readonly property is. (It's not a lack of desire to support it, just the logical issues in ensuring different features' invariants are maintained.) > > Note that it's still unclear what set from a child set hook should do, given that readonly properties are also silently private-set, even if the property itself is protected or public. Potentially we could mandate that a child set MUST use parent::$prop::set(), so the actual write is technically in the parent class. I've noted this before as a design flaw in readonly that really needs to be corrected, but that's not a job for this RFC. (We actually had a solution in the aviz RFC, but people pushed back on it so we had to remove it.) > > > Now, points 2 through 4 are fairly minor and niche by themselves, but > > if we take all these restrictions as a whole... I'm a bit worried. > > Hopefully the above comments make you less worried. :-) > > --Larry Garfield
Off-topic random thought:
> The Aviz RFC was put to a vote last year but didn't pass.
It would be really nice if votes weren't just a yes/no vote, but yes/needs-more-work/no vote, where needs-more-work and no are effectively the same in terms of passing the RFC, but needs-more-work just means there is more to do (either addressing ugly syntax or the idea is sound, but as it says, it needs more work), and can thus be simply revoted on after concerns are addressed -- instead of creating a whole new RFC that needs to pass the "not too similar to other RFCs rule." I got the impression from the Aviz discussions that most people were against Aviz due to the syntax, not the feature itself. It would be absolutely tragic if this failed to pass simply because people expected Aviz here. Robert Landers Software Engineer Utrecht NL

Tim Düsterhus

2 years ago
Hi On 4/9/24 21:32, Robert Landers wrote:
>> The Aviz RFC was put to a vote last year but didn't pass. > > It would be really nice if votes weren't just a yes/no vote, but > yes/needs-more-work/no vote, where needs-more-work and no are > effectively the same in terms of passing the RFC, but needs-more-work > just means there is more to do (either addressing ugly syntax or the > idea is sound, but as it says, it needs more work), and can thus be > simply revoted on after concerns are addressed -- instead of creating > a whole new RFC that needs to pass the "not too similar to other RFCs > rule."
Re-proposing an RFC is allowed after 6 months even without substantial changes: https://github.com/php/policies/blob/main/feature-proposals.rst#resurrecting-rejected-proposals Though I'd argue that property hooks passing would be a substantial change of the broader context. -------------- Relatedly, I would find an official "Abstain" option useful. Any "Abstain" votes would be completely disregarded for the results, but it would allow voters to indicate that they've read the RFC, thought about it and don't sufficiently care either way to influence the results. As an RFC author I think it would be useful information to me: Did some folks not vote because they missed the RFC or because they are happy with either result? Best regards Tim Düsterhus

Ilija Tovilo

2 years ago
Hi Robert On Tue, Apr 9, 2024 at 9:34 PM Robert Landers <landers.robert@gmail.com> wrote:
> > On Tue, Apr 9, 2024 at 8:56 PM Larry Garfield <larry@garfieldtech.com> wrote: > > > > The Aviz RFC was put to a vote last year but didn't pass. > > It would be really nice if votes weren't just a yes/no vote, but > yes/needs-more-work/no vote, where needs-more-work and no are > effectively the same in terms of passing the RFC, but needs-more-work > just means there is more to do (either addressing ugly syntax or the > idea is sound, but as it says, it needs more work), and can thus be > simply revoted on after concerns are addressed -- instead of creating > a whole new RFC that needs to pass the "not too similar to other RFCs > rule."
The asymmetric visibility RFC did include a poll for no votes. https://wiki.php.net/rfc/asymmetric-visibility#proposed_voting_choices
> I got the impression from the Aviz discussions that most people were > against Aviz due to the syntax, not the feature itself. It would be > absolutely tragic if this failed to pass simply because people > expected Aviz here.
According to the poll, syntax was one, but not the primary reason for its rejection. The primary reason was that some people don't believe the feature is necessary at all. IIRC, people were arguing that readonly covers 80% of use-cases, because it protects against writes to the property both publicly and privately. I don't agree with this viewpoint, because I think readonly is bad for ergonomics. In fact, we already had an RFC that attempted to fix clone for readonly (https://wiki.php.net/rfc/readonly_amendments) but this fix was not complete (because it's still not possible to pass values from clone to __clone). "Clone with" is another thing needed to fix this, and at this point it just feels like applying more band-aids. For DTOs, I believe value types (i.e. data classes, https://externals.io/message/122845) solve the problem of "spooky actions at a distance" in a cleaner and more ergonomic way. For services and other intentional reference types, readonly often isn't the right choice either, just to make the property not publicly writable. Asymmetric visibility would be a much more fitting choice. Anyway, we didn't include asymmetric visibility in this RFC because: * We wanted to avoid getting rejected by people who fundamentally dislike asymmetric visibility. * We didn't feel it was fair to "sneak" the feature back in through some other RFC, when it was explicitly rejected. Instead, we are planning to re-propose asymmetric visibility once property hooks are merged, as it may become more apparent why it is useful. Ilija

Erick de Azevedo Lima

2 years ago
> Instead, we are planning to re-propose asymmetric visibility once > property hooks are merged, as it may become more apparent why it is > useful.
I was talking today to a co-worker about this internals e-mail discussion and he thought asymmetric visibility was inherent to property-hooks. He even was surprised by the possibility of having it without Aviz. He said that because he has a C# background. I really think that property hooks will highlight the benefits of Aviz. Hey, voter right beholders, please give this awesome feature a strong yes vote!
-- Regards, Erick

Nicolas Grekas

2 years ago
Hi Ilija, Larry, Heads-up: Larry and I would like to start the vote of the property
> hooks RFC tomorrow: > https://wiki.php.net/rfc/property-hooks >
I would make 3 changes to the RFC if I were in charge : - I'd make $this->propName inside a hook access the parent property via its hooks if any. I'd do this because that'd provide encapsulation-by-default for inheritance. Aka a parent class could reasonably expect its child classes to get through its hooks. (I'd be fine with only allowing the "=" operator on this.) - I'd keep the special $field value to access the backing value. It's a clear way to signify that accessing the raw value is possible only in hooks, never in other methods. - And I'd consider forbidding any method calls in hooks except static methods. This would allow factorising functional logic without opening for side-effects on the object outside of hooks themselves. I'm sharing this in case it rings a bell somewhere, but I can work with the peculiarities of the current RFC and I must admit I didn't spend as much time as you on the topic so my proposals might fall short, while yours to actually work. Thanks for that :) I'm in favor of the RFC. Cheers, Nicolas

Matthew Weier O'Phinney

2 years ago
On Mon, Apr 8, 2024 at 4:41 PM Ilija Tovilo <tovilo.ilija@gmail.com> wrote:
> Hi everyone > > Heads-up: Larry and I would like to start the vote of the property > hooks RFC tomorrow: > https://wiki.php.net/rfc/property-hooks > > We have worked long and hard on this RFC, and hope that we have found > some middle-ground that works for the majority. One last concern we > have not officially clarified on the list: > > https://externals.io/message/122445#122667 > >
After reading JRF's reply, I found myself nodding along. I feel like while a lot of the _details_ of my original feedback were addressed, the _themes_ largely were not, and many overlap with what Juliette noted. I've taken some time again today to review the proposal, and I think I can boil my main concern down to _consistency_, and _ensuring reviewer comprehension_ when somebody is reviewing code that includes hooks. New features in PHP really should veer away from allowing _implicit_ behavior, and should be internally consistent with existing syntax (unless they are attempting to make an explicit change to existing syntax). On this latest review, I've got fewer concerns, but I still have some. 1. I'm not a fan of _implicit_. I strongly feel that there's no use case for `set` without an argument. Requiring the argument makes explicit that it is _receiving_ a value, and also makes it explicit _at the definition point_ what value _type_ is accepted, and what it is _named_. (I realize I might be in a minority here, though, and the fact that I can require this as a CS rule (eventually) mitigates it to a degree. It feels like an opportunity to reduce errors by requiring it, however.) 2. I'm not a huge fan of the short syntax, but the improvements in the most recent draft are _mostly_ ones I can live with. The part that's still unclear is when and where hooks need a `;` termination, and _why_ the `;` is used, instead of `,`. When using `match()` expressions, you use `,` to separate the expressions, but for some reason, the proposal uses `;` ... but only when using short expressions. And if you have a full-form mixed with a short form, the `;` is only needed for the short-form expression. This feels arbitrary, and it will be easy to get it wrong for people comfortable with `match()` statements. My gut take is that the syntax should use `,` to separate hooks in ALL cases: ``` public string $content { get { $content = str_replace(' ', ' ', $this->content); return $this->convert($content); }, set(string $value) => $value, } public string $summary { get => $this->convert($this->summary), set(string $value) { $value = str_replace((' ', ' ', $value); $this->summary = $value; }, } ``` This more closely matches `match()` expressions (pardon the pun), and provides a template for how we might allow blocks in `match()` expressions in the future. (Larry tells me that it's `match()` being weird here, but considering that for many developers, their only point of reference for this sort of syntax IS `match()`, making it feel like the language is ignoring its own syntax when creating new syntax.) 3. While I'd likely prefer Marco's approach to references (just don't allow them), the fact that they mirror how `__get()` and `__set()` _currently_ work gives a migration path for users who are familiar with that paradigm's gotchas. In other words, it's consistent with the current language, and will make migrating from `__set/__get` to hooks easier. It's a lot of complexity, but the table you created helps with that. That table MUST make it to the docs for the feature! 4. The interaction with serialization has a LOT of different cases, and looks like a great place to observe foot guns in the future. I'd like to see a more consistent, predictable approach here, but knowing how many pitfalls there are in serialization normally, I'm not expecting one; at least it's being consistent with how each of the serialization methods already work. What I DO expect is that you create a table detailing the behavior for the docs, similar to the one you did for references/backed values/virtual. While I definitely feel for QA/CS tool makers (this is a HUGE chunk of new syntax, and non-trivial), I also don't know how this could be done in a way that would be simpler. My only suggestions would be to remove short-form syntax and require type/varname for `set` always, but even with those changes, I think similar levels of complexity will still exist to write parsers/formatters for these anyways. I personally have wanted these features in the language for likely 15 years. Maybe the foundation might be able to sponsor some developer time towards helping the QA/CS tool makers with these features once merged (assuming the RFC passes)?
> >> I personally do not feel strongly about whether asymmetric types make > it into the initial implementation. Larry does, however, and I think it is > not fair to exclude them without providing any concrete reasons not to. > [snip] > > > > My concern is more about the external impact of what is effectively a > change to the type system of the language: [snip] will tools like PhpStan > and Psalm require complex changes to analyse code using such properties? > > In particular, this paragraph is referencing the ability to widen the > accepted $value parameter type of the set hook, described at the > bottom of https://wiki.php.net/rfc/property-hooks#set. I have talked > to Ondřej Mirtes, the maintainer of PHPStan, and he confirmed that > this should not be complex to implement in PHPStan. In fact, PHPStan > already offers the @property-read and @property-write class > annotations, which can be used to describe "virtual" properties > handled within __get/__set, already providing asymmetric types of > sorts. Hence, this concern should be a non-issue. > > Thank you to everybody who has contributed to the discussion! > > Ilija >
-- Matthew Weier O'Phinney mweierophinney@gmail.com https://mwop.net/ he/him

Ilija Tovilo

2 years ago
Hi Matthew We're going to skip over the reiterations of Juliettes arguments, as they have already been responded to. On Thu, Apr 11, 2024 at 12:08 AM Matthew Weier O'Phinney <mweierophinney@gmail.com> wrote:
> > On Mon, Apr 8, 2024 at 4:41 PM Ilija Tovilo <tovilo.ilija@gmail.com> wrote: >> >> https://externals.io/message/122445#122667 >> > > 2. I'm not a huge fan of the short syntax, but the improvements in the most recent draft are _mostly_ ones I can live with. The part that's still unclear is when and where hooks need a `;` termination, and _why_ the `;` is used, instead of `,`. When using `match()` expressions, you use `,` to separate the expressions, but for some reason, the proposal uses `;` ... but only when using short expressions. And if you have a full-form mixed with a short form, the `;` is only needed for the short-form expression. This feels arbitrary, and it will be easy to get it wrong for people comfortable with `match()` statements.
I agree that the distinction of `,` and `;` isn't clear-cut. I would categorize hooks as declarations, because they are really just functions attached to the property. Declarations are `;`-terminated, or have a body (`{}`) (properties, (non-)abstract methods, class consts, etc). `,`, on the other hand, is used for lists of expressions and other things (array elements, match cases, function arguments). The other argument for the syntax we chose is that it's already used in C#.
> (Larry tells me that it's `match()` being weird here, but considering that for many developers, their only point of reference for this sort of syntax IS `match()`, making it feel like the language is ignoring its own syntax when creating new syntax.)
Given my description from above, I don't believe this is true either. Match arms most closely relate to the array element syntax, which already uses `,`.
> 3. While I'd likely prefer Marco's approach to references (just don't allow them), the fact that they mirror how `__get()` and `__set()` _currently_ work gives a migration path for users who are familiar with that paradigm's gotchas. In other words, it's consistent with the current language, and will make migrating from `__set/__get` to hooks easier. It's a lot of complexity, but the table you created helps with that. That table MUST make it to the docs for the feature!
Precisely. Our decision to support references (as much as possible) comes from the desire to maintain compatibility (as much as possible), not only with `__get`/`__set` but also plain props. Ilija