SoftDelete::runSoftDelete does not take into account overridden Model::setKeysForSaveQuery

[elnur-ibr]

• Laravel Version: 5.8.4
• PHP Version: 7.2.4
• Database Driver & Version: MySql

Description:

SoftDelete::runSoftDelete does not take into account overridden Model::setKeysForSaveQuery method.
I use this for multi-tenancy purposes. Or for use composite primary keys.

Model:

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;

class Shop extends Model
{
    use SoftDeletes;

    public $primaryKey= 'shop_id';

    protected function setKeysForSaveQuery(Builder $query) {
        $query
            ->where('user_id', '=', $this->getAttribute('user_id'))
            ->where('shop_id', '=', $this->getAttribute('shop_id'));

        return $query;
    }
}

When running below code:

$shop = Shop::find(1);
$shop->delete();

Expected query:
UPDATE shop SET deleted_at = ?, Shop.updated_at = ? WHERE shop_id = ? AND user_id = ?

Actual query:
UPDATE shop SET deleted_at = ?, SHOP.updated_at = ? WHERE shop_id = ?

Solution is to replace below line

<?php

namespace Illuminate\Database\Eloquent;

trait SoftDeletes
{
     ...
     protected function runSoftDelete()
    {
        $query = $this->newModelQuery()->where($this->getKeyName(), $this->getKey());
        ...
     }
     ...
}

To

$query = $this->setKeysForSaveQuery($this->newModelQuery());

[driesvints]

Why would update Shop be expected here?

[elnur-ibr]

@driesvints it was mistype. Corrected.

[driesvints]

Heh, I'm not sure that setKeysForSaveQuery is meant to be used like this. Can't you just overwrite the runSoftDelete method if you want?

[elnur-ibr]

Yes you can overwrite runSoftDelete but I think if I am overwrite Model::setKeysForSaveQuery so SoftDelete::runSoftDelete should take it into account.

Please see below setKeysForSaveQuery and runSoftDelete methods below. The where part practically same.

protected function setKeysForSaveQuery(Builder $query)
    {
        $query->where($this->getKeyName(), '=', $this->getKeyForSaveQuery());

        return $query;
    }

protected function runSoftDelete()
    {
        $query = $this->newModelQuery()->where($this->getKeyName(), $this->getKey());
        ...
     }
     ...

[driesvints]

The question is if this has side effects. @staudenmeir do you maybe see any from the changes above?

[staudenmeir]

I agree that this should be changed. We should also use setKeysForSaveQuery() in SoftDeletes::performDeleteOnModel() to match Model::performDeleteOnModel().

An example where the current behavior is incorrect:

$user = User::find(1);
$user->id = 2;
$user->delete();

This works without SoftDeletes but not with it.

[elnur-ibr]

So should I create new pull request ? And update both SoftDeletes::performDeleteOnModel() and SoftDelete::runSoftDelete ?

[staudenmeir]

Yes, please create a PR.

[driesvints]

@staudenmeir thanks for verifying! :)

[driesvints]

PR was merged.