wordpress-stubs icon indicating copy to clipboard operation
wordpress-stubs copied to clipboard

x|void is not a valid type

Open kkmuffme opened this issue 1 year ago • 6 comments

e.g. https://github.com/php-stubs/wordpress-stubs/blame/v6.6.2/functionMap.php#L69

The correct type would be string|null

If you think it doesn't matter, try this:

function foo(): string|void {
	if ( rand(0, 1 ) ) {
		return;
	}
	return 'foo';
}

kkmuffme avatar Oct 10 '24 13:10 kkmuffme

This is too painful. Please see https://github.com/szepeviktor/phpstan-wordpress/issues/176

szepeviktor avatar Oct 10 '24 13:10 szepeviktor

Thanks, but that issue misses the point:

  • yes, phpstan added support for x|void (and psalm has long had that)
  • yes, WP is bad for using these in the first place

However, that's irrelevant to begin with I think because:

  • where possible valid PHP types should be used, and x|null is, but x|void isn't
  • at some point someone will PR WP core to change these to return null;.

So if we have the option now, why keep using x|void in functionMap when we can do it correct now and not have to fix possibly tons of more types later on?

kkmuffme avatar Oct 10 '24 14:10 kkmuffme

What change do you propose? Please be gentle with me! I feel stomach pain when I see |void on my screen. Thank you.

szepeviktor avatar Oct 10 '24 14:10 szepeviktor

In functionMap.php change whatever|void to whatever|null

kkmuffme avatar Oct 10 '24 14:10 kkmuffme

@johnbillion @IanDelMar @swissspidy Please vote! It'll be highly "democratic". You vote then I say no.

szepeviktor avatar Oct 10 '24 14:10 szepeviktor

I think @kkmuffme made a very valid point. Additionally, there is/was a return type extension in phpstan-wordpress that did this as well. There is no reason to keep the void types in functionMap.php.

IanDelMar avatar Oct 10 '24 14:10 IanDelMar

Perhaps we need to be cautious with regard to hooks. Will PHPStan be able to recognise that an x|void function should not be used as a filter once it returns x|null according to the stubs?

IanDelMar avatar Nov 12 '24 12:11 IanDelMar

at some point someone will PR WP core to change these to return null;.

I started on this in the 6.7 release cycle, and will hopefully continue to make incremental progress on this and other PHPStan/typing issues in https://core.trac.wordpress.org/ticket/52217 (collab encouraged - I'm maintaining a list of tech-debt baselines in https://github.com/WordPress/wordpress-develop/pull/7619 ).

Probably not fast enough to obviate this ticket, but hopefully will lessen the impact either way 🤞

justlevine avatar Dec 09 '24 10:12 justlevine

The lack of software principles cause this return type. There should be two separate functions: one that returns a value, and a second one calling the first one.

szepeviktor avatar Dec 09 '24 10:12 szepeviktor

We have progress https://github.com/php-stubs/wordpress-stubs/actions/runs/14495264249/job/40661254343

szepeviktor avatar Apr 16 '25 14:04 szepeviktor

We have progress https://github.com/php-stubs/wordpress-stubs/actions/runs/14495264249/job/40661254343

And more otw 🤞https://core.trac.wordpress.org/ticket/63268

justlevine avatar Apr 16 '25 15:04 justlevine