Bug when duplicating model with `withDefault()` relation

[chelout]

• Laravel Version: 5.7.14
• PHP Version: 7.2.11
• Database Driver & Version: MySQL 8.0.12

Description:

Recently i encountered a bug when duplicating model with withDefault() relation, that raises Illuminate\Database\QueryException. Someone could say that it is a feature, but i'm confident it's a bug.

Steps To Reproduce:

Let's create two models:

Deal

<?php

use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Migrations\Migration;

class CreateDealsTable extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::create('deals', function (Blueprint $table) {
            $table->increments('id');
            $table->timestamps();

            $table->string('name');
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::dropIfExists('deals');
    }
}

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;

class Deal extends Model
{
    protected $fillable = [
        'name',
    ];

    public function transactions()
    {
        return $this->hasMany(Transaction::class);
    }
}

Transaction

<?php

use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Migrations\Migration;

class CreateTransactionsTable extends Migration
{
    /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::create('transactions', function (Blueprint $table) {
            $table->increments('id');
            $table->timestamps();

            $table->unsignedInteger('deal_id')->nullable()->comment('ID сделки');
            $table->foreign('deal_id')->references('id')->on('deals')->onDelete('SET NULL')->onUpdate('RESTRICT');
        });
    }

    /**
     * Reverse the migrations.
     *
     * @return void
     */
    public function down()
    {
        Schema::dropIfExists('transactions');
    }
}

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;

class Transaction extends Model
{
    public function deal()
    {
        return $this->belongsTo(Deal::class)
            ->withDefault();
    }
}

Filling data:

factory('App\Deal')->create([
    'name' => 'Test deal',
]);

// Transaction with relation
factory('App\Transaction')->create([
    'deal_id' => 1,
]);

$transaction = Transaction::with('deal')->find(1); // has relation
$new = $transaction->replicate();
$new->push(); // returns true

// Transaction without relation
factory('App\Transaction')->create();

$transaction = Transaction::with('deal')->find(1); // has relation
$new = $transaction->replicate();
$new->push(); // throws Illuminate/Database/QueryException

Proposal:

We can add one more check for such situation to Model's push method like so:

    public function push()
    {
        if (! $this->save()) {
            return false;
        }

        // To sync all of the relationships to the database, we will simply spin through
        // the relationships and save each model via this "push" method, which allows
        // us to recurse into all of these nested relations for the model instance.

        foreach ($this->relations as $models) {
            $models = $models instanceof Collection
                        ? $models->all() : [$models];

            foreach (array_filter($models) as $model) {
                if ($model->exists && ! $model->push()) {
                    return false;
                }
            }
        }

        return true;
    }

Added check for related model existance.

[chelout]

@driesvints if you confirm it's a bug, i can make a pr upon my proposal.

[driesvints]

@chelout I'll need more time to figure this out. I think you're correct but not sure.

[chelout]

@driesvints No problem, just let me know.

[staudenmeir]

The fix would break DatabaseEloquentModelTest::testPushOneRelation() and testPushManyRelation(), but I'm not entirely sure whether these tests actually make sense in their current form.

What they basically test:

$user = new User();
$user->setRelation('posts', new \Illuminate\Database\Eloquent\Collection([new Post]));
$user->push();

At the moment, push() saves both the user and the related post. But it doesn't set the foreign key, so the post ends up with user_id => null.

What would be a scenario where an unsaved related model makes sense?

I can only think of this rather theoretical example:

$user = User::first();
$post = new Post(['user_id' => $user->id]);
$user->setRelation('posts', new \Illuminate\Database\Eloquent\Collection([$post]));
$user->push();

[taylorotwell]

I'm not really sure I consider this a bug based on the current implementation and purpose of push.