Adding hasParameter() and getParameter() methods to ReflectionFunctionAbstract

[ollieread]

Description

We currently have getMethod() and getProperty() methods to retrieve a single method or properly by its name, but we do not have the same for parameters, even though we now have named properties.

I am happy to PR this, and I think I may have figured out a way to achieve it, but right now, it involves looping through the parameters until one that matches is found, which feels less than ideal.

Here's ReflectionFunctionAbstract::hasParameter()

/* {{{ Returns whether a parameter exists or not */
ZEND_METHOD(ReflectionFunctionAbstract, hasParameter)
{
    reflection_object *intern;
    zend_function *fptr;
    struct _zend_arg_info *arg_info;
    zend_string *name, *lc_name;

    if (zend_parse_parameters(ZEND_NUM_ARGS(), "S", &name) == FAILURE) {
        RETURN_THROWS();
    }

    if (zend_parse_parameters_none() == FAILURE) {
        RETURN_FALSE;
    }

    GET_REFLECTION_OBJECT_PTR(fptr);

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

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

    if (!num_args) {
        RETURN_FALSE;
    }

    lc_name = zend_string_tolower(name);

    for (i = 0; i < num_args; i++) {
        zval parameter;

        reflection_parameter_factory(
            _copy_function(fptr),
            Z_ISUNDEF(intern->obj) ? NULL : &intern->obj,
            arg_info,
            i,
            i < fptr->common.required_num_args,
            &parameter
        );

        if (arg_info.name == lc_name) {
            RETURN_TRUE;
        }

        arg_info++;
    }

    zend_string_release(lc_name);

    RETURN_FALSE;
}
/* }}} */

I think, if I can figure out (or pointed towards) how to return parameter, I could use the same code for ReflectionFunctionAbstract::getParameter().

Would appreciate any feedback on the proposal and also pointers for the code.

[iluuu1994]

• zend_parse_parameters_none() == FAILURE is contradicting the parse just above it.
• The string comparison and construction of the parameter reflection should be switched to avoid useless construction here. In fact, for the hasParameter variant reflection_parameter_factory is completely unnecessary.
• Variables are case-sensitive so the tolower is incorrect. Furthermore, a == is insufficient, you need a string comparison function like zend_string_equals.

With those this doesn't seem like a bad solution. Feel free to create a PR. I don't think this needs an RFC, unless somebody else feels different.

[ollieread]

• zend_parse_parameters_none() == FAILURE is contradicting the parse just above it.

  • The string comparison and construction of the parameter reflection should be switched to avoid useless construction here. In fact, for the hasParameter variant reflection_parameter_factory is completely unnecessary.

  • Variables are case-sensitive so the tolower is incorrect. Furthermore, a == is insufficient, you need a string comparison function like zend_string_equals.

With those this doesn't seem like a bad solution. Feel free to create a PR. I don't think this needs an RFC, unless somebody else feels different.

Thanks @iluuu1994, I've updated the methods to be like so:

This is hasParameter

/* {{{ Returns whether a parameter exists or not */
ZEND_METHOD(ReflectionFunctionAbstract, hasParameter)
{
    reflection_object *intern;
    zend_function *fptr;
    struct _zend_arg_info *arg_info;
    zend_string *name;
    uint32_t i, num_args;

    if (zend_parse_parameters(ZEND_NUM_ARGS(), "S", &name) == FAILURE) {
        RETURN_THROWS();
    }

    GET_REFLECTION_OBJECT_PTR(fptr);

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

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

    if (!num_args) {
        RETURN_FALSE;
    }

    for (i = 0; i < num_args; i++) {
        if (zend_string_equals(arg_info[0].name, name)) {
            RETURN_TRUE;
        }

        arg_info++;
    }

    RETURN_FALSE;
}
/* }}} */

This is getParameter.

/* {{{ Returns a parameter specified by its name */
ZEND_METHOD(ReflectionFunctionAbstract, getParameter)
{
    reflection_object *intern;
    zend_function *fptr;
    struct _zend_arg_info *arg_info;
    zend_string *name;
    uint32_t i, num_args;

    if (zend_parse_parameters(ZEND_NUM_ARGS(), "S", &name) == FAILURE) {
        RETURN_THROWS();
    }

    GET_REFLECTION_OBJECT_PTR(fptr);

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

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

    if (!num_args) {
        RETURN_FALSE;
    }

    for (i = 0; i < num_args; i++) {
        zval parameter;

        reflection_parameter_factory(
            _copy_function(fptr),
            Z_ISUNDEF(intern->obj) ? NULL : &intern->obj,
            arg_info,
            i,
            i < fptr->common.required_num_args,
            &parameter
        );

        if (zend_string_equals(arg_info[0].name, name)) {
            RETURN_ZVAL(parameter);
        }

        arg_info++;
    }

    RETURN_NULL();
}
/* }}} */

The issue I'm having is that running php build/gen_stubs.php does nothing, and if I manually edit the file myself, I get the following errors:

/Users/ollieread/Code/ollieread/php-src/ext/reflection/php_reflection.c:2205:34: error: too few arguments provided to function-like macro invocation
            RETURN_ZVAL(parameter);
                                 ^
/Users/ollieread/Code/ollieread/php-src/Zend/zend_API.h:1041:9: note: macro 'RETURN_ZVAL' defined here
#define RETURN_ZVAL(zv, copy, dtor)             do { RETVAL_ZVAL(zv, copy, dtor); return; } while (0)
        ^
/Users/ollieread/Code/ollieread/php-src/ext/reflection/php_reflection.c:2205:13: error: use of undeclared identifier 'RETURN_ZVAL'
            RETURN_ZVAL(parameter);

I'm unsure of the best way to return the ReflectionParameter and I can't find anything that points me towards the right thing.

[kocsismate]

A few suggestions:

• The parameter type of these function should be int|string so that either positional and named arguments are supported
• IMO ReflectionFunctionAbstract::getParameter() should throw an exception if the parameter doesn't exist, including the case when there is no parameter (if (!num_args)).
• reflection_parameter_factory should be called once and only once for the parameter to be returned
• Instead of arg_info[0].name, you should check for arg_info[i].name
• Instead of RETURN_ZVAL, use RETURN_OBJ

[ollieread]

Thanks, will look into it.

• reflection_parameter_factory should be called once and only once for the parameter to be returned

I don't get what this means, would you be able to elaborate? As far as I can tell, it is called once per parameter, the same way it is for ReflectionFunctionAbstract::getParameters().

[iluuu1994]

I don't get what this means, would you be able to elaborate? As far as I can tell, it is called once per parameter, the same way it is for ReflectionFunctionAbstract::getParameters().

reflection_parameter_factory instantiates the reflection object. For hasParameter this is completely unnecessary as it's never returned, and leaks. For getParameter you only need it once, namely when you've found the right parameter.

[ollieread]

I don't get what this means, would you be able to elaborate? As far as I can tell, it is called once per parameter, the same way it is for ReflectionFunctionAbstract::getParameters().

reflection_parameter_factory instantiates the reflection object. For hasParameter this is completely unnecessary as it's never returned, and leaks. For getParameter you only need it once, namely when you've found the right parameter.

Ah okay, thanks @iluuu1994.

Will look into the best way to handle positions as well as names. I know there's arg_info.offset, but the best way to decipher whether this is a name or position call.

[kocsismate]

but the best way to decipher whether this is a name or position call.

If the argument is a number/numeric string then you can just return the n+1th parameter (of course if it is less than or equal to num_args. Otherwise you can search for the parameter name just like now.

[ollieread]

I think I've nailed it now, here is hasParameter.

ZEND_METHOD(ReflectionFunctionAbstract, hasParameter)
{
    reflection_object *intern;
    zend_function *fptr;
    struct _zend_arg_info *arg_info;
    zval *parameter;
    uint32_t i, num_args, position;

    if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &parameter) == FAILURE) {
        RETURN_THROWS();
    }

    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();
    }

    switch(Z_TYPE_P(parameter)) {
        case IS_LONG:
            position = Z_LVAL_P(parameter);

            if (position < 0 || position > num_args) {
                RETURN_FALSE;
            }

            RETURN_TRUE;
            break;
        case IS_STRING:
            for (i = 0; i < num_args; i++) {
                if (zend_string_equals(arg_info->name, Z_STR_P(parameter))) {
                    RETURN_TRUE;
                }

                arg_info++;
            }

            RETURN_FALSE;
            break;
        default:
            zend_argument_type_error(1, "must be of type %s, %s given", "int or string", zend_zval_type_name(parameter));
        break;
    }
}

For some reason, build/gen_stubs.php generated this method as

hasParameter(string $name): bool;

Instead of

hasParameter(string|int $parameter): bool;

So I deleted the generated files, and now no matter how many times I run make clean and make build, the tests come up:

Fatal error: Uncaught Error: Call to undefined method ReflectionFunction::hasParameter() in Standard input code:8

[kocsismate]

Please create a proper PR, it's very cumbersome to review code in comments + tests are not run this way

[iluuu1994]

PR is here: #10431 Closing this in the meantime, as this issue no longer servers a purpose.

[ollieread]

PR is here: #10431 Closing this in the meantime, as this issue no longer servers a purpose.

Yes, sorry about the delay. Will continue with the PR asap, had a lot of stuff happening in my personal life lately.