Re: [RFC] Asymmetric Visibility, with readonly

> On 2 Dec 2022, at 01:21, Larry Garfield <larry@garfieldtech.com> wrote: > > On Wed, Nov 30, 2022, at 7:38 PM, Stephen Reay wrote: > >>>> So please, can you explain to me why consistency with an implementation >>>> detail of readonly properties is more important than consistency with >>>> declared developer intention for regular properties via the magic >>>> setter method? >>> There's a couple of reasons. >>> One, and arguably the most important, readonly and aviz being incompatible is, >>> hopefully, a temporary situation. There's some fiddly bits to work out design-wise, and based >>> on earlier comments in the thread we're going to punt on that for now to avoid that dragging >>> down the whole RFC. I believe we absolutely should allow them together in the future (maybe in a >>> later 8.3 RFC, maybe a future version, TBD), which means ensuring, now, that they are compatible in >>> the future. This approach involves the fewest future BC breaks.. >>> Second, I wouldn't call the current behavior of readonly a mere implementation >>> detail. It's weird and unexpected, I'd agree, but only as a side effect of previous >>> design decisions, some of which are even older than readonly.. But it's an observed behavior >>> that code can rely on, and in some cases does. For example: >>> >>> https://peakd.com/hive-168588/@crell/php-tricks-lazy-public-readonly-properties >>> The "unset a declared property to force it through __get once" is a trick >>> that some ORMs use extensively. readonly just inherited that, leading to the current behavior: >>> __set depends on the write/set visibility of the property and its settedness. This RFC doesn't >>> change anything there. >>> The alternative would be to have __set called always for a non-public-set property. >>> However, that is a place for bugs, as you then can't not have a back-door way to publicly set a >>> property even if it's declared private(set).. (Or, you have to be very careful in your __set >>> to avoid it.) That is both inconsistent with the language today, and error prone. >>> Finally, we're planning to work in the near-future on property hooks (aka property >>> accessors), which would allow per-property custom set routines. That would largely remove the issue >>> entirely, as the use of __set would go way down and you'd basically never have to use it with a >>> declared property, fancy tricks or no, so this issue would never come up at all. >>> --Larry Garfield >>> -- >>> PHP Internals - PHP Runtime Development Mailing List >>> To unsubscribe, visit: https://www.php.net/unsub.php >> Hi Larry, >> I think there must be some confusion somewhere, based on some of your comments. >> I’m not suggesting that the “unset to force a **public** property to go >> through getter/setter methods” logic should be specifically different. >> I’m suggesting that when the decision is made to call __set or not, the >> properties **set** visibility is what should be considered, not it’s >> **get** visibility. >> Your own comment even describes this behaviour: "leading to the current >> behavior: __set depends on the write/set visibility of the property" >> But your RFC says that __set will depend on the **read/get** visibility >> of the property. >>> you then can't not have a back-door way to publicly set a property even if >>> it's declared private(set). (Or, you have to be very careful in your __set to avoid it.) That >>> is both inconsistent with the language today, and error prone. >> If a developer adds a _set() method that can write to a private(set) >> property, I would expect that is working exactly as desired, exactly as >> it does **now** where it’s just a “protected” property. > > I think we're talking past each other a bit. :-) > > The logic described in the RFC is, to the best of out knowledge, what already happens in > 8.1/8.2 with readonly. Whether that is good or bad, obvious or intuitive, it's what PHP > already does, for better or worse. We view readonly as, effectively, a shorthand for > "private(set) write-once" (which is what it is), and want to ensure that future RFCs can > do that explicitly so that we can allow public protected(set) > readonly in the future. > > For that to be possible, not changing the existing behavior is a necessity.. Changing the > behavior described in the RFC right now is a BC break. That's true whether the logic described > in the RFC is good or not, because that's how it already works with > readonly. > > Is it weird that __set depends in part on read visibility? Kinda, yeah. But that's > already the behavior in 8.1/8.2. We're not changing anything, and this RFC is not the place to > break that kind of BC. > > If someone can demonstrate that the logic described there is not what actually happens now, > please let us know, because the goal is to not change any behavior in this regard. Effectively, > that whole section is descriptive of PHP today and a comment "and we're not breaking > it." > > --Larry Garfield > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php Hi Larry, Given that you agree the current behaviour is “a bit weird” why not keep the current limitation tied specifically to the readonly keyword then? I can actually see a scenario (an “edge case” as mentioned as the reasoning for the limitation in the readonly rfc) where weird behaviour could be observed with readonly - logic performed in the __set method that assumes the write/set below will work. The same isn’t true for non-readonly. I believe all or most edge-cases that the limitation is meant to prevent would likely be related specifically to a field being readonly, not to it being implicitly asymmetrically visible. So here’s my last attempt: Please change this behaviour in your rfc. You are explicitly making it mutually exclusive with readonly now, so that’s not a bc break - if/when it becomes compatible with readonly the authors of that rfc can either keep the limitation as it exists **for readonly properties** (I’m not an expert but I don’t believe this would be a BC break either), or decide to drop the limitation completely (deliberate BC break) Keeping the limitation now on non-readonly properties makes no sense, and will be confusing to user land developers. Cheers Stephen