sebastianbergmann
@no-named-arguments leads to static analysis errors for variadic arguments
| Q | A |
|---|---|
| PHPUnit version | at least version 9 |
Summary
The InvocationMocker class is annotated with @no-named-arguments - as this is on a class-level, it also affects the with() function, that only has a single variadic argument. This leads to static analysis issues.
Current behavior
When using PHPStan in a project where with() is used with an array (not a list), this may lead to a static analysis error (although the keys are ignored anyways):
Method […]\InvocationMocker::willReturn() invoked with unpacked array with possibly string key, but it's not allowed because of @no-named-arguments.
How to reproduce
$input = ['a' => 'foo', 'b' => 'bar'];
$mock->with(...$input); // invalid
$mock->with(...array_values($input)); // valid
$mock->willReturn(/* … */);
Expected behavior
Both cases are a valid usage, so I'd expect there would be no static analysis errors.
This sounds like a shortcoming of PHPStan to me.
What do you think, @ondrejmirtes @staabm?
PHPUnit could in theory make use of the array keys of a ...$parameters argument, i.e. to map it as named parameters to the mocked function. But any name changes to the PHPUnit method signature would not break the consumer's code.
But in general, you could use the $parameters to invoke a library function, and then it would matter again. So not sure if PHPStan would have a way to detect this.
Not sure what PHPStan should do here. This is named arguments call:
$input = ['a' => 'foo', 'b' => 'bar'];
$mock->with(...$input); // invalid
So PHPStan marks it as such. If you think there should be some exceptions to what is marked as valid and invalid with @no-named-arguments, please open a feature request on PHPStan's GitHub.
My opinion is that if PHPUnit allows this use-case, there shouldn't be class-wide @no-named-arguments, but only above methods which do not support named arguments.
PHPUnit has @no-named-arguments for all classes, interfaces, traits, functions because parameter names are not covered by the backward compatibility promise for PHPUnit.
That being said, the argument can be made to have an exception from this rule. However, I am not sure that the use-case mentioned here is actually supported.
However, I am not sure that the use-case mentioned here is actually supported
What do you mean? Why shouldn't it be OK to pass an associative array to a function that just decides to use its values? There is nothing bad in doing so.
What does passing an associative array as an argument have to do with named arguments?
in combination with the ... operator the named keys of the argument array is mapped to the variable names
https://3v4l.org/gON2X
Can you please clarify from which method(s) you propose the @no-named-arguments to be removed? Thank you.
I'm not OP, but I think the only method from which it should be removed is with https://github.com/sebastianbergmann/phpunit/blob/004ea3b3edc88ac9263ddbd50eb818cff51c591e/src/Framework/MockObject/Runtime/Interface/InvocationStubber.php#L46
For will*, there are variadic arguments, but it does not make sense to use named arguments here since PHP does not allow multiple return values.
It would be cool if there was a @named-arguments allowing to cancel the @no-named-arguments annotation just for this one method.
By the way, a perhaps more compelling reason to be able to do this:
$mock->with(meaningfulParameter: true, otherMeaningfulParameter: false);
Here, meaningfulParameter and otherMeaningfulParameter are part of the API of the code under test, not PHPUnit's API.
