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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type checking for hook callbacks in add_action and add_filter #107

Merged
merged 57 commits into from Nov 9, 2022

Conversation

johnbillion
Copy link
Sponsor Contributor

@johnbillion johnbillion commented Jun 1, 2022

This implements part of #106.

I'm learning more about PHPStan internals as I work on this, so it's going to take some time 馃槃 . Once everything's working and covered by tests then I'll refactor it.

Applicable to all hooks:

  • The number used in the $accepted_args parameter should match the number of parameters that the callback accepts
  • The callback for add_filter must return a value
  • The callback for add_action must return void

Applicable when the hook signature is known (eg. a WordPress core hook):

  • The signature of the callback function should be checked against the parameters that get passed to the hook
  • A filter should not be used as an action, nor vice versa
  • The callback for add_filter must return a value with a type which is compatible with the type passed to it

I've decided to implement the WordPress core hook handling in a separate PR because it relies on the wp-hooks API package that I'm writing and it's not ready yet.

composer.json Outdated Show resolved Hide resolved
src/HookCallbackRule.php Outdated Show resolved Hide resolved
@szepeviktor
Copy link
Owner

This PR brings a fantastic feature to phpstan-wordpress.

@johnbillion
Copy link
Sponsor Contributor Author

We're all green but I want to add some more tests in.

@johnbillion johnbillion marked this pull request as ready for review August 16, 2022 21:53
@szepeviktor
Copy link
Owner

k茅p

Copy link
Owner

@szepeviktor szepeviktor left a comment

Choose a reason for hiding this comment

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

I'm waiting for our local PHPStan specialist.
@herndlm

@johnbillion
Copy link
Sponsor Contributor Author

@szepeviktor @herndlm Would be great to get this in

Copy link
Contributor

@herndlm herndlm left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to take a look at this, I was a bit frightened away in the beginning by the big diff, but this seems to be mostly coming from tests :)

I'm not a specialist for PHPStan Rules tbh, you might already know them better yourself. But I checked the rule and added a couple of comments, in general it looks well-made to me and very useful! I did not check the test thoroughly, you surely know the correct WP function behaviour there better than me :)

src/HookCallbackRule.php Show resolved Hide resolved
src/HookCallbackRule.php Show resolved Hide resolved
src/HookCallbackRule.php Show resolved Hide resolved
{
$acceptedType = $callbackAcceptor->getReturnType();
$accepted = $this->ruleLevelHelper->accepts(
new VoidType(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I never even used it myself yet, but does it make sense maybe to also add tests for the never return type? Not sure what we'd expect, e.g. if PHPStan or this rule is going to complain that the callable never returns / always exits 馃

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I'll take a look!

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Ah I've already got tests for this! https://github.com/szepeviktor/phpstan-wordpress/pull/107/files#diff-56e889cfab3a17dd1d1fbc7b198fcf3130ca973b4a7fd3342c696d29df13a96dR199-R213.

Unfortunately it is acceptable for both filters and actions to exit because WP YOLO. I'm not sure how VoidType relates to NeverType but this is working as expected and the tests pass, so I think we're all good here.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, ok. I missed those tests

@herndlm
Copy link
Contributor

herndlm commented Nov 9, 2022

ok then, I'd say go for it! :)

@szepeviktor szepeviktor merged commit e150358 into szepeviktor:master Nov 9, 2022
1 check passed
@szepeviktor
Copy link
Owner

Thank you! and Thank you!

@johnbillion johnbillion deleted the wp-hooks branch November 9, 2022 20:16
@szepeviktor
Copy link
Owner

@johnbillion @herndlm We have a bunch of problems.
https://app.travis-ci.com/github/szepeviktor/phpstan-wordpress/builds/257619024
Are they coming from this PR?

@szepeviktor
Copy link
Owner

Or PHPStan, or WP stubs?

@szepeviktor
Copy link
Owner

Last successful test back in September was also with WP 6.0.1

Locking php-stubs/wordpress-stubs (v6.0.1)

@herndlm
Copy link
Contributor

herndlm commented Nov 9, 2022

there are still the \FQCN problems there was another issue about. I had an easy solution in mind, but I think you didn't like it because it involved a phpstan min version bump. but I'll check that one out quickly.

and the rest are theme related type changes? could be stubs?

@szepeviktor
Copy link
Owner

szepeviktor commented Nov 9, 2022

there are still the \FQCN problems

Let's close our eyes for this type of problems! 馃檲

@johnbillion
Copy link
Sponsor Contributor Author

Yeah might be related to #118 or #119 :(

@szepeviktor
Copy link
Owner

All right guys!
Here comes a new release ...
馃

szepeviktor added a commit that referenced this pull request Nov 9, 2022
鈥#107) (#121)

* Setup the hook callback rule and test.

* Add the wp-hooks library.

* Start adding tests.

* Preparing more tests.

* If we don't have enough arguments, bail out and let PHPStan handle the error.

* If the callback is not valid, bail out and let PHPStan handle the error:

* Simplify this.

* More valid test cases.

* Update some test line numbers.

* First pass at detecting mismatching accepted and expected argument counts.

* Remove some accidental duplicates.

* Fix some logic.

* Let's split this up before it gets out of hand.

* Reduce this down.

* More tidying up.

* We're going to be needing this in multiple methods soon.

* Prep for callback return type checking, not yet functional.

* Split action return type assertion into its own method.

* Bring this message more inline with those used by PHPStan.

* Add detection for filter callbacks with no return value.

* Don't need this just yet.

* Use early exit to reduce code nesting.

* Remove unused imports.

* Yoda comparisons are disallowed.

* Coding standards.

* Remove unused code.

* Use early return to reduce code nesting.

* Remove more unused code.

* Renaming.

* Use early return to reduce code nesting.

* Tidying up.

* Correct logic introduced during refactoring.

* Exclude "maybe" callables.

* More handling for parameter counts.

* More handling for optional parameters in hook callbacks.

* Make the tests more manageable.

* Tests for more callback types.

* Tidying up.

* This isn't used.

* More test cases.

* Tests for variadic callbacks.

* More specific handling for variadic callbacks.

* More tests.

* The hooks names are not relevant to the tests.

* Remove an `else`.

* I'm still not entirely sure why this works, but it does. Props @herndlm.

* Another test for an action with a return value.

* More variadic handling.

* Turns out PHPStan handles typed and untyped functions differently.

* Partly broken callbacks will trigger an error in PHPStan so we don't need to handle them.

* Terminology.

* Refactoring.

* PHP 7.2 compatibility.

* Reorder the processing so the return type is checked first.

* More tests.

Co-authored-by: Viktor Sz茅pe <viktor@szepe.net>

Co-authored-by: John Blackbourn <john@humanmade.com>
@herndlm
Copy link
Contributor

herndlm commented Nov 9, 2022

oh, this was actually one thing, the FQCN change in PHPStan 1.8.7 was causing the WP_Theme extension class to not do anything anymore (because the extension relied on the old faulty behaviour). should be fixed with #122

szepeviktor added a commit that referenced this pull request Nov 9, 2022
* Fix FQCN

* Add type checking for hook callbacks in `add_action` and `add_filter` (#107) (#121)

* Setup the hook callback rule and test.

* Add the wp-hooks library.

* Start adding tests.

* Preparing more tests.

* If we don't have enough arguments, bail out and let PHPStan handle the error.

* If the callback is not valid, bail out and let PHPStan handle the error:

* Simplify this.

* More valid test cases.

* Update some test line numbers.

* First pass at detecting mismatching accepted and expected argument counts.

* Remove some accidental duplicates.

* Fix some logic.

* Let's split this up before it gets out of hand.

* Reduce this down.

* More tidying up.

* We're going to be needing this in multiple methods soon.

* Prep for callback return type checking, not yet functional.

* Split action return type assertion into its own method.

* Bring this message more inline with those used by PHPStan.

* Add detection for filter callbacks with no return value.

* Don't need this just yet.

* Use early exit to reduce code nesting.

* Remove unused imports.

* Yoda comparisons are disallowed.

* Coding standards.

* Remove unused code.

* Use early return to reduce code nesting.

* Remove more unused code.

* Renaming.

* Use early return to reduce code nesting.

* Tidying up.

* Correct logic introduced during refactoring.

* Exclude "maybe" callables.

* More handling for parameter counts.

* More handling for optional parameters in hook callbacks.

* Make the tests more manageable.

* Tests for more callback types.

* Tidying up.

* This isn't used.

* More test cases.

* Tests for variadic callbacks.

* More specific handling for variadic callbacks.

* More tests.

* The hooks names are not relevant to the tests.

* Remove an `else`.

* I'm still not entirely sure why this works, but it does. Props @herndlm.

* Another test for an action with a return value.

* More variadic handling.

* Turns out PHPStan handles typed and untyped functions differently.

* Partly broken callbacks will trigger an error in PHPStan so we don't need to handle them.

* Terminology.

* Refactoring.

* PHP 7.2 compatibility.

* Reorder the processing so the return type is checked first.

* More tests.

Co-authored-by: Viktor Sz茅pe <viktor@szepe.net>

Co-authored-by: John Blackbourn <john@humanmade.com>

* Fix

* Fix

Co-authored-by: John Blackbourn <john@humanmade.com>
@swissspidy
Copy link
Contributor

@johnbillion How well is this supposed to work yet? I am getting lots of "Filter callback return statement is missing" false positives.

Example:

https://github.com/GoogleForCreators/web-stories-wp/blob/c3b5eec59b0bb79dc096260f5c9614cffc3b3778/includes/Admin/Admin.php#L75C45-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.

Looks like it doesn't take @return doc blocks into account at all.

@johnbillion
Copy link
Sponsor Contributor Author

It's supposed to be perfect, beautiful, flawless, delightful, and kind, but it probably needs some tweaking. Can you open a new issue please?

@szepeviktor
Copy link
Owner

szepeviktor commented Nov 10, 2022

FYI @swissspidy There is no string|mixed type as mixed includes Everything :)

Please open that issue with the first problem 馃殌

@herndlm
Copy link
Contributor

herndlm commented Nov 11, 2022

thx again for this, renovatebot is just updating this in our projects, CI still green so far 馃 馃帀

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.

None yet

4 participants