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

False positives in add_filter() type checking #125

Closed
swissspidy opened this issue Nov 10, 2022 · 5 comments · Fixed by #130
Closed

False positives in add_filter() type checking #125

swissspidy opened this issue Nov 10, 2022 · 5 comments · Fixed by #130

Comments

@swissspidy
Copy link
Sponsor Contributor

Re-sharing from #107 (comment)

The new rules check that callbacks added for add_filter actually return a value.

In my initial testing it doesn't always work, the rule only seems to check the return type declaration, but is ignoring any @return docblocks.

Example:

https://github.com/GoogleForCreators/web-stories-wp/blob/c3b5eec59b0bb79dc096260f5c9614cffc3b3778/includes/Admin/Admin.php#L75-L78

The top three lines are getting flagged because those methods don't have a return type (just in PHPDoc).
The last one has a PHP return type and is not flagged.

@johnbillion
Copy link
Sponsor Contributor

@swissspidy Does the analysis pass if you add declare(strict_types=1); to the top of the file?

@johnbillion
Copy link
Sponsor Contributor

If so, I'll need to look into how PHPStan varies its type checking based on that strict flag because I haven't figured it out yet.

@swissspidy
Copy link
Sponsor Contributor Author

declare(strict_types=1); doesn't appear to make a difference.

@herndlm
Copy link
Contributor

herndlm commented Nov 11, 2022

I found another false-positive with a mixed return type (which currently cannot be distinguished from a no return which is also mixed I think). I'll also try to check if I can improve it. actually this might be the same problem.

@johnbillion
Copy link
Sponsor Contributor

I started adding some failing tests to #130

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 a pull request may close this issue.

3 participants