Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
'<?php function foo(int $a = null, string $b) {}',
'<?php function foo(?int $a, string $b) {}',
There was a problem hiding this comment.
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 ofnullable_type_declaration_for_default_null_value
, butno_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.testThere was a problem hiding this comment.
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.
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 🙂.
@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 defaultnull
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 usenullable_type_declaration_for_default_null_value
withuse_nullable_type_declaration=true
+ there should be a separate, risky fixer which would remove default value for params not being after required params.There was a problem hiding this comment.
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)
.There was a problem hiding this comment.
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 -
?
ornull|
) with better name, e.g.ExplicitlyNullableParameterType
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😆).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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
whennull
is not default value (as followed by param without default)?There was a problem hiding this comment.
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 fixfoo($a = null, $b)
(optional before required) we need a new, risky fixer.