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

Fix getFunctionIdsFromCallableArg to return the function id for array($this, 'foo') in params providers for example #10745

Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from

Conversation

kkmuffme
Copy link
Contributor

Required to get consistent types in e.g. params providers used in plugins.

@kkmuffme kkmuffme force-pushed the fix-getting-function-id-from-this-in-param-type-provider branch from 7da878b to 5b7b6eb Compare February 24, 2024 12:37
@kkmuffme kkmuffme marked this pull request as ready for review February 24, 2024 12:41
@kkmuffme
Copy link
Contributor Author

@weirdan merge please

Copy link
Collaborator

@weirdan weirdan left a comment

Choose a reason for hiding this comment

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

Can you provide a test demonstrating what this change affects?

@kkmuffme
Copy link
Contributor Author

@weirdan this is an issue I encountered in an internal plugin we use - it's a custom params provider for a custom callable.
Where would I put this params provider plugin in psalm's tests to test this?

@weirdan
Copy link
Collaborator

weirdan commented Feb 26, 2024

Wherever tests/Config/Plugin/Hook/FooMethodProvider.php is used I suppose

@kkmuffme kkmuffme force-pushed the fix-getting-function-id-from-this-in-param-type-provider branch from 8744b3b to 2bdba92 Compare February 29, 2024 16:07
…($this, 'foo') in params providers for example
@kkmuffme kkmuffme force-pushed the fix-getting-function-id-from-this-in-param-type-provider branch from b761040 to bf1fc66 Compare February 29, 2024 16:23
Copy link
Contributor Author

@kkmuffme kkmuffme left a comment

Choose a reason for hiding this comment

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

Added test

@kkmuffme
Copy link
Contributor Author

@weirdan please merge

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

Successfully merging this pull request may close these issues.

None yet

2 participants