Fix #79969: Wrong error message in strict mode in ReflectionMethod::_construct

[jdomenechb]

The ReflectionMethod constructor can be called by using 1 or 2 parameters. The parameter parsing was checking first if the two parameters were correct quietly and, in case they weren't, it would try again but considering one parameter, this time throwing errors if it was not correct. As checking for the parameter type was performed in the first check (with two parameters), the type error was muted and replaced instead with the errors for the check with one parameter.

[cmb69]

Thanks for the PR! It seems to me that the error message is still misleading if more than 2 or zero arguments are passed. If we go with the argnum dispatch, I'd add a check for that, and do ZEND_WRONG_PARAM_COUNT() in that case. Changing the if-else-if to a switch statement might be beneficial as well.

[jdomenechb]

Thank you for your feedback @cmb69 ! I have already applied your feedback.

[Girgias]

The too few/many argument should still throw IMHO as it used the ZPP Throw mode which threw an ArgumentCountError. Moreover due to the Consistent TypeError RFC (https://wiki.php.net/rfc/consistent_type_errors) while merging this into master the throw behaviour would be back and the test case would need to be edited again.

[jdomenechb]

I understand @Girgias . Given the fact I am a new contributor and my knowledge in php-src code is very limited, could you give me some guidelines in how to proceed and some reference code examples if possible so I can improve the fix further given your feedback?

[Girgias]

Suggested change

	ZEND_WRONG_PARAM_COUNT();
	zend_wrong_parameters_count_exception(1, 2);

Should do the trick, for PHP 7.3 and 7.4, the person doing the merge-up to master/PHP 8.0 will need to change it to zend_wrong_parameters_count_error() as the former API has been removed in master.

[Girgias]

Nit: missing new line before EOF

[kocsismate]

@jdomenechb First, I'd also like to thank you your contribution!

On the other hand however, I think the most correct solution would be to make the second parameter nullable and optional (by using z|s!), and then:

• throw an exception if the first argument is neither a string, nor an object
• throw an exception if the first argument is a string and the second argument is not-null
• throw an exception if the first argument is an object and the second argument is null

By doing the above, all the possible invalid state can be eliminated, and then the rest of the logic of ReflectionMethod::construct() can be implemented.

The same should be fixed for PHP 8 as well, but there we can do even better, since instead of using the z old ZPP mode, we can use the Z_PARAM_STR_OR_OBJ() union type check via fast ZPP. It will also allow us to add the object|string type to the first parameter in php_reflection.stub.php.

To be honest, I don't think this fix is worth the hassle for PHP 7.x, but it would be quite nice for PHP 8. I made a few cleanups around this method recent, but unfortunately, I missed the possibility to clean up the parameter parsing. :)

[jdomenechb]

Thank you @kocsismate , very happy to collaborate :)

I'm sorry, but I tried to do as you said, and the following condition should not be as you mentioned:

* throw an exception if the first argument is a string and the second argument is not-null

This is because the first argument can also be a class name (string), as the docs mention.

So, the possible ways to call the constructor are:

public ReflectionMethod::__construct ( object $class , string $name )

public ReflectionMethod::__construct ( string $class , string $name )

public ReflectionMethod::__construct ( string $class_method )

Before messing more the code, would you mind to confirm if you still find the solution you gave me feasible despite not implementing the mentioned check, as other possibilities should already be covered by z|s!? If not, I could always follow @Girgias suggestion.

[Girgias]

Thank you @kocsismate , very happy to collaborate :)

I'm sorry, but I tried to do as you said, and the following condition should not be as you mentioned:

* throw an exception if the first argument is a string and the second argument is not-null

This is because the first argument can also be a class name (string), as the docs mention.

So, the possible ways to call the constructor are:

public ReflectionMethod::__construct ( object $class , string $name )

public ReflectionMethod::__construct ( string $class , string $name )

public ReflectionMethod::__construct ( string $class_method )

Before messing more the code, would you mind to confirm if you still find the solution you gave me feasible despite not implementing the mentioned check, as other possibilities should already be covered by z|s!? If not, I could always follow @Girgias suggestion.

@kocsismate suggestion is way better than mine and feasible, the z "format" corresponds to ZVAL meaning any variable type therefore you can check if it's an object, if not check if the zval can be converted to a zend_string, bailout if not. At this point you check if the second argument corresponds to null and then proceed to the correct call.

[kocsismate]

@jdomenechb Ah, thanks for the correction, you are certainly right! :) I think the solution I wrote (without the check in question) should still be implementable.

[kocsismate]

Suggested change

	if (ZEND_NUM_ARGS() == 1) {
	if (!name_str) {

But we'll need @cmb69 's approval for making the second parameter nullable for PHP 7.3. But it should absolutely work for PHP 8.

[cmb69]

Not a strict no from me, but I'd rather not change the signature in a stable version. @derickr what do you think about that?

[kocsismate]

Yeah, Probably adding the ! modifier only makes sense for PHP 8.

[jdomenechb]

Maybe to not complicate it further it would be better if I revert the code back to the first commit in this PR, and then apply this feedback. Then, for PHP 8, a different solution like having a nullable 2nd parameter, could be applied.

However, I am not sure if this is desirable and, in case it is, about the procedure to make a change in different PHP versions.

What do you think?

[kocsismate]

I think it's also a sensible idea. We could also ask from ourselves if this is really needed to be fixed in PHP 7 at all? Personally, I'd only fix it in 8, but ultimately, it's Christoph's and Derick's call :)

[cmb69]

I think that fixing the erroneous error message for PHP-7.3 makes sense (given there is no change in the signature, i.e. second parameter is not nullable). After that has been merged into PHP-7.4 and the "master" branch, another PR removing the function overload (i.e. second parameter made nullable) could follow up.

[kocsismate]

Suggested change

	if (ZEND_NUM_ARGS() == 1 || !name_str) {
	if (!name_str) {

name_str will be NULL if 1 argument is given

[jdomenechb]

I had to explicitly put ZEND_NUM_ARGS() == 1 || because when no parameter was provided, name_str was filled up with garbage. Despite i had been looking why for a very long time, I still ignore the reason...

[kocsismate]

You have to initialize name_str and name_len, since optional parameters are only initialized during ZPP if they are provided. So that's the reason :)

[jdomenechb]

Oh, thank you! My C is still a little rusty after so much time...

[kocsismate]

I know what you're talking about :) I needed quite a lot of time to have even a vague idea about what I see in php-src :)

[Girgias]

Oh, thank you! My C is still a little rusty after so much time...

PHP-src is more it's own version of C than actual C at times due to the bazillion macros, so yeah don't worry lol

[kocsismate]

@jdomenechb I know that you got contradicting suggestions :/ But are you still interested in this fix? Specifically, I'm wondering if you'd like to fix the issue for PHP 8? Since time starts to be pressing (there is 2 weeks left until RC1, which is the deadline for the more comprehensive solution), I'd take it over for this version very soon, unless you want to do it on your own :)

[jdomenechb]

@kocsismate I'm sorry, I wasn't available these previous days. I see that you already started progress on #6098 on master, for PHP 8. Are you interested that I do what I mention in #5995 (comment) for previous PHP versions?

[kocsismate]

@jdomenechb Yeah, sorry, I provided the PR because PHP 8 RC1 is tagged on Tuesday. For 7.3 and 7.4, feel free to fix the issue according to George's suggestions. My idea was only appropriate for a new version.

[nikic]

This has a clean fix in PHP 8, and I don't think we should do anything for older versions, as the machinery to do this properly doesn't exist there.

[jdomenechb]

Then I suggest to close this PR and take no action, as #6098 already fixes this in the newest version.

[nikic]

Okay, I'm closing this PR per the comments above.