Fix chaining of 'nullable' for 'foreignId' in Laravel 7 (#217)

Author: Pr3d4dor

src/Generators/MigrationGenerator.php

@@ -131,15 +131,24 @@ protected function buildDefinition(Model $model)
             $foreign_modifier = $column->isForeignKey();
 
             if ($this->shouldAddForeignKeyConstraint($column)) {
-                $foreign = $this->buildForeignKey($column->name(), $foreign_modifier === 'foreign' ? null : $foreign_modifier, $column->dataType(), $column->attributes());
+                $foreign = $this->buildForeignKey(
+                    $column->name(),
+                    $foreign_modifier === 'foreign' ? null : $foreign_modifier,
+                    $column->dataType(),
+                    $column->attributes(),
+                    $column->modifiers()
+                );
+
                 if ($column->dataType() === 'id' && $this->isLaravel7orNewer()) {
                     $column_definition = $foreign;
                     $foreign = '';
                 }
 
                 // TODO: unset the proper modifier
                 $modifiers = collect($modifiers)->reject(function ($modifier) {
-                    return (is_array($modifier) && key($modifier) === 'foreign') || $modifier === 'foreign';
+                    return ((is_array($modifier) && key($modifier) === 'foreign')
+                        || $modifier === 'foreign'
+                        || ($modifier === 'nullable' && $this->isLaravel7orNewer()));
                 });
             }
 
@@ -198,7 +207,7 @@ protected function buildPivotTableDefinition(array $segments)
         return trim($definition);
     }
 
-    protected function buildForeignKey(string $column_name, ?string $on, string $type, array $attributes = [])
+    protected function buildForeignKey(string $column_name, ?string $on, string $type, array $attributes = [], array $modifiers = [])
     {
         if (is_null($on)) {
             $table = Str::plural(Str::beforeLast($column_name, '_'));
@@ -216,14 +225,18 @@ protected function buildForeignKey(string $column_name, ?string $on, string $typ
         }
 
         if ($this->isLaravel7orNewer() && $type === 'id') {
+            $prefix = in_array('nullable', $modifiers)
+                ? '$table->foreignId' . "('{$column_name}')->nullable()"
+                : '$table->foreignId' . "('{$column_name}')";
+
             if ($column_name === Str::singular($table) . '_' . $column) {
-                return self::INDENT . '$table->foreignId' . "('{$column_name}')->constrained()->cascadeOnDelete()";
+                return self::INDENT . "{$prefix}->constrained()->cascadeOnDelete()";
             }
             if ($column === 'id') {
-                return self::INDENT . '$table->foreignId' . "('{$column_name}')->constrained('{$table}')->cascadeOnDelete()";
+                return self::INDENT . "{$prefix}->constrained('{$table}')->cascadeOnDelete()";
             }
 
-            return self::INDENT . '$table->foreignId' . "('{$column_name}')->constrained('{$table}', '{$column}')->cascadeOnDelete()";
+            return self::INDENT . "{$prefix}->constrained('{$table}', '{$column}')->cascadeOnDelete()";
         }
 
         return self::INDENT . '$table->foreign' . "('{$column_name}')->references('{$column}')->on('{$table}')->onDelete('cascade')";

tests/Feature/Generator/MigrationGeneratorTest.php

@@ -368,6 +368,60 @@ public function output_does_not_duplicate_pivot_table_migration_laravel6()
         $this->assertEquals(['created' => [$company_migration, $people_migration, $pivot_migration]], $this->subject->output($tree));
     }
 
+    /**
+     * @test
+     */
+    public function output_creates_foreign_keys_with_nullable_chained_correctly()
+    {
+        $this->files->expects('stub')
+            ->with('migration.stub')
+            ->andReturn(file_get_contents('stubs/migration.stub'));
+
+        $now = Carbon::now();
+        Carbon::setTestNow($now);
+
+        $model_migration = str_replace('timestamp', $now->format('Y_m_d_His'), 'database/migrations/timestamp_create_carts_table.php');
+
+        $this->files
+            ->expects('put')
+            ->with($model_migration, $this->fixture('migrations/nullable-chaining.php'));
+
+        $tokens = $this->blueprint->parse($this->fixture('definitions/nullable-chaining.bp'));
+        $tree = $this->blueprint->analyze($tokens);
+
+        $this->assertEquals(['created' => [$model_migration]], $this->subject->output($tree));
+    }
+
+    /**
+     * @test
+     */
+    public function output_creates_foreign_keys_with_nullable_chained_correctly_laravel6()
+    {
+        $app = \Mockery::mock();
+        $app->shouldReceive('version')
+            ->withNoArgs()
+            ->andReturn('6.0.0');
+        App::swap($app);
+
+        $this->files->expects('stub')
+            ->with('migration.stub')
+            ->andReturn(file_get_contents('stubs/migration.stub'));
+
+        $now = Carbon::now();
+        Carbon::setTestNow($now);
+
+        $model_migration = str_replace('timestamp', $now->format('Y_m_d_His'), 'database/migrations/timestamp_create_carts_table.php');
+
+        $this->files
+            ->expects('put')
+            ->with($model_migration, $this->fixture('migrations/nullable-chaining-laravel6.php'));
+
+        $tokens = $this->blueprint->parse($this->fixture('definitions/nullable-chaining.bp'));
+        $tree = $this->blueprint->analyze($tokens);
+
+        $this->assertEquals(['created' => [$model_migration]], $this->subject->output($tree));
+    }
+
     public function modelTreeDataProvider()
     {
         return [

tests/fixtures/definitions/nullable-chaining.bp

@@ -0,0 +1,4 @@
+models:
+  Cart:
+    name: string
+    user_id: id foreign nullable

tests/fixtures/migrations/nullable-chaining-laravel6.php

@@ -0,0 +1,34 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+class CreateCartsTable extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::create('carts', function (Blueprint $table) {
+            $table->bigIncrements('id');
+            $table->string('name');
+            $table->unsignedBigInteger('user_id')->nullable();
+            $table->foreign('user_id')->references('id')->on('users')->onDelete('cascade');
+            $table->timestamps();
+        });
+    }
+
+    /**
+     * Reverse the migrations.
+     *
+     * @return void
+     */
+    public function down()
+    {
+        Schema::dropIfExists('carts');
+    }
+}

tests/fixtures/migrations/nullable-chaining.php

@@ -0,0 +1,33 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+class CreateCartsTable extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::create('carts', function (Blueprint $table) {
+            $table->id();
+            $table->string('name');
+            $table->foreignId('user_id')->nullable()->constrained()->cascadeOnDelete();
+            $table->timestamps();
+        });
+    }
+
+    /**
+     * Reverse the migrations.
+     *
+     * @return void
+     */
+    public function down()
+    {
+        Schema::dropIfExists('carts');
+    }
+}