Framework: Pivot::original contains incorrect data

[KKSzymanowski]

• Laravel Version: 5.8.19
• PHP Version: 7.1.13
• Database Driver & Version: MySQL 5.5.5-10.3.8-MariaDB

Description:

After creating a Model, the original array is empty and the attributes array contains all the attributes used to create the Model and optionally the auto-incrementing id.
Same is true when a Pivot is created via FooPivot::create().

However after creating a Pivot using the $model->relationship()->attach($id, $attributes) method, the original array contains all of the foreign keys and the pivot attributes passed to the attach() call.

Steps To Reproduce:

Run:

php artisan make:model Invoice -m 
php artisan make:model Product -m 
php artisan make:model InvoiceProduct -m -p

Fill in the migrations:
create_invoices_table:

Schema::create('invoices', function (Blueprint $table) {
    $table->bigIncrements('id');
    $table->string('number');
    $table->timestamps();
});

create_invoice_product_table:

Schema::create('invoice_product', function (Blueprint $table) {
    $table->unsignedInteger('invoice_id');
    $table->unsignedInteger('product_id');
    $table->unsignedInteger('count');
});

You can leave the create_products_table migration unchanged.

Update the models:
App\Invoice:

class Invoice extends Model
{
    protected $guarded = [];

    public static function boot()
    {
        parent::boot();

        static::created(function (Model $model) {
            dump([
                'attributes' => $model->getAttributes(),
                'original'   => $model->getOriginal(),
                'dirty'      => $model->getDirty(),
            ]);
        });
    }

    public function products()
    {
        return $this->belongsToMany(Product::class)
                    ->using(InvoiceProduct::class)
                    ->withPivot('count');
    }
}

App\InvoiceProduct:

class InvoiceProduct extends Pivot
{
    public static function boot()
    {
        parent::boot();

        static::created(function (Model $model) {
            dump([
                'attributes' => $model->getAttributes(),
                'original'   => $model->getOriginal(),
                'dirty'      => $model->getDirty(),
            ]);
        });
    }
}

You can leave the App\Product model unchanged.

Run:

php artisan migrate

In the Tinker run:

$invoice = \App\Invoice::create(['number' => '123']);

You should see the correct attributes, original and dirty arrays:

array:3 [
  "attributes" => array:4 [
    "number" => "123"
    "updated_at" => "2019-06-01 10:08:57"
    "created_at" => "2019-06-01 10:08:57"
    "id" => 1
  ]
  "original" => []
  "dirty" => array:4 [
    "number" => "123"
    "updated_at" => "2019-06-01 10:08:57"
    "created_at" => "2019-06-01 10:08:57"
    "id" => 1
  ]
]

Then, again in the Tinker, run:

$product = \App\Product::create();
$invoice->products()->attach($product, ['count' => 2]);

You should see the following output:

array:3 [
  "attributes" => array:3 [
    "product_id" => 1
    "invoice_id" => 1
    "count" => 2
  ]
  "original" => array:3 [
    "product_id" => 1
    "invoice_id" => 1
    "count" => 2
  ]
  "dirty" => []
]

Clearly the pivot has just been created, therefore the original array should be empty and the dirty array should contain at least the count field.

One side note: If you add auto-incrementing id to the pivot table and enable it with

public $incrementing = true;

you're gonna see the id field in the dirty array but not in the original array.

[staudenmeir]

/cc @ralphschindler

[ralphschindler]

So currently, original and attributes are both populated because AsPivot calls setRawAttributes on the new model with the sync parameter set to true (this ensures both original and attributes are the same).

https://github.com/laravel/framework/blob/master/src/Illuminate/Database/Eloquent/Relations/Concerns/AsPivot.php#L80

I don't know what the original motivation was to sync them. Removing the sync flag causes another assertion in another test to fail (I am unsure yet as to if the failing test is reasonable or not):

https://github.com/laravel/framework/blob/master/tests/Integration/Database/EloquentBelongsToManyTest.php#L154

I will think this through.

[driesvints]

Did https://github.com/laravel/framework/pull/29734 fix this?

[KKSzymanowski]

It doesn't appear it did. I'm still getting the same output when on Laravel ^6.0 and dev-master 5558a11.

[royduin]

This is also an issue with saving on the pivot model (running Laravel 6.8.0):

static::saving(function ($model) {
    Log::debug($model->getAttributes());
    Log::debug($model->getOriginal());
    Log::debug($model->getDirty());
});

The result:

array (
  'product_id' => 6671,
  'size_id' => 17,
  'cost' => '17.78',
  'backorder' => '0',
  'ean' => '1234',
  'barcodes' => '["12345"]',
)  
array (
  'product_id' => 6671,
  'size_id' => 17,
)  
array (
  'cost' => '17.78',
  'backorder' => '0',
  'ean' => '1234',
  'barcodes' => '["12345"]',
)

My use case: sizes are linked to products. On the pivot table there are barcodes; if a barcode changes or a new size is linked I need to push it to an external service. Because the pivot model is always dirty it's always pushing to the external service.

[taylorotwell]

@KKSzymanowski I guess I don't see the bug here. In the created hook the InvoiceProduct was just created and shouldn't have any dirty attributes. The isDirty method returns false as I would expect.

[taylorotwell]

I don't see any bug here with @royduin's situation either. If I do this:

Count is dirty as expected. If I don't set any attributes nothing is dirty.

[taylorotwell]

OK, actually I see what you all are talking about now. So, it works for me if I change the hard-coded true on setRawAttributes to just be the value of $exists, which I think makes more sense. If the model doesn't exist, we don't want to sync the originals. If we are creating a new model that does exist, we do want to sync original.

No existing tests break with this change.

[KKSzymanowski]

@KKSzymanowski I guess I don't see the bug here. In the created hook the InvoiceProduct was just created and shouldn't have any dirty attributes. The isDirty method returns false as I would expect.

What I mean is I think there is inconsistency in the behaviour of isDirty(), getDirty() and getOriginal() between pivots and models.
In the created hook isDirty() returns true for models but false for pivots.
Similarly for models getDirty() returns all attributes with which the model has been created plus the auto-increment id but an empty array for pivots.

[KKSzymanowski]

For some background behind this issue: I sometimes use these created, updated, deleted etc. hooks for simple activity log. In the hook I compare the getDirty and getOriginal arrays to see what has been changed and I save that difference along with the current user id in the database. This works fine for models but does not work for pivots.

[taylorotwell]

See my last comment for possible solution we could implement.

[KKSzymanowski]

Sorry, I thought your comment was directed to royduin.
I have tested your change and it fixes my issue. The framework tests also appear to work with after change. Do you want me to create a PR with this change or will you add it yourself?

[taylorotwell]

I can add it.

[royduin]

Thanks for looking into this @taylorotwell. You see the bug when you're using sync(). This is the code currently in the project:

// A sync should be enough but the pivot model events do not
// work properly, so we've build the sync functionality by
// ourself. This way we can fire events where wanted.
// See: https://github.com/laravel/framework/issues/28690#issuecomment-567432929

// $model->$attribute()->sync(
//     !$request->$attribute ? [] : $syncData
// );

$pivots = ProductSizePivot::where('product_id', $model->id)->get();
foreach ($pivots as $pivot) {
    if (isset($syncData[$pivot->size_id])) {
        if ($pivot->fill($syncData[$pivot->size_id])->isDirty()) {
            event(new ProductSaving($pivot));
            $model->$attribute()->updateExistingPivot($pivot->size_id, $syncData[$pivot->size_id]);
        }
        unset($syncData[$pivot->size_id]);
    } else {
        $model->$attribute()->detach($pivot->size_id);
    }
}

foreach ($syncData as $sizeId => $sizeData) {
    $pivot = (new ProductSizePivot)->fill(array_merge([
        'product_id' => $model->id,
        'size_id' => $sizeId,
    ], $sizeData));
    event(new ProductSaving($pivot));
    $model->$attribute()->attach($sizeId, $sizeData);
}

And in the listener on ProductSaving I'm checking for dirty attributes:

// Only new and certain dirty attributes.
if ($model->exists && !$model->isDirty(['ean', 'barcodes'])) {
    return;
}

Note: I'm not using SerializesModels on the ProductSaving event.

[taylorotwell]

Fixed in next 6.x / 7.x patch release.

[KKSzymanowski]

Thanks