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

x|void is not a valid type #250

Open
kkmuffme opened this issue Oct 10, 2024 · 9 comments
Open

x|void is not a valid type #250

kkmuffme opened this issue Oct 10, 2024 · 9 comments

Comments

@kkmuffme
Copy link

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';
}
@szepeviktor
Copy link
Member

This is too painful.
Please see szepeviktor/phpstan-wordpress#176

@kkmuffme
Copy link
Author

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?

@szepeviktor
Copy link
Member

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

@kkmuffme
Copy link
Author

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

@szepeviktor
Copy link
Member

szepeviktor commented Oct 10, 2024

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

@IanDelMar
Copy link
Contributor

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.

@php-stubs php-stubs deleted a comment Oct 25, 2024
@IanDelMar
Copy link
Contributor

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?

@justlevine
Copy link

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 WordPress/wordpress-develop#7619 ).

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

@szepeviktor
Copy link
Member

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.

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

No branches or pull requests

5 participants
@szepeviktor @kkmuffme @justlevine @IanDelMar and others