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 #193

Merged

Conversation

Jubeki
Copy link
Contributor

@Jubeki Jubeki commented Jul 6, 2023

PHP-CS-Fixer Rule: nullable_type_declaration

With the given configuration this will change it like this (Taken from the PHP-CS-Fixer Page):

--- Original
+++ New
 <?php
-function bar(null|int $value, null|\Closure $callable): void {}
+function bar(?int $value, ?\Closure $callable): void {}
--- Original
+++ New
 <?php
 class ValueObject
 {
-    public null|string $name;
+    public ?string $name;
     public ?int $count;
-    public null|bool $internal;
-    public null|\Closure $callback;
+    public ?bool $internal;
+    public ?\Closure $callback;
 }

@driesvints
Copy link
Member

Thanks @Jubeki but I don't think we should do this. It's true that right now we do this in our own codebase but null is a valid type in PHP 8.2

If people have PHP 8.2 as a default for their project they'll probably want to use null instead of ?. So this could be opinionated. In the future when 8.2 is the minimum for Laravel and all of its first party packages (which is still a while away. And Pint has 8.2 as a default, we could change it to enforce null instead.

Side note: I also think null should always be placed as the last type like |null.

@Jubeki
Copy link
Contributor Author

Jubeki commented Jul 6, 2023

@driesvints I understand that, I don't think there as currently a rule where |null will be enforced at the end.

If you take a look at this PR/rule: #192 there would als be a configuration where the |null is added at the end (instead of only removing the null or question_mark).

Edit: I took a look at the rule, which I referenced, it also doesn't always enforce the |null with default values.

@taylorotwell taylorotwell merged commit 3421f9c into laravel:main Jul 7, 2023
5 checks passed
@Jubeki Jubeki deleted the add-rule/nullable_type_declaration branch July 9, 2023 13:07
@browner12
Copy link

I'm with @driesvints on this one. Much prefer |null over ?

@zepfietje
Copy link
Contributor

I'm with @driesvints on this one. Much prefer |null over ?

Me too. Also, it's already valid starting with PHP 8.0. PHP 8.2 added support for null as a standalone type, but that's not what this PR and rule are about.

@Wirone
Copy link

Wirone commented Jul 21, 2023

FYI: Instead of 'nullable_type_declaration' => true, you just could do 'nullable_type_declaration' => ['syntax' => 'union'], because rule works both ways.

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.

6 participants