-
Notifications
You must be signed in to change notification settings - Fork 144
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
Add Rule: nullable_type_declaration_for_default_null_value #192
Add Rule: nullable_type_declaration_for_default_null_value #192
Conversation
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.
Seems to make sense 👍
Hey @Jubeki - can you resolve the conflict on this and mark ready for review again? Thanks! |
3cefe7e
to
29f5299
Compare
I know this PR has already been merged, but I'm not sure the semantics of the change make sense. PHP's
From a code readability standpoint, |
I think it would look better to keep the - command(?string $to = null, ?string $forward = 'RootEntityId')
+ command(string $to = null, ?string $forward = 'RootEntityId') Removing the first Feels more consistent and easier to keep the question mark. |
o use it the way it was with the "?" indicator. Would I have to change the pint version in the composer? Because if this change were made it would be necessary to at least have some configuration to keep it as it was. |
Update your
|
PHP-CS-Fixer Rule:
nullable_type_declaration_for_default_null_value
With the given configuration this will change it like this (Taken from the PHP-CS-Fixer Page):
This rule is currently not enforced in
laravel/framework
repository, but I saw some comments in PR's were it was said that the question mark should not be added because of the null default argument:Illuminate\Mail\Mailables\Envelope
Illuminate\Process\Factory