Skip to content

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet
Copy link
Contributor Author

Params names are coming from https://www.php.net/manual/en/function.extract.php
They are also here:
https://github.com/php/php-src/blob/7e45e57d8f85f8d1fa2521028f394da30268187d/ext/standard/basic_functions.stub.php#L1643-L1644

I added some tests to check & does not break the extract inference.

Is something more needed for this PR ? @ondrejmirtes

@thg2k
Copy link
Contributor

thg2k commented Oct 12, 2024

The param names should be the ones from php7. Reflection takes care of the correct names for 8.x.

@VincentLanglet
Copy link
Contributor Author

The param names should be the ones from php7. Reflection takes care of the correct names for 8.x.

Was the param names different in php 7 ?

@claudepache
Copy link
Contributor

Was the param names different in php 7 ?

See https://3v4l.org/kMrVd . The current parameter names aren’t exactly those of PHP 7 either…

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Nov 12, 2024

Since named parameter is only a PHP 8 feature, isn't it better to use them in stubs ?
I'm not sure what's the best for PHPStan

assertVariableCertainty(TrinaryLogic::createYes(), $foo);
assertType('42', $foo);

$foo = ['foo' => 42];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would make more sense to test this separately in a different function.

@ondrejmirtes
Copy link
Member

Also: please search for @prefer-ref in vendor/phpstan/php-8-stubs and update all the functions you find too.

@VincentLanglet
Copy link
Contributor Author

Would make more sense to test this separately in a different function.

Done in f9fa213

Also: please search for @prefer-ref in vendor/phpstan/php-8-stubs and update all the functions you find too.

Done in b5e5de5 if I understood correctly

'array_merge' => ['array', 'arr1'=>'array', '...args='=>'array'],
'array_merge_recursive' => ['array', 'arr1'=>'array', '...args='=>'array'],
'array_multisort' => ['bool', '&rw_array1'=>'array', 'array1_sort_order='=>'array|int', 'array1_sort_flags='=>'array|int', '...args='=>'array|int'],
'array_multisort' => ['bool', 'rw_array1'=>'array', 'array1_sort_order='=>'array|int', 'array1_sort_flags='=>'array|int', '...args='=>'array|int'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the parameter is array1, not rw_array1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw many other param with a wrong name, like

  • array_pop => rw_stack
  • array_push => rw_stack
  • array_shift => rw_stack
  • array_splice => rw_input
  • array_unsplice => rw_stack
  • ...

There is at least 63 occurences of &rw_ param which might need to be checked...

Do you want me to open another PR with such param renaming ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't part of the name when there's & in front of them. There's an explanation at the top of functionMap.php

@ondrejmirtes ondrejmirtes merged commit ceac309 into phpstan:1.12.x Nov 13, 2024
452 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants