On Mon, 14/10/2024 at 06:01, Larry Garfield <la...@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
