psalm / psalm-plugin-laravel

A Psalm plugin for Laravel
MIT License
307 stars 72 forks source link

Migrations that make columns non-null are mistaken as nullable #274

Closed dpash closed 1 year ago

dpash commented 1 year ago

Describe the bug

If you have a migration that changes a column to non-null:

$table->string('column')->nullable(false)->change();

the migration parser ignores the false parameter. I imagine it would also be confused with non-changing migrations.

Impacted Versions

laravel-ide-helper/tree/v2.12.3barryvdh/laravel-ide-helper           v2.12.3           Laravel IDE Helper,...
laravel/frameworklaravel/framework                     v9.48.0           The Laravel Framework.
laravel/horizon/tree/v5.13.0laravel/horizon                       v5.13.0           Dashboard and code-...
laravel/pintlaravel/pint                          v1.4.0            An opinionated code...
laravel/saillaravel/sail                          v1.18.1           Docker files for ru...
laravel/sanctumlaravel/sanctum                       v3.2.1            Laravel Sanctum pro...
laravel/serializable-closurelaravel/serializable-closure          v1.2.2            Laravel Serializabl...
laravel/telescope/tree/v4.12.0laravel/telescope                     v4.12.0           An elegant debug as...
laravel/tinker/tree/v2.8.0laravel/tinker                        v2.8.0            Powerful REPL for t...
psalm/psalm-plugin-laravel/tree/v2.2.0psalm/plugin-laravel                  v2.2.0            A Laravel plugin fo...
laravel/tree/3.1.3sentry/sentry-laravel                 3.1.3             Laravel SDK for Sen...
laravel-health/tree/1.18.1spatie/laravel-health                 1.18.1            Monitor the health ...
laravel-ignitionspatie/laravel-ignition               1.6.4             A beautiful error p...
laravel-package-tools/tree/1.14.0spatie/laravel-package-tools          1.14.0            Tools for creating ...
laravel-permission/tree/5.8.0spatie/laravel-permission             5.8.0             Permission handling...
laravel-ray/tree/1.32.0spatie/laravel-ray                    1.32.0            Easily debug Larave...
psalm/tree/5.4.0vimeo/psalm                           5.4.0             A static analysis t...

Additional context This can be fixed with the following code at SchemaAggregator.php:191

                        $noParameter = count($root_var->args) === 0;
                        $trueParameter =
                            array_key_exists(0, $root_var->args)
                        && $root_var->args[0]->value instanceof PhpParser\Node\Expr\ConstFetch
                        && $root_var->args[0]->value->name instanceof PhpParser\Node\Name
                        && $root_var->args[0]->value->name->parts === ['true'];
                        $nullable = $noParameter || $trueParameter;
mr-feek commented 1 year ago

Thanks for the detailed report @dpash -- are you able to make a PR?

dpash commented 1 year ago

@mr-feek I've just updated the PR to fix some psalm and php-cs-fixer errors. Unit tests are failing, but I don't think that's anything I did.