laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.52k stars 11.02k forks source link

Using increment() on custom pivot attribute does not save the incrementation to the database. #34553

Closed Smoggert closed 4 years ago

Smoggert commented 4 years ago

Description:

$custom_pivot->increment('columname', value); does not update database with incremented value

Steps To Reproduce:

The product model:

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Database\Eloquent\Model;

class Product extends Model
{
    use HasFactory;

    /**
     * The attributes that are mass assignable.
     *
     * @var array
     */
    protected $fillable = [
        'name',
        'description',
        'price',
        'stock',
        'width',
        'height',
        'available'
    ];
    public function users()
    {
        return $this->belongsToMany('App\Models\User', 'App\Models\Shoppingcartorder')->withPivot('amount')->withTimestamps();
    }
}

The User Model: (jetstream user model)

<?php

namespace App\Models;

use Illuminate\Contracts\Auth\MustVerifyEmail;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Foundation\Auth\User as Authenticatable;
use Illuminate\Notifications\Notifiable;
use Laravel\Fortify\TwoFactorAuthenticatable;
use Laravel\Jetstream\HasProfilePhoto;
use Laravel\Sanctum\HasApiTokens;

class User extends Authenticatable
{
    use HasApiTokens;
    use HasFactory;
    use HasProfilePhoto;
    use Notifiable;
    use TwoFactorAuthenticatable;

    /**
     * The attributes that are mass assignable.
     *
     * @var array
     */
    protected $fillable = [
        'name',
        'email',
        'password',
    ];

    /**
     * The attributes that should be hidden for arrays.
     *
     * @var array
     */
    protected $hidden = [
        'password',
        'remember_token',
        'two_factor_recovery_codes',
        'two_factor_secret',
    ];

    /**
     * The attributes that should be cast to native types.
     *
     * @var array
     */
    protected $casts = [
        'email_verified_at' => 'datetime',
    ];

    protected $with = [
        'products',
    ];

    /**
     * The accessors to append to the model's array form.
     *
     * @var array
     */
    protected $appends = [
        'profile_photo_url',
    ];

    public function products()
    {
        return $this->belongsToMany('App\Models\Product','App\Models\Shoppingcartorder')->withPivot('amount')->withTimestamps();
    }
}

The pivot

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Relations\Pivot;

class Shoppingcartorder extends Pivot
{
       /**
     * The attributes that are mass assignable.
     *
     * @var array
     */
    protected $fillable = [
        'product_id',
        'user_id',
        'amount',
    ];
}

The function I was trying to use in my pivot's controller

public function incrementAmount(Product $product, Int $amount = 0)
    {
        $pivot = $product->users()->where('user_id', Auth::id())->first()->pivot;
        $pivot->increment('amount', $amount);
        //$pivot->save();
    }

Either with or without save() This function DOES increment the pivot model. dd() shows amount as X + $amount. But the database is unaffected.

So I rewrote the incrementing function myself, and this works. I checked out the incrementing() function in Model to see why the behavior is different but it does a bunch of abstractions and calls that just confuse me.

public function incrementAmount(Product $product, Int $amount = 0)
    {
        $pivot = $product->users()->where('user_id', Auth::id())->first()->pivot;
        $pivot->amount = $pivot->amount + $amount;
        $pivot->save();
    }

If this is intended behavior, sorry for the time wasted. This is incredibly confusing.

taylorotwell commented 4 years ago

Does it work if you assign an auto-incrementing primary key to your pivot model table?

Smoggert commented 4 years ago
<?php

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

class Shoppingcartorder extends Migration
{
 /**
     * Run the migrations.
     *
     * @return void
     */
    public function up()
    {
        Schema::create('shoppingcartorder', function (Blueprint $table) {
            $table->timestamps();
            $table->id();
            $table->foreignId('product_id');
            $table->foreignId('user_id');
            $table->integer('amount');
        });
    }

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

Adjusted my migration to include an id()

        /**
     * Indicates if the IDs are auto-incrementing.
     *
     * @var bool
     */
    public $incrementing = true;

Added this to the pivot model.

Still increment() does not function.

Smoggert commented 4 years ago

When I dd the resulting pivot.

    public function incrementAmount(Product $product, Int $amount = 0)
    {
        $pivot = $product->users()->where('user_id', Auth::id())->first()->pivot;
        $pivot->increment('amount', $amount);
        dd($pivot);
    }

The DD data that is different between the 2 functions: <<< increment('column', Int amount)

  #original: array:5 [▼
    "product_id" => "7"
    "user_id" => "2"
    "amount" => 2  
    "created_at" => "2020-09-27 22:02:47"
    "updated_at" => "2020-09-27 22:02:47"
  ]
  #changes: array:1 [▼
    "amount" => 2
  ]

<<< regular selfwritten function

#original: array:5 [▼
    "product_id" => "7"
    "user_id" => "2"
    "amount" => 1    
    "created_at" => "2020-09-27 22:02:47"
    "updated_at" => "2020-09-27 22:02:47"
  ]

image

Smoggert commented 4 years ago

The culprit is the custom saving function written and returned in Illuminate\Database\Eloquent\Model::class: protected function incrementOrDecrement($column, $amount, $extra, $method) L:632

return tap($query->where(
            $this->getKeyName(), $this->getKey()
        )->{$method}($column, $amount, $extra), function () use ($column) {
            $this->syncChanges();

            $this->fireModelEvent('updated', false);

            $this->syncOriginalAttribute($column);
}

Specifically $this->syncOriginalAttribute($column); I'm unaware why this creates issues with custom pivot columns (it might have issues with other columns too, cba to check)

This can be alleviated by calling the inherent save function of the model ! (after all that's what it's there for !)

        return tap($query->where(
            $this->getKeyName(), $this->getKey()
        )->{$method}($column, $amount, $extra), function () use ($column) {
           $this->save();
        });

Whether or not this breaks some other code that this dodgy custom save is based on is beyond me. But this works perfectly for me.

taylorotwell commented 4 years ago

I see the problem. Fixed in next patch.