Skip to content

Implement ReflectionProperty::is{Readable,Writable}() #16209

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

Draft
wants to merge 1 commit into
base: PHP-8.4
Choose a base branch
from
Draft

Implement ReflectionProperty::is{Readable,Writable}() #16209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 141 additions & 0 deletions ext/reflection/php_reflection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6617,6 +6617,147 @@ ZEND_METHOD(ReflectionProperty, isFinal)
_property_check_flag(INTERNAL_FUNCTION_PARAM_PASSTHRU, ZEND_ACC_FINAL);
}

static zend_result get_ce_from_scope_name(zend_class_entry **scope, zend_string *scope_name, zend_execute_data *execute_data)
{
if (!scope_name) {
*scope = NULL;
return SUCCESS;
}
if (zend_string_equals(scope_name, ZSTR_KNOWN(ZEND_STR_STATIC))) {
*scope = EX(prev_execute_data)->func->common.scope;
return SUCCESS;
}

*scope = zend_lookup_class(scope_name);
if (!*scope) {
zend_throw_error(NULL, "Class \"%s\" not found", ZSTR_VAL(scope_name));
return FAILURE;
}
return SUCCESS;
}

static zend_always_inline uint32_t set_visibility_to_visibility(uint32_t set_visibility)
{
switch (set_visibility) {
case ZEND_ACC_PUBLIC_SET:
return ZEND_ACC_PUBLIC;
case ZEND_ACC_PROTECTED_SET:
return ZEND_ACC_PROTECTED;
case ZEND_ACC_PRIVATE_SET:
return ZEND_ACC_PRIVATE;
EMPTY_SWITCH_DEFAULT_CASE();
}
}

static bool check_visibility(uint32_t visibility, zend_class_entry *ce, zend_class_entry *scope)
{
if (!(visibility & ZEND_ACC_PUBLIC) && (scope != ce)) {
if (!scope) {
return false;
}
if (visibility & ZEND_ACC_PRIVATE) {
return false;
}
ZEND_ASSERT(visibility & ZEND_ACC_PROTECTED);
if (!instanceof_function(scope, ce) && !instanceof_function(ce, scope)) {
return false;
}
}
return true;
}

ZEND_METHOD(ReflectionProperty, isReadable)
{
reflection_object *intern;
property_reference *ref;
zend_string *scope_name = ZSTR_KNOWN(ZEND_STR_STATIC);

ZEND_PARSE_PARAMETERS_START(0, 1)
Z_PARAM_OPTIONAL
Z_PARAM_STR_OR_NULL(scope_name)
ZEND_PARSE_PARAMETERS_END();

GET_REFLECTION_OBJECT_PTR(ref);
zend_property_info *prop = ref->prop;
if (!prop) {
_DO_THROW("May not use isReadable on dynamic properties");
RETURN_THROWS();
}

zend_class_entry *scope;
if (get_ce_from_scope_name(&scope, scope_name, execute_data) == FAILURE) {
RETURN_THROWS();
}

if (!check_visibility(prop->flags & ZEND_ACC_PPP_MASK, prop->ce, scope)) {
RETURN_FALSE;
}

if (prop->flags & ZEND_ACC_VIRTUAL) {
ZEND_ASSERT(prop->hooks);
if (!prop->hooks[ZEND_PROPERTY_HOOK_GET]) {
RETURN_FALSE;
}
}

RETURN_TRUE;
}

ZEND_METHOD(ReflectionProperty, isWritable)
{
reflection_object *intern;
property_reference *ref;
zend_object *obj;
zend_string *scope_name = ZSTR_KNOWN(ZEND_STR_STATIC);

ZEND_PARSE_PARAMETERS_START(1, 2)
Z_PARAM_OBJ(obj)
Z_PARAM_OPTIONAL
Z_PARAM_STR_OR_NULL(scope_name)
ZEND_PARSE_PARAMETERS_END();

GET_REFLECTION_OBJECT_PTR(ref);
zend_property_info *prop = ref->prop;
if (!prop) {
_DO_THROW("May not use isReadable on dynamic properties");
RETURN_THROWS();
}

zend_class_entry *scope;
if (get_ce_from_scope_name(&scope, scope_name, execute_data) == FAILURE) {
RETURN_THROWS();
}
if (!instanceof_function(obj->ce, prop->ce)) {
_DO_THROW("Given object is not an instance of the class this property was declared in");
RETURN_THROWS();
}

uint32_t set_visibility = prop->flags & ZEND_ACC_PPP_SET_MASK;
if (!set_visibility) {
set_visibility = zend_visibility_to_set_visibility(prop->flags & ZEND_ACC_PPP_MASK);
}
if (!check_visibility(set_visibility_to_visibility(set_visibility), prop->ce, scope)) {
RETURN_FALSE;
}

if (prop->flags & ZEND_ACC_VIRTUAL) {
ZEND_ASSERT(prop->hooks);
if (!prop->hooks[ZEND_PROPERTY_HOOK_SET]) {
RETURN_FALSE;
}
}

if (prop->flags & ZEND_ACC_READONLY) {
ZEND_ASSERT(prop->offset != ZEND_VIRTUAL_PROPERTY_OFFSET);
zval *prop_val = OBJ_PROP(obj, prop->offset);
if (Z_TYPE_P(prop_val) != IS_UNDEF && !(Z_PROP_FLAG_P(prop_val) & IS_PROP_REINITABLE)) {
RETURN_FALSE;
}
}

RETURN_TRUE;
}

/* {{{ Constructor. Throws an Exception in case the given extension does not exist */
ZEND_METHOD(ReflectionExtension, __construct)
{
Expand Down
4 changes: 4 additions & 0 deletions ext/reflection/php_reflection.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,10 @@ public function hasHook(PropertyHookType $type): bool {}
public function getHook(PropertyHookType $type): ?ReflectionMethod {}

public function isFinal(): bool {}

public function isReadable(?string $scope = 'static'): bool {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about this some more, it would likely be beneficial to also accept the object here and check it for IS_UNDEF.
Copy link
Contributor

Choose a reason for hiding this comment

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

What IS_UNDEF means in userland?
Copy link
Member Author

Choose a reason for hiding this comment

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

unset() properties or typed properties that have not been assigned to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Then yes, it would be great to have an object parameter here.

public function isWritable(object $object, ?string $scope = 'static'): bool {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that object should be a nullable optional second parameter, because a property might be static. Also non-readonly properties can be checked without an object.
Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.
}

/** @not-serializable */
Expand Down
15 changes: 14 additions & 1 deletion ext/reflection/php_reflection_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

84 changes: 84 additions & 0 deletions ext/reflection/tests/ReflectionProperty_isReadable.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
--TEST--
Test ReflectionProperty::isReadable()
--FILE--
<?php

class A {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have added some tests for traits. For example, a test ensuring that protected properties from a shared trait are not accessible (https://3v4l.org/hM5TM). Also a trait must not be accepted as a scope.

This comment was marked as duplicate.

Sign in to view


class B extends A {
public $a;
protected $b;
private $c;
public protected(set) int $d;
public $e { get => 42; }
public $f { set {} }
}

class C extends B {}

$test = static function ($scope) {
$rc = new ReflectionClass(B::class);
foreach ($rc->getProperties() as $rp) {
echo $rp->getName() . ' from ' . ($scope ?? 'global') . ': ';
var_dump($rp->isReadable($scope));
}
};

foreach (['A', 'B', 'C'] as $scope) {
$test($scope);
$test->bindTo(null, $scope)('static');
}

$test(null);
$test->bindTo(null, null)('static');

?>
--EXPECT--
a from A: bool(true)
b from A: bool(true)
c from A: bool(false)
d from A: bool(true)
e from A: bool(true)
f from A: bool(false)
a from static: bool(true)
b from static: bool(true)
c from static: bool(false)
d from static: bool(true)
e from static: bool(true)
f from static: bool(false)
a from B: bool(true)
b from B: bool(true)
c from B: bool(true)
d from B: bool(true)
e from B: bool(true)
f from B: bool(false)
a from static: bool(true)
b from static: bool(true)
c from static: bool(true)
d from static: bool(true)
e from static: bool(true)
f from static: bool(false)
a from C: bool(true)
b from C: bool(true)
c from C: bool(false)
d from C: bool(true)
e from C: bool(true)
f from C: bool(false)
a from static: bool(true)
b from static: bool(true)
c from static: bool(false)
d from static: bool(true)
e from static: bool(true)
f from static: bool(false)
a from global: bool(true)
b from global: bool(false)
c from global: bool(false)
d from global: bool(true)
e from global: bool(true)
f from global: bool(false)
a from static: bool(true)
b from static: bool(false)
c from static: bool(false)
d from static: bool(true)
e from static: bool(true)
f from static: bool(false)
90 changes: 90 additions & 0 deletions ext/reflection/tests/ReflectionProperty_isWritable.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
--TEST--
Test ReflectionProperty::isWritable()
--FILE--
<?php

class A {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this test does not check protected properties from parent/child classes?
Copy link
Member Author

Choose a reason for hiding this comment

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

Because the impl uses the same code for checking scope as read, so it's somewhat redundant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests should not assume any implementation details, because implementation might change later, but the tests should detect regression no matter how things are implemented.
public $a;
protected $b;
private $c;
public protected(set) int $d;
public $e { get => 42; }
public $f { set {} }
public readonly int $g;
public private(set) int $h;

public function setG($g) {
$this->g = $g;
}

public function __clone() {
$rp = new ReflectionProperty(A::class, 'g');
echo $rp->getName() . ' from static (initialized, clone): ';
var_dump($rp->isWritable($this));
}
}

$test = static function ($scope) {
$rc = new ReflectionClass(A::class);
foreach ($rc->getProperties() as $rp) {
echo $rp->getName() . ' from ' . ($scope ?? 'global') . ': ';
var_dump($rp->isWritable(new A(), $scope));

if ($rp->name == 'g') {
$a = new A();
$a->setG(42);
echo $rp->getName() . ' from ' . ($scope ?? 'global') . ' (initialized): ';
var_dump($rp->isWritable($a, $scope));
clone $a;
}
}
};

$test('A');
$test->bindTo(null, 'A')('static');

$test(null);
$test->bindTo(null, null)('static');

?>
--EXPECT--
a from A: bool(true)
b from A: bool(true)
c from A: bool(true)
d from A: bool(true)
e from A: bool(false)
f from A: bool(true)
g from A: bool(true)
g from A (initialized): bool(false)
g from static (initialized, clone): bool(true)
h from A: bool(true)
a from static: bool(true)
b from static: bool(true)
c from static: bool(true)
d from static: bool(true)
e from static: bool(false)
f from static: bool(true)
g from static: bool(true)
g from static (initialized): bool(false)
g from static (initialized, clone): bool(true)
h from static: bool(true)
a from global: bool(true)
b from global: bool(false)
c from global: bool(false)
d from global: bool(false)
e from global: bool(false)
f from global: bool(true)
g from global: bool(false)
g from global (initialized): bool(false)
g from static (initialized, clone): bool(true)
h from global: bool(false)
a from static: bool(true)
b from static: bool(false)
c from static: bool(false)
d from static: bool(false)
e from static: bool(false)
f from static: bool(true)
g from static: bool(false)
g from static (initialized): bool(false)
g from static (initialized, clone): bool(true)
h from static: bool(false)