Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.8] Fixed: SoftDelete::runSoftDelete and SoftDelete::performDeleteOnModel did not take into account overwrite Model::setKeysForSaveQuery #28081

Merged
merged 3 commits into from
Apr 1, 2019

Conversation

elnur-ibr
Copy link
Contributor

Description:

SoftDelete::runSoftDelete and SoftDelete::performDeleteOnModel did not take into account overwrite Model::setKeysForSaveQuery.
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 lines

<?php

namespace Illuminate\Database\Eloquent;

trait SoftDeletes
{
     ...
     protected function performDeleteOnModel()
    {
        ...
            return $this->newModelQuery()->where($this->getKeyName(), $this->getKey())->forceDelete();
        }

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

To
return $this->setKeysForSaveQuery($this->newModelQuery())->forceDelete();
$query = $this->setKeysForSaveQuery($this->newModelQuery());

… did not take into account overwrite Model::setKeysForSaveQuery
@driesvints driesvints changed the title Fixed: SoftDelete::runSoftDelete and SoftDelete::performDeleteOnModel did not take into account overwrite Model::setKeysForSaveQuery [5.8] Fixed: SoftDelete::runSoftDelete and SoftDelete::performDeleteOnModel did not take into account overwrite Model::setKeysForSaveQuery Apr 1, 2019
@driesvints
Copy link
Member

@ElnurIbrahimzade this seems to break the tests. Can you also create a regression test for the problem at hand?

@elnur-ibr
Copy link
Contributor Author

Added DatabaseSoftDeletingTraitStub::setKeysForSaveQuery method to DatabaseSoftDeletingTraitTest

That fixed problem.

@taylorotwell taylorotwell merged commit 6391166 into laravel:5.8 Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants