> On 2 Dec 2022, at 01:21, Larry Garfield <la...@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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
