Skip to content
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

Merged
merged 1 commit into from Dec 13, 2022

Conversation

othercorey
Copy link
Contributor

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.

'Fiber::getReturn' => ['mixed'],
'Fiber::getCurrent' => ['?self'],
'Fiber::suspend' => ['mixed', 'value='=>'null|mixed'],
'FiberError::__construct' => ['void'],
Copy link
Contributor Author

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',
Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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 :).

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@orklah
Copy link
Collaborator

orklah commented Dec 4, 2022

Seems great! A few errors to solve in CI and it can be merged!

@othercorey othercorey changed the title Add validation for Object::Method names in CallMap Add validation for Class::Method names in CallMap Dec 5, 2022
@othercorey othercorey force-pushed the callmap-methods branch 2 times, most recently from 0bd11b1 to 7461ebe Compare December 5, 2022 05:46
@orklah orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Dec 13, 2022
@orklah orklah merged commit 00706de into vimeo:master Dec 13, 2022
@orklah
Copy link
Collaborator

orklah commented Dec 13, 2022

Thanks!

@othercorey othercorey deleted the callmap-methods branch December 13, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants