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

Add Rule: nullable_type_declaration_for_default_null_value #192

Conversation

Jubeki
Copy link
Contributor

@Jubeki Jubeki commented Jul 6, 2023

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):

--- Original
+++ New
 <?php
-function sample(?string $str = null)
+function sample(string $str = null)
 {}
--- Original
+++ New
 <?php
-function sample(string|int|null $str = null)
+function sample(string|int $str = null)
 {}
--- Original
+++ New
 <?php
-function sample((\Foo&\Bar)|null $str = null)
+function sample(\Foo&\Bar $str = null)
 {}

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:

Copy link
Member

@driesvints driesvints left a 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 👍

https://3v4l.org/3lGsj

@taylorotwell
Copy link
Member

Hey @Jubeki - can you resolve the conflict on this and mark ready for review again? Thanks!

@taylorotwell taylorotwell marked this pull request as draft July 7, 2023 06:41
@Jubeki Jubeki force-pushed the add-rule/nullable_type_declaration_for_default_null_value branch from 3cefe7e to 29f5299 Compare July 7, 2023 07:18
@Jubeki Jubeki marked this pull request as ready for review July 7, 2023 07:18
@taylorotwell taylorotwell merged commit c46749b into laravel:main Jul 8, 2023
4 checks passed
@Jubeki Jubeki deleted the add-rule/nullable_type_declaration_for_default_null_value branch July 9, 2023 13:06
@woganmay
Copy link

I know this PR has already been merged, but I'm not sure the semantics of the change make sense. PHP's ? operator indicates that the value is optional and will default to null if not provided, so these are readable as:

  • sample(string $str = null) means "A string value $str is required and will default to null if not provided"
  • sample(?string $str = null) means "A string value $str is NOT required and will default to null if not provided"
  • sample(?string $str) means "A string value $str is NOT required" and "by PHP default behavior, will default to null if not provided"

From a code readability standpoint, ?string reads more explicitly than string $str = null, in that it clearly indicates that $str is nullable. I don't know what the general consensus on this is in the PHP community, but I would have thought that pushing for ? being the standard for indicating nullable makes the most sense?

@adevade
Copy link

adevade commented Aug 9, 2023

I think it would look better to keep the ? as well. Using the latest Pint gave me the following diff:

- command(?string $to = null, ?string $forward = 'RootEntityId')
+ command(string $to = null, ?string $forward = 'RootEntityId')

Removing the first ? and keeping the second one.

Feels more consistent and easier to keep the question mark.

@andrade-felipe-dev
Copy link

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.

@dennisvandalen
Copy link

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 pint.json:

{
    "preset": "laravel",
    "rules": {
        "nullable_type_declaration_for_default_null_value": true
    }
}

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.

7 participants