Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125795
Feedback-ID: i8414410d:Fastmail
Precedence: bulk
MIME-Version: 1.0
Date: Sun, 13 Oct 2024 21:57:37 -0500
To: "php internals" <internals@lists.php.net>
Message-ID: <c1eb871f-4991-4794-9a29-e8ee244c0d7c@app.fastmail.com>
In-Reply-To: 
 <CAPyeZ4O0Dc9ggrhCLBsnihqQcuBTr9EMSAXFBf6N+vwAy49uiw@mail.gmail.com>
References: 
 <CAPyeZ4N8GMiLNVCDGxdofcy6txBhng_i2UKUGpiS5bnZ-YcYvA@mail.gmail.com>
 <2A7CF24F-3AE3-4125-965F-C65431C42DFB@gmail.com>
 <CAPyeZ4MSc=Sw9UE0soCdBEnVAJ8wXHHhYqm88OffkvNLrHPxFw@mail.gmail.com>
 <e966cf16-794d-4827-9a5e-b0de780a5b91@app.fastmail.com>
 <30a41608-a1ea-40a9-8d2a-c53c508cd89f@jnvsor.net>
 <a6cdfe39-2a18-40b1-8da5-f49e9e7a7918@app.fastmail.com>
 <CAMrTa2FM4o6q8TXdBbuHfnD66vtqFBVL2Hw6_412Bf1r2T1QMg@mail.gmail.com>
 <CAPyeZ4M0LDzPBTjT_Qa-ccB73DNatGWdARg-mKMO1T1XmnTzUg@mail.gmail.com>
 <CAMrTa2HMx0ZQJ8OMAcyjkGjQfdbCZ5h5u7seS9gP0pzFNYUCsg@mail.gmail.com>
 <CAPyeZ4O0Dc9ggrhCLBsnihqQcuBTr9EMSAXFBf6N+vwAy49uiw@mail.gmail.com>
Subject: Re: [PHP-DEV] Asymmetric visibility is a BC break
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
From: larry@garfieldtech.com ("Larry Garfield")

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