-
Notifications
You must be signed in to change notification settings - Fork 672
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 validation for Class::Method names in CallMap #8826
Conversation
'Fiber::getReturn' => ['mixed'], | ||
'Fiber::getCurrent' => ['?self'], | ||
'Fiber::suspend' => ['mixed', 'value='=>'null|mixed'], | ||
'FiberError::__construct' => ['void'], |
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.
Moved to match alphabetical order.
'normalizer_get_raw_decomposition', | ||
'oauth_get_sbs', |
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.
These should be re-added. I will work on an updated list.
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.
do you want to do that now or in a next PR?
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.
Let me check this list before merging.
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 arrays should show only additions now.
We have problems where certain functions aren't ignored for specific PHP versions. If you run the test with PHP 8.1 it will tell you to remove some functions from the ignore list even though they will fail with PHP 8.0. This can be addressed separately.
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, that's an issue, CI is failing because of that. Maybe we should build the list with the version psalm uses for tests now and enforce that version in tests? The day where we want to change it, we'll have to update the list with the correct version?
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.
Yea, I'll look into it some more.
I'm pretty sure this tells us what functions/methods changed in 8.1 though :).
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.
@orklah Updated the ignore arrays to support php versions like the original $ignoredFunctions does. This should work for php 8.0 - 8.2, but there are probably a couple missing from php 8.2.
Looks like your github actions config doesn't enable most of the extensions anymore, but I generated the lists with as as many extensions as I could load.
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.
Most of the methods not ignored for PHP 8.0 are missing reflection information. It doesn't exactly mean that CallMap is right or wrong.
Seems great! A few errors to solve in CI and it can be merged! |
1151baf
to
cf91c5d
Compare
0bd11b1
to
7461ebe
Compare
7461ebe
to
45e2992
Compare
45e2992
to
e57079f
Compare
Thanks! |
We only validate classes that exist as the original provider only validate functions that exist.
Unreflectable methods are handled and validated. Sometimes these are methods that were removed in PHP and should be removed from the CallMap, but there are some like '__invoke' that are unreflectable.
Some of the methods in CallMap are missing return types, but this just ignores them for now.