Skip to content

Conversation

sancherie
Copy link
Contributor

Hi everybody ✋

This PR fixes the behavior of SoftDeletes::performDeleteOnModel() method in case of forceDeleting.

The Current Issue

Currently, when a model with the SoftDeletes trait is force deleted, the model property $model->exists is set to false before the database is called for deletion.

/** @in Illuminate\Database\Eloquent::performDeleteOnModel() */

$this->exists = false;

return $this->setKeysForSaveQuery($this->newModelQuery())->forceDelete();

This behavior is particularly annoying if an error occurs during the deletion (e.g: foreign key constraint), if we catch the Exception, the $model->exists property will be equal to false whereas the model has not been deleted.

/** @var App\Models\User $user */

try {
    $user->forceDelete(); // this throws an error
} catch (\Exception $e) {
    echo $user->exists; // false

    // fix user deletion
}

// retry will do nothing because $user->exists === false
$user->forceDelete();

The Fix

With this fix, the database deletion call is made before the property setting. This ensure that $model->exists property is set to false only when the deletion succeeded

/** @in Illuminate\Database\Eloquent::performDeleteOnModel() */

 return tap($this->setKeysForSaveQuery($this->newModelQuery())->forceDelete(), function () {
    $this->exists = false;
});

@sancherie sancherie marked this pull request as ready for review December 11, 2021 18:13
@taylorotwell taylorotwell merged commit 24fc129 into laravel:8.x Dec 13, 2021
jaulz pushed a commit to jaulz/framework that referenced this pull request Dec 14, 2021
…e only when deletion succeeded (laravel#39987)

* SoftDelete::performDeleteOnModel() sets "exists" property to false only when succeeded

* Fix lint
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.

2 participants