-
Notifications
You must be signed in to change notification settings - Fork 506
Named argument detection is scope php-version dependent #3662
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
Conversation
This pull request has been marked as ready for review. |
@@ -1,7 +1,7 @@ | |||
<?php | |||
|
|||
namespace FunctionNamedArguments; | |||
|
|||
if (PHP_VERSION_ID < 80000) return; |
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.
We don't need this here, right? The test should run only on PHP 8+.
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 test which uses this file, has error expectations for PHP 7, see
phpstan-src/tests/PHPStan/Rules/Functions/CallToFunctionParametersRuleTest.php
Lines 494 to 499 in a0e007a
if (PHP_VERSION_ID < 80000) { | |
$errors[] = [ | |
'Missing parameter $arr1 (array) in call to function array_merge.', | |
14, | |
]; | |
} |
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.
It doesn't make sense to me. PHP 7.x does not support named arguments so it doesn't make sense to check them?
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.
so do you say I should skip the test on php7, or should the test be made to assert 0 errors on php7?
(and I guess the rule needs adjustments for that)
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 went with skipping the tests for now
5f93ffc
to
87418b6
Compare
Thank you. |
refs #3642 (comment)