Skip to content

ext/reflection: add getLine / getFileName / isUserDefined to reflection attribute #13889

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ext/reflection: add getLine / getFileName / isUserDefined to reflection attribute #13889

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
71 changes: 60 additions & 11 deletions ext/reflection/php_reflection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ typedef struct _attribute_reference {
zend_attribute *data;
zend_class_entry *scope;
zend_string *filename;
bool is_user_defined;
uint32_t target;
} attribute_reference;

Expand Down Expand Up @@ -1116,7 +1117,7 @@ static void _extension_string(smart_str *str, zend_module_entry *module, char *i

/* {{{ reflection_attribute_factory */
static void reflection_attribute_factory(zval *object, HashTable *attributes, zend_attribute *data,
zend_class_entry *scope, uint32_t target, zend_string *filename)
zend_class_entry *scope, uint32_t target, zend_string *filename, bool is_user_defined)
{
reflection_object *intern;
attribute_reference *reference;
Expand All @@ -1129,14 +1130,15 @@ static void reflection_attribute_factory(zval *object, HashTable *attributes, ze
reference->scope = scope;
reference->filename = filename ? zend_string_copy(filename) : NULL;
reference->target = target;
reference->is_user_defined = is_user_defined;
intern->ptr = reference;
intern->ref_type = REF_TYPE_ATTRIBUTE;
ZVAL_STR_COPY(reflection_prop_name(object), data->name);
}
/* }}} */

static int read_attributes(zval *ret, HashTable *attributes, zend_class_entry *scope,
uint32_t offset, uint32_t target, zend_string *name, zend_class_entry *base, zend_string *filename) /* {{{ */
uint32_t offset, uint32_t target, zend_string *name, zend_class_entry *base, zend_string *filename, bool is_user_defined) /* {{{ */
{
ZEND_ASSERT(attributes != NULL);

Expand All @@ -1149,7 +1151,7 @@ static int read_attributes(zval *ret, HashTable *attributes, zend_class_entry *s

ZEND_HASH_PACKED_FOREACH_PTR(attributes, attr) {
if (attr->offset == offset && zend_string_equals(attr->lcname, filter)) {
reflection_attribute_factory(&tmp, attributes, attr, scope, target, filename);
reflection_attribute_factory(&tmp, attributes, attr, scope, target, filename, is_user_defined);
add_next_index_zval(ret, &tmp);
}
} ZEND_HASH_FOREACH_END();
Expand Down Expand Up @@ -1181,7 +1183,7 @@ static int read_attributes(zval *ret, HashTable *attributes, zend_class_entry *s
}
}

reflection_attribute_factory(&tmp, attributes, attr, scope, target, filename);
reflection_attribute_factory(&tmp, attributes, attr, scope, target, filename, is_user_defined);
add_next_index_zval(ret, &tmp);
} ZEND_HASH_FOREACH_END();

Expand All @@ -1190,7 +1192,7 @@ static int read_attributes(zval *ret, HashTable *attributes, zend_class_entry *s
/* }}} */

static void reflect_attributes(INTERNAL_FUNCTION_PARAMETERS, HashTable *attributes,
uint32_t offset, zend_class_entry *scope, uint32_t target, zend_string *filename) /* {{{ */
uint32_t offset, zend_class_entry *scope, uint32_t target, zend_string *filename, bool is_user_defined) /* {{{ */
{
zend_string *name = NULL;
zend_long flags = 0;
Expand Down Expand Up @@ -1223,7 +1225,7 @@ static void reflect_attributes(INTERNAL_FUNCTION_PARAMETERS, HashTable *attribut

array_init(return_value);

if (FAILURE == read_attributes(return_value, attributes, scope, offset, target, name, base, filename)) {
if (FAILURE == read_attributes(return_value, attributes, scope, offset, target, name, base, filename, is_user_defined)) {
RETURN_THROWS();
}
}
Expand Down Expand Up @@ -1956,7 +1958,7 @@ ZEND_METHOD(ReflectionFunctionAbstract, getAttributes)

reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
fptr->common.attributes, 0, fptr->common.scope, target,
fptr->type == ZEND_USER_FUNCTION ? fptr->op_array.filename : NULL);
fptr->type == ZEND_USER_FUNCTION ? fptr->op_array.filename : NULL, ZEND_USER_CODE(fptr->type));
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'm a bit confused by the intention here. Does this check whether the attribute class is internal, or whether the target is internal? It currently does the latter, but that doesn't seem super useful?
Copy link
Author

@joelwurtz joelwurtz Apr 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The intention is more : does the attribute (the instance not the class) as been defined (written) in user land code (wether it's a method / parameter / class / etc ...),

If it's an attribute from user land on an user land code it should return true
If it's an attribute from php core on an user land code it should return true
If it's an attribute from php core on php core it should return false
If it's an attribute from user land code on php core it should return false (but i don't think this use case is possible)
Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify why this matters? I understand that internal attributes can have additional validation without instanciation. But why does it matter whether the attrbute is placed on a user or internal target?
Copy link
Author

@joelwurtz joelwurtz Apr 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is mainly if some library that do error handling (like https://github.com/symfony/error-handler by example) want to provide a generic way to pin point those error, they could then only do that when they detect that the attribute has been writed by the user

But i'm wondering if PHP internal class may have internal attributes attached to their method or class (now or in the future) defined ? I know there is pre defined attributes by PHP but not sure if there are used in internal methods or if an extension can declare them using C code ?

If not then this is useless
Copy link
Member

Choose a reason for hiding this comment

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

Yes, internal attributes can be attached to internal classes, but instantiating them should never error. If it does error, that would be a bug in core/the extension itself. I'm still confused about when and why exactly Symfony error handler would need this information.

Can you provide a concrete example? Moreover, whether the target is internal would already be known, by looking at the reflection of the class/method itself.
Copy link
Author

@joelwurtz joelwurtz Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As an example we could, in a generic error handler, do the following interface :

interface LinkReflectionAttribute
{
   public function getAttribute(): \ReflectionAttribute;
}

When the error handler catch an exception with this interface it may do special things like show the exception (like before) and add a special text (or HTML in web context) to the user to show which attributes this exception concern (it could even show the portion of code in web context).

Library would only have to implement this interface in their exception and the error handler would do the rest

Having the isUserDefined() method would allow this generic handler to be sure that the reflection attribute given has been written in a PHP file and does not originate from the core (in the last case there is no point showing something since it's not written in a PHP file and so it's not fixable by the user)

This method may not be necessary if the interface look like this :

interface LinkReflectionAttribute
{
   public function getAttribute(): \ReflectionAttribute;
   
   public function getTarget(): \ReflectionClass | \ReflectionMethod |\ReflectionFunction | \ReflectionParameter | \ReflectionProperty
}

In this case we could determine if it's user defined given the specific target.

The disadvantage, is that user implementing this interface would need to track every Attribute with their Target, and in some cases it may involved passing those 2 values a lot of times which is cumbersome.

But maybe the isUserDefined() method is not the best approach. Wouldn't it be better if instead we have this getTarget() method inside the ReflectionAttribute class ?

So we could have the target of the attribute without having to keep track of it in PHP code ?

There is already a getTarget() method that returns an int, there could be a getDeclaringTarget() that returns \ReflectionClass | \ReflectionMethod |\ReflectionFunction | \ReflectionParameter | \ReflectionProperty
}
/* }}} */

Expand Down Expand Up @@ -2837,7 +2839,7 @@ ZEND_METHOD(ReflectionParameter, getAttributes)

reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
attributes, param->offset + 1, scope, ZEND_ATTRIBUTE_TARGET_PARAMETER,
param->fptr->type == ZEND_USER_FUNCTION ? param->fptr->op_array.filename : NULL);
param->fptr->type == ZEND_USER_FUNCTION ? param->fptr->op_array.filename : NULL, ZEND_USER_CODE(param->fptr->type));
}

/* {{{ Returns whether this parameter is an optional parameter */
Expand Down Expand Up @@ -4021,7 +4023,7 @@ ZEND_METHOD(ReflectionClassConstant, getAttributes)

reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
ref->attributes, 0, ref->ce, ZEND_ATTRIBUTE_TARGET_CLASS_CONST,
ref->ce->type == ZEND_USER_CLASS ? ref->ce->info.user.filename : NULL);
ref->ce->type == ZEND_USER_CLASS ? ref->ce->info.user.filename : NULL, ref->ce->type == ZEND_USER_CLASS);
}
/* }}} */

Expand Down Expand Up @@ -4429,7 +4431,7 @@ ZEND_METHOD(ReflectionClass, getAttributes)

reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
ce->attributes, 0, ce, ZEND_ATTRIBUTE_TARGET_CLASS,
ce->type == ZEND_USER_CLASS ? ce->info.user.filename : NULL);
ce->type == ZEND_USER_CLASS ? ce->info.user.filename : NULL, ce->type == ZEND_USER_CLASS);
}
/* }}} */

Expand Down Expand Up @@ -5866,7 +5868,7 @@ ZEND_METHOD(ReflectionProperty, getAttributes)

reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
ref->prop->attributes, 0, ref->prop->ce, ZEND_ATTRIBUTE_TARGET_PROPERTY,
ref->prop->ce->type == ZEND_USER_CLASS ? ref->prop->ce->info.user.filename : NULL);
ref->prop->ce->type == ZEND_USER_CLASS ? ref->prop->ce->info.user.filename : NULL, ref->prop->ce->type == ZEND_USER_CLASS);
}
/* }}} */

Expand Down Expand Up @@ -6681,6 +6683,53 @@ ZEND_METHOD(ReflectionAttribute, getArguments)
}
/* }}} */

/* {{{ Returns true if the attribute is defined by user land code, false otherwise */
ZEND_METHOD(ReflectionAttribute, isUserDefined)
{
reflection_object *intern;
attribute_reference *attr;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}
GET_REFLECTION_OBJECT_PTR(attr);

RETURN_BOOL(attr->is_user_defined);
}
/* }}} */

/* {{{ Returns the filename (if it exists : user land code) at which the attribute was defined */
ZEND_METHOD(ReflectionAttribute, getFileName)
{
reflection_object *intern;
attribute_reference *attr;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}
GET_REFLECTION_OBJECT_PTR(attr);
if (attr->filename != NULL) {
RETURN_STR_COPY(attr->filename);
}
RETURN_FALSE;
}
/* }}} */

/* {{{ Returns the line at which the attribute was defined */
ZEND_METHOD(ReflectionAttribute, getLine)
{
reflection_object *intern;
attribute_reference *attr;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}
GET_REFLECTION_OBJECT_PTR(attr);

RETURN_LONG(attr->data->lineno);
}
/* }}} */

static int call_attribute_constructor(
zend_attribute *attr, zend_class_entry *ce, zend_object *obj,
zval *args, uint32_t argc, HashTable *named_params, zend_string *filename)
Expand Down
3 changes: 3 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 @@ -771,6 +771,9 @@ public function getTarget(): int {}
public function isRepeated(): bool {}
public function getArguments(): array {}
public function newInstance(): object {}
public function isUserDefined(): bool {}
public function getFileName(): string|false {}
public function getLine(): int {}

public function __toString(): string {}

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.

24 changes: 24 additions & 0 deletions ext/reflection/tests/ReflectionAttribute_getFileName.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
ReflectionAttribute getFileName
--FILE--
<?php
#[Attribute]
class A {}

class Foo {
#[A]
public function bar() {}
}

$rm = new ReflectionMethod(Foo::class, "bar");
$attribute = $rm->getAttributes()[0];

if ($attribute->getFileName() === __FILE__) {
echo "OK";
} else {
echo "FAIL";
}

?>
--EXPECT--
OK
22 changes: 22 additions & 0 deletions ext/reflection/tests/ReflectionAttribute_getLine.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
ReflectionAttribute getLines
--FILE--
<?php
#[Attribute]
class A {}

class Foo {
#[A]
public function bar() {}
}

$rm = new ReflectionMethod(Foo::class, "bar");
$attribute = $rm->getAttributes()[0];

var_dump($rm->getStartLine());
var_dump($attribute->getLine());

?>
--EXPECT--
int(7)
int(6)
20 changes: 20 additions & 0 deletions ext/reflection/tests/ReflectionAttribute_isUserDefined.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
--TEST--
ReflectionAttribute isUserDefined
--FILE--
<?php
#[Attribute]
class A {}

class Foo {
#[A]
public function bar() {}
}

$rm = new ReflectionMethod(Foo::class, "bar");
$attribute = $rm->getAttributes()[0];

var_dump($attribute->isUserDefined());

?>
--EXPECT--
bool(true)