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

use explicitly nullable parameter types (implicit deprecated in php 8.4) #1369

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slepic
Copy link

@slepic slepic commented Jan 9, 2025

PHP supports nullable type declarations as of PHP 7.1 with the ?T syntax, and with the T|null syntax as of PHP 8.0 when generalized support for union types was added.

https://wiki.php.net/rfc/deprecate-implicitly-nullable-types

enqueue packages declares to require at least php ^7.4, therefore the ?T syntax is used here.

I believe that many of those parameters dont actually need a default value, but I leave that out because it might be BC if someone is actually calling those setters with no params.

@Steveb-p
Copy link
Contributor

Steveb-p commented Jan 9, 2025

I think we cannot do that. Changing argument types by widening them for classes that can potentially be extended - as we do not prohibit it - would be a breaking change (syntax error) if any of those methods is redeclared.

This deprecation can only be removed in major version, and as you've noted majority of those do not actually allow null values.

@slepic
Copy link
Author

slepic commented Jan 9, 2025

@Steveb-p

would be a breaking change (syntax error) if any of those methods is redeclared.

it's actually not widening the types, it just uses more uptodate syntax for the same

if you extend a class which has ?T $param = null, and change it to T $param = null in a child, it is totally ok except for triggering the deprecation notice in 8.4

see here
https://phpstan.org/r/0f1a0e2c-47cd-4a0a-9314-35f1e5dfe1d7


majority of those do not actually allow null values

actually right now, they do
they just probably shouldn't from the logic point of view,or they at least should require the parameter explicitly as in:

setThis(null);

rather than

setThis();

right now both are allowed, which may be artifact of the fact that T $param = null syntax prior to 7.1 was the only way to declare a typed parameter that could also be nullable, introducing a default value whether you wanted it or not.

since 7.1 the new syntax allows to distinguish between the two ?T $param = null vs. ?T $param.

but whether to remove the default = null is what need to be left for major, because in fact as you noted you may actually want non nullable T $param in some cases which needs case by case considerations.

while the contents of this PR are totally ok for a patch version

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.

2 participants