Re: [RFC] Asymmetric Visibility, with readonly

> On 30 Nov 2022, at 22:09, Larry Garfield <larry@garfieldtech.com> wrote: > > On Tue, Nov 29, 2022, at 11:25 PM, Stephen Reay wrote: > >> Hi Larry, >> >> Thank you for clarifying the setter behaviour in more explicit terms, >> but I have to say I’m quite disappointed in this continued “use the >> logic of readonly to apply to something that is explicitly not >> readonly” - this is even more stark now that you’ve explicitly made >> them mutually exclusive behaviours. >> >> I’m generally very in favour of maintaining consistency, but this seems >> like it’s using technical consistency as an excuse to justify >> unintuitive behaviour that breaks consistency in another, much more >> obvious way. >> >> >> Can you explain why it makes more sense to maintain consistency with >> “readonly” than it does to maintain consistency with the existing >> “__set()” behaviour for properties, particularly now that you’ve >> indicated these features (asymmetric visibility and readonly) are >> mutually exclusive? >> >> While it’s stated multiple times that “readonly” introduced a limited >> form of asymmetric visibility, and thus this is a continuation, in >> terms of intuitiveness, the existing __set() rules are very easy to >> comprehend even with readonly: >> >> - if the property is declared as public, __set() is never called; if >> it’s declared as protected, __set is called when the property is >> accessed from outside that class or it’s hierarchy. Yes, I know that >> readonly imposes an implicit visibility difference - but that is >> essentially an implementation detail, from the point of view of the >> userland developer, it’s not a clear statement of intended behaviour on >> their part, expressed through the code as written. >> >> For example, with public readonly int $foo it’s quite >> obvious why >> __set() isn’t called, using the exiting well-understood logic: it’s a >> public property. PHP applies a kind of asymmetric visibility to the >> property behind the scenes, but that isn’t what the developer declared, >> it’s the implementation. This behaviour matches that of regular, >> non-readonly fields: when the field is declared public (or has implicit >> public visibility) __set() is never called. >> >> If we make that field protected, __set() will be called when the >> property is written to from outside the class, regardless of whether >> it’s readonly or not. >> >> >> What you’re proposing changes that, in a way that is completely >> unintuitive: when attempting to *write* data to a property that is >> marked as protected(set), the __set() method will not be called. >> >> >> 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. > we're planning to work in the near-future on property hooks That’s great, but I don’t think weird unintuitive behaviour should be added because “well we’re gonna try and solve this through something else later” Cheers Stephen