Re: Asymmetric visibility is a BC break

On Mon, 14/10/2024 at 06:01, Larry Garfield <larry@garfieldtech.com> wrote: > > On Sun, Oct 13, 2024, at 9:37 PM, Valentin Udaltsov wrote: > > > > First of all, I have already agreed above that PHP does not have a BC > > break here. Now we are discussing the potential problems in the PHP > > ecosystem and how they could be mitigated. > > Ilija and I have discussed this issue a bit. > > The first issue is that isPublic() technically means "does this property have the public > flag set," and nothing more. Prior to 8.1, that implicitly also meant "can the property be > read and written to from public scope," because of how properties worked. (And same for > isProtected().) That implicit assumption became invalid in 8.1 with readonly, which stealth > introduced limited and not fully designed asymmetric visibility as well as properties that could not > be set multiple times from any scope. Full aviz in 8.4 doesn't change that. It just makes the > previous assumption change more apparent. The fact that no one seems to have reported it as an issue > until now suggests it's not a particularly widespread problem. In practice, if someone is > using reflection to determine the visibility of a property, they'll be writing to it through > reflection as well if at all. > > The best solution here is probably to just clarify the docs, which I will do as part of the > aviz docs that I have already been working on. cf: > https://github.com/php/doc-en/pull/3828 > > The second issue is that the behavior of isProtectedSet() / isPrivateSet() was not as clearly > defined in the RFC as it should have been. That's on us for not being clearer, as we apologize > for the oversight. > > Those methods follow the low level pattern of isPublic() , that is, they just report of a given > flag is set, not what the implications of that flag in various contexts are. That is consistent with > the rest of the reflection API, so we feel that is best left as-is. > > That still means the "so can I read/write this property or not?" question has no > simple operation for it. Again: it never did, we just kinda sorta had it indirectly and implicitly. > For that we feel the best answer, as well as least disruptive given we're in RCs, is dedicated > methods as Ilija has already described that take all property behavior and context into account. > (isReadable and isWriteable.) > > As a reminder, the concept is: > > $rProp->isReadable($obj); // Can this property on $obj be read from the calling scope? > $rProp->isReadable($obj, 'static'); // Same as previous. > $rProp->isReadable($obj, null); // Can this property on $obj be read from global scope? > $rProp->isReadable($obj, Foo::class); // Can this property on $obj be read from code inside > class Foo? > > $rProp->isWriteable($obj); // Can this property on object $obj be written from the calling > scope? > $rProp->isWriteable($obj, 'static'); // Same as previous. > $rProp->isWriteable($obj, null); // Can this property on object $obj be written from global > scope? > $rProp->isWriteable($obj, Foo::class); // Can this property on object $obj be written from > code inside class Foo? > > cf: https://github.com/php/php-src/pull/16209 > > (The use of null to indicate global scope is borrowed from Closure::bind(), which does the > same.) > > These methods do runtime analysis to see if a property should be readable/writeable. > Specifically: > > isReadable() > * Checks that the property is readable from the passed scope > * Checks that the property is initialized (i.e. not typed and never written to) > * Checks that the property is not virtual or has a get hook > isWritable() > * Checks that the property is writable (respecting symmetric and asymmetric properties) from > the passed scope > * Checks that the property is not readonly, is not yet initialized, or is reinitializable > (__clone) > * Checks that the property is not virtual or has a set hook > > Of note, this does not absolutely guarantee that a read/write will succeed. There's at > least two exceptions: One, some PHP built-in classes have effectively immutable properties but do > not use readonly or private(set). > Those would not be detected here, until and unless they are updated to use the now-available > mechanisms. (See, eg: https://github.com/php/php-src/issues/15309) > The other is that a get or set hook may throw an exception under various circumstances. There is no > way to evaluate that via reflection, so it's a gap that will necessarily always be there. > > Whether those methods are OK to add in the RC phase or if they should be left to early 8.5, and > if they would need a formal RFC, is up to the RMs to decide. RMs, what is your preference? > > --Larry Garfield Hi, Larry! Thank you very much for this response. I agree with every point. I have only one comment about the methods' signatures. They should not require an object, because properties can be static or the developer might want to check writable/readable without instantiating an object (in a code generator, for instance). Then it makes sense to make $object the 2nd parameter. This will also be consistent with Closure::bind(). isReadable(?string $scope = 'static', ?object $object = null): bool isWritable(?string $scope = 'static', ?object $object = null): bool -- Best regards, Valentin