-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
base: master
Are you sure you want to change the base?
Conversation
The reason you can't call your newly added functions from php scripts yet is becausr you need to update the stubs. In particular you need to add your method declarations to php_reflection.stub.php iirc, and then run build/gen_stub.php or run make. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this feature, I think it generally makes sense, just some comments on some details :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the Z_PARAM_STR_OR_LONG()
ZPP function. For examples of usage look at ZEND_METHOD(ReflectionParameter, __construct)
in the same file.
Sorry all, a lot has been going on. What's the best way to pick this back up? I know we are approaching an 8.3 release, so I should target that, or is 8.2 still fine? |
Should be rebased on master for 8.3 release |
I've used |
I'm unable to run
|
You need to do a complete recompile, which entails |
Excellent, thank you. Sorry, I'll try and write them down this time 😅 |
You also have to add your new functions to the stub file php_reflection.stub.php, such that PHP can find it and is aware of how your function signature looks. |
Will add tests for ReflectionMethod, just running some errands first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very quick review with some immediate comments
ext/reflection/tests/ReflectionFunction_getParameter_basic.phpt
Outdated
Show resolved
Hide resolved
ext/reflection/tests/ReflectionFunction_getParameter_error1.phpt
Outdated
Show resolved
Hide resolved
I'm so sorry about leaving this, had eviction, moving and family issues. Should hopefully pick this back up either today after my kids' nativity play, or tomorrow. Thank you for your patience and not closing the PR. |
No worries, life happens :) |
This reverts commit 99ea19c.
…r a function/method that doesn't exist
Both of the debug jobs are failing on the tests I just updated, and the release one is failing because of a separate test (not one of mine). I've actually had a similar problem with another PR #13224, where jobs are failing on the debug builds, I've no idea why that would happen. |
You are leaking memory I would recommend that you build your debug build with UBSAN and ASAN as this will catch a lot of errors :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me other than the position thing.
All updated, but I am having issues with the format for the exception, as |
You can use the |
Thank you! I did see that and wonder, but I'm still very new to C itself, let alone this codebase! |
All done and working now, the MacOS failed test is unrelated. |
# Conflicts: # ext/reflection/php_reflection_arginfo.h
The failures after the merge are because the arginfo file is not up-to-date. Please regenerate it. |
} | ||
|
||
if (arg_name != NULL) { | ||
if (ZSTR_LEN(arg_name) == 0) { |
There was a problem hiding this comment.
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)
RETURN_THROWS(); | ||
} | ||
} else { | ||
if (position < 0) { |
There was a problem hiding this comment.
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 ...
)
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
} | ||
|
||
if (arg_name != NULL) { | ||
if (ZSTR_LEN(arg_name) == 0) { |
There was a problem hiding this comment.
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
This PR is a work in progress, but it adds
hasParameter()
andgetParameter()
toReflectionFunctionAbstract
.Both methods accept
int|string $parameter
to retrieve/check parameters by position or name.hasParameter
returnstrue
orfalse
, whereasgetParameter
returnsReflectionParameter
or throws an exception.Addresses #10341