phpstan-wordpress
phpstan-wordpress copied to clipboard
szepeviktor
`void` vs. `null`
Should we ditch void in the EchoKey and the EchoParameter extensions and use null instead? Technically there is no such thing as void in PHP. However, as of PHP 7.1.0 there is a void return type. The PHP manual says
void is a return-only type declaration indicating the function does not return a value, but the function may still terminate. Therefore, it cannot be part of a union type declaration. Available as of PHP 7.1.0.
It also says
Note: Even if a function has a return type of void it will still return a value, this value is always null.
WordPress does use void in union types in doc blocks... which actually should be nullable return types. See the_title() for an example. Having void as return type for these functions leads to
sprintf('', the_title('<span>', '</span>', false))
giving Parameter #2 ...$values of function sprintf expects bool|float|int|string|null, string|void given. though sprintf() is completely fine with the_title('<span>', '</span>', false) as it is either string or null.
I don't know why this didn't pop up earlier.
Static analysis is about strict types.
I am sorry.
Void is different from null.
When I should return null a simple return; is an error.
I consider PHP functions returning implicit null-s a language error. It must be something from PHP 4.
Wait!
Return: Void if $display argument is true, current post title if $display is false.
Why do we have an extra condition in our code for the_title?
https://github.com/szepeviktor/phpstan-wordpress/blob/51a29b68231d0ff4f0606274a70ea95c398dbe61/src/EchoParameterDynamicFunctionReturnTypeExtension.php#L113-L118
Because for the_title() void is always possible irrespective of the value of $display.
I see! When title is an empty string.
If that is the case one should always check whether the return value of the_title is a string.
I get sick 🤢 in MINUTES when talking about WordPress.
So properly written code for WordPress is long, hard to understand and silly. I need to heal.
sprintf('', the_title('<span>', '</span>', false) ?? '')
There is no solution in PHP for detecting void.
One is forced to use /** @var string|null */
You healing or the code?
Me healing. This rabbit hole is stronger than me. Each of these issues makes me delete all my software for WordPress from the Internet.
I was also annoyed already by the fact that wpdb::prepare has a string|void return type (see also https://developer.wordpress.org/reference%2Fclasses%2Fwpdb%2Fprepare%2F/) and I can't just cast the return to string without phpstan complaining. string|null woudl allow that. But I think I just accepted it..
See also https://phpstan.org/r/23820230-f5a3-4533-9543-79b453cd5bd8
But if they internally just do return; and not return null; then the type is technically correct I suppose? It is indeed a mess though..
but wait, as @IanDelMar quoted in the beginning
Therefore, it cannot be part of a union type declaration.
and indeed it crashes spectacularly: https://3v4l.org/iRfF2
I wonder if PHPStan should be more strict with these invalid unions. Considering that PHPDoc is interpreted as it would be native type by default, the type is invalid IMO. I'll open a PHPStan issue.
@johnbillion do you think we can change all those implicit null returns from return; to return null; and adapt the return types? how high is the chance to get this in WP itself? :) union types with void are invalid and looks like PHPStan is going to check this at some point too..
what would be alternative if WP is not doing this - adapt it in the stubs I suppose?
Changing a return; to return null; shouldn't have any side effects because if anything is reading the return value it'll be treated as null. Example: https://3v4l.org/ZXFKY .
Making those functions nullable instead of voidable sounds like a good idea.
Making those functions nullable instead of voidable sounds like a good idea.
Okay. Let's go on with this good idea.
The return type extension for redirect_canonical() does override WP's incorrect usage of void with null. It seems as this has already been some issue in the past.
Has anyone opened a ticket to get this into core? Or is planning to do so?
fyi this is going to be dealt with here: https://github.com/phpstan/phpstan-src/pull/2778
all void returns are going to be transformed to null after calling a function and e.g. assigning them to a variable. that should avoid the issues some had here too. it should be fine to keep them in the phpdoc / stubs / wordpress.
I dream about WordPress not having string|void-s and PHP throwing fatal errors when void return values are used.
fyi this is going to be dealt with here: phpstan/phpstan-src#2778
all
voidreturns are going to be transformed tonullafter calling a function and e.g. assigning them to a variable. that should avoid the issues some had here too. it should be fine to keep them in the phpdoc / stubs / wordpress.
PHPStan v 1.10.50 has this feature.
I think you can close this one, no?
UPDATE: or maybe bump min PHPStan version and adapt tests if necessary? UPDATE2: nothing changed, not sure why, maybe the extensions should be adapted after all.