Skip to content

Make ReflectionProperty and ReflectionMethod access private/protected symbols by default #5411

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.

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

Conversation

Ocramius
Copy link
Contributor

Note: pending RFC.

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.

…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.
@Ocramius
Copy link
Contributor Author

Note: please ignore any failures - local php-src build environment doesn't work for me, so I basically yolo-pushed it to let travis do the hard work for me.

@beberlei
Copy link
Contributor

Does it make sense to keep ->setAccessible(false) in this case? I would tend to remove this functionality alltogether and get rid of the ignore_visibility variable and update the code in setAccessible to ignore that flag. Also maybe we could then raise an E_DEPRECATED error that this function is not needed anymore.

@Ocramius
Copy link
Contributor Author

as discussed in chat, #5412 implements a more aggressive version of this.

I plan to open a third PR with a deprecation too.

@nikic
Copy link
Member

nikic commented Jun 2, 2021

Do you have any plans to move forward with this change?

@Ocramius
Copy link
Contributor Author

Ocramius commented Jun 2, 2021 via email

@Ocramius
Copy link
Contributor Author

After a lot of "putting this on the side", I decided to indeed go with complete removal of the Reflection*#setAccessible() behavior: the method still exists, but is effectively just dead code.

That implementation is now at #5412

Closing here - we can continue in #5412.

@Ocramius Ocramius closed this Jun 13, 2021
@Ocramius Ocramius deleted the feature/make-reflection-property-and-method-accessible-by-default branch June 13, 2021 15:50
Ocramius added a commit to Ocramius/psalm that referenced this pull request Dec 7, 2022
…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"
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants