[RFC] make Reflection*#setAccessible() no-op

php.internals

Marco Pivetta

5 years ago
Hey folks, I'm posting here to introduce a new simplification, as well as quality-of-life-improving RFC: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op The RFC is quite minimal, and proposes removing any runtime behavior from `ReflectionMethod#setAccessible()` and `ReflectionProperty#setAccessible()`, making `ReflectionMethod` and `ReflectionProperty` accessible by default. The rationale is: * this API is probably coming from a copy-pasted java-ism (although I couldn't verify that, so I did not factor it into the RFC) * removes the last bit of mutable state from `ReflectionProperty` and `ReflectionMethod` * simplifies usage of the API * if I'm up to no good, I don't need to actually solemnly swear that i am up to no good (that's stuff for fantasy books) I don't really know what the deadline for 8.1 features is, but I assume it's coming up quite quickly, so friendly NikiC poked me to see if this long-standing patch of mine was still relevant. Should be short/sweet, but I'm looking forward to your feedback. Greets, Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

Larry Garfield

5 years ago
On Sun, Jun 13, 2021, at 11:44 AM, Marco Pivetta wrote:
> Hey folks, > > I'm posting here to introduce a new simplification, as well as > quality-of-life-improving RFC: > > https://wiki.php.net/rfc/make-reflection-setaccessible-no-op > > The RFC is quite minimal, and proposes removing any runtime behavior from > `ReflectionMethod#setAccessible()` and > `ReflectionProperty#setAccessible()`, making `ReflectionMethod` and > `ReflectionProperty` accessible by default. > > The rationale is: > > * this API is probably coming from a copy-pasted java-ism (although I > couldn't verify that, so I did not factor it into the RFC) > * removes the last bit of mutable state from `ReflectionProperty` and > `ReflectionMethod` > * simplifies usage of the API > * if I'm up to no good, I don't need to actually solemnly swear that i am > up to no good (that's stuff for fantasy books) > > I don't really know what the deadline for 8.1 features is, but I assume > it's coming up quite quickly, so friendly NikiC poked me to see if this > long-standing patch of mine was still relevant. > > Should be short/sweet, but I'm looking forward to your feedback.
I'm generally OK with it. The change makes sense. What I'm unclear on is why it's not including the actual deprecation. Not doing that yet and having it just become a noop first seems fine, but it's also clear that having it throw a deprecation in the future, and presumably then having those methods be removed outright, is the long-term plan. I'm good with that too, but I don't see why the full death-cycle isn't included, and half of it is instead punted to a future RFC. Please explain. --Larry Garfield

Marco Pivetta

5 years ago
Hey Larry, On Sun, Jun 13, 2021, 20:46 Larry Garfield <larry@garfieldtech.com> wrote:
> I'm generally OK with it. The change makes sense. > > What I'm unclear on is why it's not including the actual deprecation. Not > doing that yet and having it just become a noop first seems fine, but it's > also clear that having it throw a deprecation in the future, and presumably > then having those methods be removed outright, is the long-term plan. I'm > good with that too, but I don't see why the full death-cycle isn't > included, and half of it is instead punted to a future RFC. >
Two reasons:
>
1. I don't see a removal of the methods as necessary at all anyway: while they are no-op, removal increases the scope of the RFC too much, and it's really not worth going there for the added work, when a further RFC would make it more mechanical/isolated for 8.2+ and 9.0 afterwards. If the methods survived in 9.0 too, it would be almost no impact anyway. 2. The usage of E_DEPRECATED inside php-src and in the composer ecosystem at large irks me, a lot. Since we operate on a scripting language, we defer things to the runtime, and by doing so we turn pure APIs into impure ones, and lead to potential production disruption (happened to me multiple times, precisely because of deprecations). I believe we don't have the right tools to address deprecations upfront **yet**, and I'm mostly hoping for the `#[Deprecated]` attribute to make its appearance before introducing yet another harmful `trigger_error()` call. In this case, we have a clean cut of API that can be **declared** deprecated, rather than turned into a runtime problem: I want to see if the situation improves, before creating new problems. Would it help if I raised a second Draft RFC together with this one, so that the promise of dealing with this later on is already fulfilled?

tyson andre

5 years ago
Hi Marco Pivetta, > I'm posting here to introduce a new simplification, as well as > quality-of-life-improving RFC: > > https://wiki.php.net/rfc/make-reflection-setaccessible-no-op > > The RFC is quite minimal, and proposes removing any runtime behavior from > `ReflectionMethod#setAccessible()` and > `ReflectionProperty#setAccessible()`, making `ReflectionMethod` and > `ReflectionProperty` accessible by default. > > The rationale is: > >  * this API is probably coming from a copy-pasted java-ism (although I > couldn't verify that, so I did not factor it into the RFC) >  * removes the last bit of mutable state from `ReflectionProperty` and > `ReflectionMethod` >  * simplifies usage of the API >  * if I'm up to no good, I don't need to actually solemnly swear that i am > up to no good (that's stuff for fantasy books) > > I don't really know what the deadline for 8.1 features is, but I assume > it's coming up quite quickly, so friendly NikiC poked me to see if this > long-standing patch of mine was still relevant. > > Should be short/sweet, but I'm looking forward to your feedback. The deadline for new features for 8.1 is July 20: https://wiki.php.net/todo/php81 (with some discretion from the release managers, e.g. for amendments to changes already made in 8.1) I think that isAccessible should be added if any applications actually did depend on ReflectionException being thrown for correctness - they could throw their own exception if isAccessible was false. (e.g. for code meant to handle possibly undefined public typed properties by checking for initialization then getting the value) I can't actually remember needing this personally, though, since `$obj->{$method}()` could be used. I've only used this to access private and protected properties/methods. Regards, Tyson

Marco Pivetta

5 years ago
Hey folks, On Sun, Jun 13, 2021 at 6:44 PM Marco Pivetta <ocramius@gmail.com> wrote:
> Hey folks, > > I'm posting here to introduce a new simplification, as well as > quality-of-life-improving RFC: > > https://wiki.php.net/rfc/make-reflection-setaccessible-no-op > > The RFC is quite minimal, and proposes removing any runtime behavior from > `ReflectionMethod#setAccessible()` and > `ReflectionProperty#setAccessible()`, making `ReflectionMethod` and > `ReflectionProperty` accessible by default. >
Since feedback has been relatively minimal and positive, I will likely start voting tomorrow, since I don't want trouble with any upcoming 8.1 deadline (plus I'll otherwise just forget). Greets, Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

Christoph Becker

5 years ago
On 21.06.2021 at 17:53, Marco Pivetta wrote:
> On Sun, Jun 13, 2021 at 6:44 PM Marco Pivetta <ocramius@gmail.com> wrote: > >> I'm posting here to introduce a new simplification, as well as >> quality-of-life-improving RFC: >> >> https://wiki.php.net/rfc/make-reflection-setaccessible-no-op >> >> The RFC is quite minimal, and proposes removing any runtime behavior from >> `ReflectionMethod#setAccessible()` and >> `ReflectionProperty#setAccessible()`, making `ReflectionMethod` and >> `ReflectionProperty` accessible by default. > > Since feedback has been relatively minimal and positive, I will likely > start voting tomorrow, since I don't want trouble with any upcoming 8.1 > deadline (plus I'll otherwise just forget).
Given that private constants and static properties already can be accessed right away, I'm in favor of this RFC.
-- Christoph M. Becker