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

fix: NullableTypeDeclarationForDefaultNullValueFixer - do not create implicitly nullable parameter types #7830

Conversation

kubawerlos
Copy link
Contributor

Fixes #7787

@coveralls
Copy link

coveralls commented Feb 8, 2024

Coverage Status

coverage: 94.775% (+0.001%) from 94.774%
when pulling f20fcfe on 6b7562617765726c6f73:fix_NullableTypeDeclarationForDefaultNullValueFixer
into e383c84 on PHP-CS-Fixer:master.

@kubawerlos kubawerlos marked this pull request as ready for review February 8, 2024 14:30
yield 'do not create implicitly nullable parameter types' => [
// @see https://wiki.php.net/rfc/deprecate-implicitly-nullable-types
'<?php function foo(?int $a = null, string $b) {}',
null,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test case I would expeect is one that turns 1 into 2:

  1. '<?php function foo(int $a = null, string $b) {}',
  2. '<?php function foo(?int $a, string $b) {}',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing = null is not the responsibility of nullable_type_declaration_for_default_null_value, but no_unreachable_default_argument_value, and using both is needed to achieve that transformation, which is happening already, without this fix: https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/v3.49.0/tests/Fixtures/Integration/priority/nullable_type_declaration_for_default_null_value%2Cno_unreachable_default_argument_value.test

Copy link
Member

@Wirone Wirone Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kubawerlos This fix prevents the fixer from creating implicit nullable param when there are required params, which is somehow confusing - it does not fix the main issue (optional param being before required param) but at the same time it deviates from the behaviour under use_nullable_type_declaration=false as it does not do what's the sole purpose of this fixer 🤔.

I also don't get what's the difference between current example 2 and the test case in this PR.

-- Original
+++ New
 <?php
-function sample(?string $str = null)
+function sample(string $str = null)
 {}

I mean, I know that the difference is that one has only one parameter, and the other has two, but when it comes to the RFC, one prevents deprecated syntax, while the other not. I am not sure if it should work this way. Just thinking out loud 🙂.

I think the test case I would expeect is one that turns 1 into 2:

  1. '<?php function foo(int $a = null, string $b) {}',
  2. '<?php function foo(?int $a, string $b) {}',

@nicolas-grekas I believe this is out of the scope of this particular fixer in its current state, which is about handling nullable type (? or |null) for params with default null value. It does not touch the default value itself, and even if adding such feature is technically possible, it would make this rule risky (compared to this). I believe in this case you should just use nullable_type_declaration_for_default_null_value with use_nullable_type_declaration=true + there should be a separate, risky fixer which would remove default value for params not being after required params.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the end user pov, the current behavior is the risky one, because it turns
a non-deprecated syntax: (int $a = null, $b)
into a deprecated one: (?int $a = null, $b).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it totally wrong 😁

After more thinking about this, I now think all the whole ['use_nullable_type_declaration' => false] strategy is about creating implicitly nullable parameter types, which is going to be deprecated in PHP 8.4.

Maybe the right direction is to deprecate that strategy in this fixer? Or, to deprecate this fixer in favour of new one (that could be configurable with what to use -? or null|) with better name, e.g. ExplicitlyNullableParameterType?

Copy link
Member

@Wirone Wirone Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After more thinking about this, I now think all the whole ['use_nullable_type_declaration' => false] strategy is about creating implicitly nullable parameter types, which is going to be deprecated in PHP 8.4.

Maybe the right direction is to deprecate that strategy in this fixer?

I wanted to write it before and I forgot after writing all that thoughts 😆. Yeah, we should deprecate the nullable_type_declaration_for_default_null_value option and from next major version this fixer should only ensure explicit nullability. Fixer's name is OK in my opinion, as the expected code should have "nullable type" (which is implicitly about explicitness 😆).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the end user pov, the current behavior is the risky one, because it turns
a non-deprecated syntax: (int $a = null, $b)
into a deprecated one: (?int $a = null, $b).

Only considering the RFC, which was created way after the rule 😉. Previously it was only a matter of preference.

Anyway, it only shows we should deprecate the config option and allow only enforcing explicit nullability in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this fixer: https://cs.symfony.com/doc/rules/language_construct/nullable_type_declaration.html ? Maybe this one should take care of int $a = null when null is not default value (as followed by param without default)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullable_type_declaration also does not cover default values, only standardise the way how nullability is defined (? vs |null). IMHO if we want to fix foo($a = null, $b) (optional before required) we need a new, risky fixer.

@kubawerlos kubawerlos closed this Feb 8, 2024
@kubawerlos kubawerlos deleted the fix_NullableTypeDeclarationForDefaultNullValueFixer branch February 8, 2024 16:09
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.

Rule nullable_type_declaration_for_default_null_value should remove default value on non-optional arguments
4 participants