Skip to content
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

Add ReflectionFunctionAbstract::getParameter() and hasParameter() #10431

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
87f8884
Add ReflectionFunctionAbstract::getParameter() and hasParameter()
ollieread Jan 24, 2023
44c8db9
Refactor based on suggestions to avoid custom code
ollieread Jul 25, 2023
618cd84
Add missing exceptions from hasParameter tests
ollieread Jul 25, 2023
c921efd
Fix bugs in ReflectionFunctionAbstract::hasParameter() and getParamet…
ollieread Jul 25, 2023
88392c0
Update equals check for parameter name
ollieread Jul 25, 2023
479bec6
Removed unused variable
ollieread Jul 25, 2023
4d9cead
Remove incrementing of arg_info
ollieread Jul 25, 2023
ba2a03d
Change parameter name handling to account for internal arg_info
ollieread Jul 25, 2023
eb9c500
Generate stubs with new methods
ollieread Jul 25, 2023
cc3c7a4
Fix test as sort method now has 2 parameters
ollieread Jul 25, 2023
4ace6d0
Add tests for all errors on ReflectionFunction::getParameter
ollieread Jul 25, 2023
279a8da
Add test for all cases of ReflectionMethod::getParameter
ollieread Jul 25, 2023
2b975b0
Change to value error for position -1 and update tests
ollieread Aug 4, 2023
0e0438d
Merge branch 'master' into reflectionfunction-getparameter
ollieread Jan 22, 2024
60b962c
Add static function for retrieving function parameter position by name
ollieread Jan 22, 2024
9a8f3a0
Refactor getParameter and hasParameter to use new static function
ollieread Jan 22, 2024
78b3d8d
Ensure that a ValueError is thrown within hasParameter
ollieread Jan 22, 2024
3162d1d
Fix bugs introduced in last few commits
ollieread Jan 22, 2024
29fb4ad
Updated header file
ollieread Jan 22, 2024
54a666b
Moved common.arg_info variable to get_parameter_position
ollieread Jan 22, 2024
bafb1ab
Change int to uint32_t for get_parameter_position() function
ollieread Jan 23, 2024
2984667
Throw an error if an empty string is passed to getParamater() or hasP…
ollieread Jan 23, 2024
99ea19c
Fix issue with empty string check and fix tests
ollieread Jan 23, 2024
28bbb66
Revert "Fix issue with empty string check and fix tests"
ollieread Jan 23, 2024
e9309a1
Refixed issue with checking for empty strings
ollieread Jan 23, 2024
23aac25
Actually throw an exception when attempting to retrieve parameters fo…
ollieread Jan 23, 2024
47bbe4b
Made error message when there are no parameters more specific
ollieread Jan 25, 2024
ddb9e30
Add macro for throwing formatted exception messages
ollieread Jan 25, 2024
01aa789
Change parameter name to param to be consistent
ollieread Feb 1, 2024
0a2f4e3
Changed error messages to be more helpful
ollieread Feb 1, 2024
627c9f8
Change exception message for empty strings
ollieread Feb 1, 2024
da0a602
Use correct format for zend_long type
ollieread Feb 1, 2024
8d389b9
Merge branch 'master' into reflectionfunction-getparameter
ollieread Feb 2, 2024
ceda3c1
Merge branch 'master' into reflectionfunction-getparameter
ollieread Feb 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 138 additions & 1 deletion ext/reflection/php_reflection.c
Original file line number Diff line number Diff line change
Expand Up @@ -1519,6 +1519,28 @@ static int get_parameter_default(zval *result, parameter_reference *param) {
}
}

static zend_long get_parameter_position(zend_function *func, zend_string* arg_name, uint32_t num_args) {
struct _zend_arg_info *arg_info = func->common.arg_info;
uint32_t i;
bool internal = has_internal_arg_info(func);

for (i = 0; i < num_args; i++) {
if (arg_info[i].name) {
if (internal) {
if (strcmp(((zend_internal_arg_info*)arg_info)[i].name, ZSTR_VAL(arg_name)) == 0) {
return i;
}
} else {
if (zend_string_equals(arg_name, arg_info[i].name)) {
return i;
}
}
}
}

return -1;
}

/* {{{ Preventing __clone from being called */
ZEND_METHOD(ReflectionClass, __clone)
{
Expand Down Expand Up @@ -2105,7 +2127,122 @@ ZEND_METHOD(ReflectionFunctionAbstract, getNumberOfRequiredParameters)
}
/* }}} */

/* {{{ Returns an array of parameter objects for this function */
/* {{{ Returns whether a parameter exists or not */
ZEND_METHOD(ReflectionFunctionAbstract, hasParameter)
{
reflection_object *intern;
zend_function *fptr;
zend_string *arg_name = NULL;
zend_long position;
uint32_t num_args;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR_OR_LONG(arg_name, position)
ZEND_PARSE_PARAMETERS_END();

GET_REFLECTION_OBJECT_PTR(fptr);

num_args = fptr->common.num_args;

if (fptr->common.fn_flags & ZEND_ACC_VARIADIC) {
num_args++;
}

if (!num_args) {
RETURN_FALSE;
}

if (arg_name != NULL) {
if (ZSTR_LEN(arg_name) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

the null byte check should also be done here as well

zend_argument_value_error(1, "must not be an empty string");
RETURN_THROWS();
}

if (get_parameter_position(fptr, arg_name, num_args) > -1) {
RETURN_TRUE;
}

RETURN_FALSE;
} else {
position -= 1;

if (position < 0) {
zend_argument_value_error(1, "must be greater than or equal to 1");
RETURN_THROWS();
}

RETURN_BOOL(position < num_args);
}
}
/* }}} */

/* {{{ Returns a parameter specified by its name */
ZEND_METHOD(ReflectionFunctionAbstract, getParameter)
{
reflection_object *intern;
zend_function *fptr;
zend_string *arg_name = NULL;
zend_long position;
struct _zend_arg_info *arg_info;
uint32_t num_args;
zval reflection;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR_OR_LONG(arg_name, position)
ZEND_PARSE_PARAMETERS_END();

GET_REFLECTION_OBJECT_PTR(fptr);

num_args = fptr->common.num_args;
arg_info = fptr->common.arg_info;

if (fptr->common.fn_flags & ZEND_ACC_VARIADIC) {
num_args++;
}

if (!num_args) {
RETURN_THROWS();
}
ollieread marked this conversation as resolved.
Show resolved Hide resolved

if (arg_name != NULL) {
if (ZSTR_LEN(arg_name) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Besides an empty string, additional null bytes should also checked the following way:

if (CHECK_NULL_PATH(source, source_len)) {
			zend_argument_value_error(1, "must not contain any null bytes");
			return NULL;
		}

(code is copy pasted from ext/dom, but there are plenty of other examples as well)

zend_argument_value_error(1, "must not be an empty string");
ollieread marked this conversation as resolved.
Show resolved Hide resolved
RETURN_THROWS();
}

position = get_parameter_position(fptr, arg_name, num_args);

if (position == -1) {
_DO_THROW("The parameter specified by its name could not be found");
ollieread marked this conversation as resolved.
Show resolved Hide resolved
RETURN_THROWS();
}
} else {
position -= 1;

if (position < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Wait, do these functions use zero index for parameters? I think it's not a good idea, since error messages use a 1-index (Argument #1 ...)

Copy link
Member

Choose a reason for hiding this comment

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

I know that getParameters() returns a zero-index array, so it's not exactly clear which indexing is the most expected for people. For me, it's the 1-based one. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we had this discussion earlier. It might make sense to send an email to internals, but I also think using 1-indexed here is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if we change that here @kocsismate we would probably want to change the return value of getPosition(), which I would imagine is going to break a lot of stuff.

Copy link
Member

Choose a reason for hiding this comment

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

My opinion is that we should use zero index to be consistent with getPosition and the parameter array.
I read "Argument #1 ..." in my head as "argument number 1", not "argument index 1".
Anyway, this needs discussion on the ML as said before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My opinion is that we should use zero index to be consistent with getPosition and the parameter array. I read "Argument #1 ..." in my head as "argument number 1", not "argument index 1". Anyway, this needs discussion on the ML as said before.

I agree, as we're used to dealing with 0-index arrays.

Shall I just continue as is for now, and then send something to the ML? What specifically should I be mentioning? This PR and the whole discussion over index vs position?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a discussion on the ML before anything is done here.

And the discussion should be, here are two new helper methods which take either the name of a parameter or its position. However there is contention between should this be 1 indexed or 0 indexed due to the existing API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer a discussion on the ML before anything is done here.

And the discussion should be, here are two new helper methods which take either the name of a parameter or its position. However there is contention between should this be 1 indexed or 0 indexed due to the existing API.

Have started one on there @Girgias, sorry about the delay.

https://externals.io/message/123251

zend_argument_value_error(1, "must be greater than or equal to 1");
RETURN_THROWS();
}
if (position >= num_args) {
_DO_THROW("The parameter specified by its offset could not be found");
RETURN_THROWS();
}
}

reflection_parameter_factory(
_copy_function(fptr),
Z_ISUNDEF(intern->obj) ? NULL : &intern->obj,
&arg_info[position],
position,
position < fptr->common.required_num_args,
&reflection
);

RETURN_OBJ(Z_OBJ(reflection));
}
/* }}} */

/* {{{ Returns the function/method specified by its name */
ZEND_METHOD(ReflectionFunctionAbstract, getParameters)
{
reflection_object *intern;
Expand Down
4 changes: 4 additions & 0 deletions ext/reflection/php_reflection.stub.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ public function hasTentativeReturnType(): bool {}
public function getTentativeReturnType(): ?ReflectionType {}

public function getAttributes(?string $name = null, int $flags = 0): array {}

public function hasParameter(int|string $parameter): bool;
ollieread marked this conversation as resolved.
Show resolved Hide resolved

public function getParameter(int|string $parameter): ReflectionParameter;
}

class ReflectionFunction extends ReflectionFunctionAbstract
Expand Down
14 changes: 13 additions & 1 deletion ext/reflection/php_reflection_arginfo.h

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

41 changes: 41 additions & 0 deletions ext/reflection/tests/ReflectionFunction_getParameter_basic.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
--TEST--
ReflectionFunction::getParameter()
--FILE--
<?php
function foo(string $bar) {}

$function = new ReflectionFunction('sort');
var_dump($function->getParameter('array'));
var_dump($function->getParameter('flags'));
var_dump($function->getParameter(1));
var_dump($function->getParameter(2));

$function = new ReflectionFunction('foo');
var_dump($function->getParameter('bar'));
var_dump($function->getParameter(1));
?>
--EXPECT--
object(ReflectionParameter)#2 (1) {
["name"]=>
string(5) "array"
}
object(ReflectionParameter)#2 (1) {
["name"]=>
string(5) "flags"
}
object(ReflectionParameter)#2 (1) {
["name"]=>
string(5) "array"
}
object(ReflectionParameter)#2 (1) {
["name"]=>
string(5) "flags"
}
object(ReflectionParameter)#1 (1) {
["name"]=>
string(3) "bar"
}
object(ReflectionParameter)#1 (1) {
["name"]=>
string(3) "bar"
}
53 changes: 53 additions & 0 deletions ext/reflection/tests/ReflectionFunction_getParameter_error.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
--TEST--
ReflectionFunction::getParameter() errors
--FILE--
<?php
$function = new ReflectionFunction('sort');
function foo(string $bar) {}

try {
var_dump($function->getParameter('Array'));
} catch(ReflectionException $e) {
print($e->getMessage() . PHP_EOL);
}

try {
var_dump($function->getParameter(-1));
} catch(ValueError $e) {
print($e->getMessage() . PHP_EOL);
}

try {
var_dump($function->getParameter(3));
} catch(ReflectionException $e) {
print($e->getMessage() . PHP_EOL);
}

$function = new ReflectionFunction('foo');

try {
var_dump($function->getParameter('Bar'));
} catch(ReflectionException $e) {
print($e->getMessage() . PHP_EOL);
}

try {
var_dump($function->getParameter(-1));
} catch(ValueError $e) {
print($e->getMessage() . PHP_EOL);
}

try {
var_dump($function->getParameter(2));
} catch(ReflectionException $e) {
print($e->getMessage() . PHP_EOL);
}

?>
--EXPECT--
The parameter specified by its name could not be found
ReflectionFunctionAbstract::getParameter(): Argument #1 ($parameter) must be greater than or equal to 1
The parameter specified by its offset could not be found
The parameter specified by its name could not be found
ReflectionFunctionAbstract::getParameter(): Argument #1 ($parameter) must be greater than or equal to 1
The parameter specified by its offset could not be found
31 changes: 31 additions & 0 deletions ext/reflection/tests/ReflectionFunction_hasParameter_basic.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
--TEST--
ReflectionFunction::hasParameter()
--FILE--
<?php
function foo(string $bar) {}

$function = new ReflectionFunction('sort');
var_dump($function->hasParameter('array'));
var_dump($function->hasParameter('Array'));
var_dump($function->hasParameter('string'));
var_dump($function->hasParameter(1));
var_dump($function->hasParameter(2));

$function = new ReflectionFunction('foo');
var_dump($function->hasParameter('bar'));
var_dump($function->hasParameter('Bar'));
var_dump($function->hasParameter('string'));
var_dump($function->hasParameter(1));
var_dump($function->hasParameter(2));
?>
--EXPECT--
bool(true)
bool(false)
bool(false)
bool(true)
bool(true)
bool(true)
bool(false)
bool(false)
bool(true)
bool(false)
25 changes: 25 additions & 0 deletions ext/reflection/tests/ReflectionFunction_hasParameter_error.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
--TEST--
ReflectionFunction::hasParameter() errors
--FILE--
<?php
function foo(string $bar) {}

$function = new ReflectionFunction('sort');

try {
var_dump($function->hasParameter(-1));
} catch (ValueError $e) {
print($e->getMessage() . PHP_EOL);
}

$function = new ReflectionFunction('foo');

try {
var_dump($function->hasParameter(-1));
} catch (ValueError $e) {
print($e->getMessage() . PHP_EOL);
}
?>
--EXPECT--
ReflectionFunctionAbstract::hasParameter(): Argument #1 ($parameter) must be greater than or equal to 1
ReflectionFunctionAbstract::hasParameter(): Argument #1 ($parameter) must be greater than or equal to 1
33 changes: 33 additions & 0 deletions ext/reflection/tests/ReflectionMethod_getParameter_basic.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
--TEST--
ReflectionMethod::getParameter()
--FILE--
<?php
class C {
public function foo(string $bar) {}
}

$method = new ReflectionMethod('WeakReference', 'create');
var_dump($method->getParameter('object'));
var_dump($method->getParameter(1));

$method = new ReflectionMethod('C', 'foo');
var_dump($method->getParameter('bar'));
var_dump($method->getParameter(1));
?>
--EXPECT--
object(ReflectionParameter)#2 (1) {
["name"]=>
string(6) "object"
}
object(ReflectionParameter)#2 (1) {
["name"]=>
string(6) "object"
}
object(ReflectionParameter)#1 (1) {
["name"]=>
string(3) "bar"
}
object(ReflectionParameter)#1 (1) {
["name"]=>
string(3) "bar"
}
Loading