Framework: Eloquent model terrible bug (Delete all table rows!)

[ahmadazimi]

Hi there,

Last day I found a terrible bug (I guess) in Laravel Eloquent model which cause loosing all of my data!

Imagine you have a model called Invoice, and you want to forceDelete one of its records.

If you get an instance of Invoice and save it in a variable called $invoice, then you forceDelete that instance, and somewhere later, you make an update to that instance, Eloquent execute that update on all of your model rows!!!

$invoice = Invoice::find(541);
$invoice->forceDelete();

//... somewhere later

// These lines of code cause all rows in Invoice table updated with 'status'=>'failed'
$invoice->update([
    'status' => 'failed',
]);

So, Is this really a bug?

[crynobone]

Looks more like a usage bug to me. Why would you still want to update a deleted record?

[crynobone]

When you run delete() or forceDelete() it would change the Invoice::$exists value to false, and then when you attempt to run update() it would run the update on all the records because of the Invoice::$exists value.

https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Model.php#L1478-L1485

[GrahamCampbell]

This is indeed just a bug in your code.

[theduke]

To me, this is clearly a bug in Eloquent. ( or rather a api design flaw)

It is very dangerous behaviour IMO.

I think save() / update() etc should throw when called on a force deleted model.

[duxthefux]

I'm totally with @theduke and @ahmadazimi . Just this week this behaviour caused soft deleting all our accounts (5000+) in a heavily used system.

$invoice = Account::find(541);
$invoice->update([
    'deleted' => 1
]);

This updates all records in the table.

[mfn]

@duxthefux I just tested this and I cannot reproduce this with 5.5.31. It only updated the record in question.

[ahmadazimi]

@GrahamCampbell I think this is so better to reopen this issue ;-)

[GuidoHendriks]

@ahmadazimi What version of Laravel are you using? The issue template is there for a reason.

[ahmadazimi]

@GuidoHendriks
5.3.30

[duxthefux]

@mfn
You need to use a scope for that. Check that out:

# Account.php model
   public function scopeSoftDelete(Builder $query)
    {
        return $query->update(array("c_deleted" => 1));
    }

Psy Shell v0.8.17 (PHP 7.1.11 — cli) by Justin Hileman
>>> $account = Account::find(63)
[!] Aliasing 'Account' to 'ready2order\Database\Model\Account' for this Tinker session.
=> ready2order\Database\Model\Account {#1215
     c_id: "63"
   }
>>> $account->SoftDelete()
=> 4 # <== deleted rows (in the table are exactly 4 rows)

[GuidoHendriks]

Can’t reproduce it. Based on the code I’d say it’s impossible that it does anything when calling update after it’s been deleted. It will immediately return false if it doesn’t exist.

[antonkomarev]

The best case for guys who found this possible bug will be creating repository on github where this issue will be in affect or create unit tests which cover this case.

[GuidoHendriks]

So this is an issue with soft deleting? The original issue doesn’t mention that at all.

[duxthefux]

@GuidoHendriks no in my case it has nothing to do with soft deletes...the ->SoftDelete() is just a scope*-magic-laravel-method and the model does not use the SoftDeletes-trait.

I'll make tests for that as suggested.