Skip to content

Make ReflectionProperty#setAccessible() and ReflectionMethod#setAccessible() no-op #5412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Make ReflectionProperty#setAccessible() and ReflectionMethod#setAccessible() no-op #5412

wants to merge 1 commit into from

Conversation

Ocramius
Copy link
Contributor

@Ocramius Ocramius commented Apr 17, 2020 •
edited
Loading

RFC: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op

Follow-up to #5411: also removes the ReflectionProperty#setAccessible(false) and ReflectionMethod#setAccessible(false).

Make ReflectionProperty#setAccessible() and ReflectionMethod#setAccessible() no-op

With this patch, it is no longer required to call ReflectionProperty#setAccessible() or
ReflectionMethod#setAccessible() at all: calling these methods will no longer have any
effect on the behavior of ReflectionMethod and ReflectionProperty.

If a userland consumer already got to the point of accessing object/class information via reflection,
it makes little sense for ext/reflection to disallow accessing private/protected symbols ever.

TODOs

  • squash
  • add full RFC text to commit message

@Ocramius Ocramius mentioned this pull request Apr 18, 2020
Closed
@nikic
Copy link
Member

nikic commented Apr 20, 2020

I'd probably also add a deprecation if setAccessible(false) is called that tells you that it doesn't do anything. We can leave setAccessible(true) alone to avoid needlessly complicating cross version code.

@Ocramius
Copy link
Contributor Author

Similar to #5411 (comment), I would probably open an alternate PR to add the deprecation there.

Effectively 3 levels:

  1. change the default (only)
  2. make setAccessible() a dummy
  3. deprecate setAccessible() and make it a dummy

@Ocramius Ocramius force-pushed the feature/remove-effects-of-reflection-property-and-method-accessibility branch 2 times, most recently from 46751dc to f3fb39a Compare June 13, 2021 15:45
@Ocramius
Copy link
Contributor Author

Ocramius commented Jun 13, 2021 •
edited
Loading

Cleaned this up: will raise an RFC, but an explicit deprecation of Reflection*#setAccessible() will only be viable after https://wiki.php.net/rfc/deprecated_attribute is discussed and moved on.

Adding a runtime side-effect here is worse, and I'd just avoid that.

I'll mention this decision in the RFC.

EDIT: deferred to later RFC about removal of setAccessible(), should this RFC be accepted.

@beberlei
Copy link
Contributor

@Ocramius imho the dependency on the attribute RFC is not really there, ad it would also trigger the runtime deprecation. Marking the method as deprecated is PHP project convention, so your preference against runtime deprecations is ranked lower here :)

@Ocramius
Copy link
Contributor Author

Ocramius commented Jun 13, 2021 •
edited
Loading

so your preference against runtime deprecations is ranked lower here :)

Will roll with it anyway: runtime side-effects are more damaging anyway.

Again, let's discuss that on the RFC, if needed, but I don't see an immediate need for any deprecation to be raised either.

EDIT: I've explicitly deferred the deprecation to a future RFC: any inter-dependencies and side-effects are valuable/to be discussed for 8.2+.

@Ocramius
Make ReflectionProperty and ReflectionMethod access private/`pr…
cad43f4 
…otected` symbols by default

With this patch, it is no longer required to call `ReflectionProperty#setAccessible()` or
`ReflectionMethod#setAccessible()` with `true`.

If a userland consumer already got to the point of accessing object/class information via reflection,
it makes little sense for `ext/reflection` to disallow accessing `private`/`protected` symbols by
default.

After this patch, calling `ReflectionProperty#setAccessible(true)` or
`ReflectionMethod#setAccessible(true)` on newly instantiated `ReflectionProperty` or
`ReflectionMethod` respectively will have no effect.

Full RFC text for the voted upon https://wiki.php.net/rfc/make-reflection-setaccessible-no-op is as follows:

```
====== PHP RFC: Make reflection setAccessible() no-op ======

  * Version: 1.0
  * Date: 2021-06-13
  * Author: Marco Pivetta
  * Status: Accepted
  * First Published at: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op

===== Introduction =====

The `ext-reflection` API is designed to inspect static details of a code-base, as well as reading and manipulating runtime state and calling internal details of objects that are otherwise inaccessible.

These methods are most notably:

  * `ReflectionMethod#invoke(): mixed`
  * `ReflectionMethod#invokeArgs(mixed ...$args): mixed`
  * `ReflectionProperty#getValue(object $object): mixed`
  * `ReflectionProperty#setValue(object $object, mixed $value): void`

While breaking encapsulation principles that allow for safe coding practices, these methods are extremely valuable to tools like:

  * mappers
  * serializers
  * debuggers
  * etc.

Infrastructural instrumentation is often required to do things that are in direct conflict with encapsulation itself.

The 4 methods listed above change behavior depending on the only mutable state within the scope of `ext-reflection` classes, which is an "accessible" flag. This "accessibility" flag is steered by:

  * `ReflectionMethod#setAccessible(bool $accessible): void`
  * `ReflectionProperty#setAccessible(bool $accessible): void`

Attempting to use any of the above listed methods without configuring accessibility first will lead to an exception being thrown. For example:

<code php>
class Foo { private $bar = 'a'; }

(new ReflectionProperty(Foo::class, 'bar'))->getValue();
</code>

https://3v4l.org/ousrD :

<code>
Fatal error: Uncaught ReflectionException: Cannot access non-public property Foo::$bar in <SNIP>
</code>

==== The problem with mutability ====

By having `ReflectionProperty#setAccessible()` and `ReflectionMethod#setAccessible()`, any consumer of a `ReflectionMethod` or `ReflectionProperty` that is given by a third party must ensure that `#setAccessible()` is called:

<code php>
function doSomethingWithState(MyObject $o, ReflectionProperty $p) : void
{
    $p->setAccessible(true); // wasteful safety check

    doSomethingWith($p->getValue($o));
}
</code>

In addition to that, any developer that is intentionally using the reflection API (after having evaluated its trade-off) will have to use this obnoxious syntax in order to use it at its fullest:

<code php>
$p = new ReflectionProperty(MyClass::class, 'propertyName');

$p->setAccessible(true);

// now $p is usable
</code>

===== Proposal =====

This RFC proposes to:

  * make `ReflectionProperty` and `ReflectionMethod` behave as if `#setAccessible(true)` had been called upfront
  * make `ReflectionProperty#setAccessible()` and `ReflectionMethod#setAccessible()` no-op operations, with no side-effects nor state mutation involved

After the RFC is successfully accepted/implemented, the following code should no longer throw, improving therefore the ergonomics around reflection.

<code php>
class Foo { private $bar = 'a'; }

(new ReflectionProperty(Foo::class, 'bar'))->getValue();
</code>

==== Deprecations ====

In order to ease migration to PHP 8.1, and minimize runtime side-effects, a deprecation is explicitly avoided in this RFC.

Instead, a deprecation should be introduced when a new/separate RFC plans for the removal of `ReflectionProperty#setAccessible()` and `ReflectionMethod#setAccessible()`.

Such RFC will be raised **after** the release of PHP 8.1, if this RFC is accepted.

===== Backward Incompatible Changes =====

Although of minimal concern, it is true that some behavior will change:

  * `ReflectionProperty#getValue()` will no longer throw an exception when used against a protected/private property
  * `ReflectionProperty#setValue()` will no longer throw an exception when used against a protected/private property
  * `ReflectionMethod#invoke()` will no longer throw an exception when used against a protected/private method
  * `ReflectionMethod#invokeArgs()` will no longer throw an exception when used against a protected/private method
  * for extensions developers, `reflection_object->ignore_visibility` no longer exists

===== Proposed PHP Version(s) =====

8.1

===== RFC Impact =====
==== To SAPIs ====
None

==== To Existing Extensions ====
None

==== To Opcache ====

None

==== New Constants ====

None

==== php.ini Defaults ====

None

===== Open Issues =====

None

===== Proposed Voting Choices =====

Accept turning `ReflectionProperty#setAccessible()` and `ReflectionMethod#setAccessible()` into a no-op? (yes/no)

===== Patches and Tests =====

php#5412

===== Vote =====

This is a Yes/No vote, requiring a 2/3 majority. Voting started on 2021-06-23 and ends on 2021-07-07.

<doodle title="Make reflection setAccessible() no-op" auth="ocramius" voteType="single" closed="true">
   * Yes
   * No
</doodle>
```
@Ocramius Ocramius force-pushed the feature/remove-effects-of-reflection-property-and-method-accessibility branch from f3fb39a to cad43f4 Compare July 7, 2021 15:17
@nikic nikic added this to the PHP 8.1 milestone Jul 7, 2021
Ocramius
Ocramius commented Jul 7, 2021
View reviewed changes
Copy link
Contributor Author

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickallaert or @ramsey can you take this to merge into master and into 8.1.x?, according to voting results?

Ref:

Waited for green build before pinging :)

@nikic nikic closed this in 6e16e1d Jul 8, 2021
@Ocramius Ocramius deleted the feature/remove-effects-of-reflection-property-and-method-accessibility branch July 8, 2021 08:57
Ocramius added a commit to Ocramius/psalm that referenced this pull request Dec 7, 2022
@Ocramius
Mark Reflection(Method|Property)#setAccessible() as pure starting f…
ed2cde1 
…rom PHP 8.1 onwards

This will highlight unused code.

Ref: php/php-src#5412
Ref: https://wiki.php.net/rfc/make-reflection-setaccessible-no-op
Ref: php/php-src#5411

Example https://3v4l.org/PNeeZ

```php
<?php

class Foo {
    private $bar = 'baz';
    private function taz() { return 'waz'; }
}

//var_dump((new ReflectionProperty(Foo::class, 'bar'))->getValue(new Foo));
//var_dump((new ReflectionMethod(Foo::class, 'taz'))->invoke(new Foo));
```

Produces (starting from PHP 8.1):

```
string(3) "baz"
string(3) "waz"
```
Ocramius added a commit to Roave/SecurityAdvisoriesBuilder that referenced this pull request Dec 19, 2022
@Ocramius
Removed some redundant Reflection*#setAccessible() calls
c1213ea 
These calls are no longer needed as per:

 * https://wiki.php.net/rfc/make-reflection-setaccessible-no-op
 * php/php-src#5412
Ocramius added a commit to Roave/you-are-using-it-wrong that referenced this pull request Dec 19, 2022
@Ocramius
Removed some redundant Reflection*#setAccessible()` calls
bf7f194 
These calls are no longer needed as per:

 * https://wiki.php.net/rfc/make-reflection-setaccessible-no-op
 * php/php-src#5412
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
RFC
Projects
None yet
Milestone
PHP 8.1
Development

Successfully merging this pull request may close these issues.
4 participants
@Ocramius @nikic @beberlei @carusogabriel