search

szepeviktor / phpstan-wordpress

WordPress extensions for PHPStan ⛏️
https://packagist.org/packages/szepeviktor/phpstan-wordpress
MIT License
262 stars 26 forks source link

Fix #221 #223

Closed IanDelMar closed 2 months ago

IanDelMar commented 2 months ago

221 was caused by the VoidType accepting NullType from PHPStan version 1.10.50 onwards (see this commit).

This PR fixes #221 by using isVoid() to check for VoidType. isVoid() does not accept NullType.

Additionally, this PR adds a test for action callbacks that return null. Before this fix, the additional test was failing as well for the same reason.

Related: #222

szepeviktor commented 2 months ago

I am not able to say yes or no. Help!!

IanDelMar commented 2 months ago

@johnbillion @herndlm what do you think?

szepeviktor commented 2 months ago

If anyone says yes I will press the merge button.

szepeviktor commented 2 months ago

What nukes my brain: why complicate things?? why not all this is in the core?

herndlm commented 2 months ago

I'm abstaining, this fries my brain too. and I'm not that well accustomed with wordpress actions and filters

johnbillion commented 2 months ago

I'll take a look

johnbillion commented 2 months ago

I don't think this has much to do with the code quality of WordPress. It's working around an issue with the detection of void and null types in PHPStan.

@IanDelMar This change looks good and the PHPUnit tests are passing but there are some failing PHPStan sniffs. Will you take a look?

Why use space-age static analysis on PHP4-smelling WordPress?

You were the one who created this library 🤣

szepeviktor commented 2 months ago

I know how to upset Viktor.

szepeviktor commented 2 months ago

[ERROR] Found 4 errors

After fixing these I will merge it. Thank you John!

IanDelMar commented 2 months ago

@johnbillion Thank you for having a look!

[ERROR] Found 4 errors

After fixing these I will merge it. Thank you John!

3 of these were not introduced by this PR but are related to https://phpstan.org/blog/using-rule-error-builder#error-identifiers. They should be handled in a separate PR. Also, ultimately it's not up to me how this project names the errors.

szepeviktor commented 2 months ago

Thank you.