DISCUSS: Model->forceDelete() clears your table if Model doesn't have softDeletingTrait.

[trip-somers]

Obviously, you should not be calling forceDelete() to delete a row in the table unless your model has the softDeletingTrait, but the unintended consequences are obviously quite terrible if you do it anyway.

With well-tested code, this issue gets caught early but here's an illustration of the problem.

Illustration: I have a loaded model -- $object -- with softDeletingTrait, I can call $object->forceDelete() to permanently remove its persistence, in this case, from my database. If that object does NOT have softDeletingTrait, the exact same method call -- $object->forceDelete() -- will clear my entire table because it runs through Query\Builder and results in a "delete from {table}" query with no where clause.

This has the same effect of running $object->forceDelete() without first loading a model, but given that the two examples above both assume a loaded object, should the same semantic method call result in two completely different behaviors?

Again, with well-tested code, the odds of this causing a problem in production are slim, but since the following just happened to me, I would expect others have accidentally cleared a development table or two by mistake.

Looking forward to thoughts from others.

[garygreen]

Oh wow, +1 to this. Definitely should throw some kind of logic exception if attempting to forceDelete() where the soft deleting trait hasn't been used.

[maktouch]

Wow. I found this thread way too late.

I upgraded 4.1 to 4.2 this morning. I carefully read the changelogs, looked okay to me.

My users table used to use SoftDelete, but at some point I changed it, but the method to delete the user still used forceDelete.

1 user deleted his account, and it wiped the whole users table. In production. FML.

This is not acceptable. It's an undocumented change of behavior. ->forceDelete() should reference delete() in this case, or throw an exception, but delete the whole table? LOL.

To test:

$u = ModelWithNoSoftDelete::find(1);
$u->forceDelete();

[maktouch]

By the way, @theTrip73,

Again, with well-tested code, the odds of this causing a problem in production are slim, but since the following just happened to me,

I disagree. My unit test didn't catch it. I kinda trusted Eloquent so I don't test the query it outputs. My unit tests only tested that the model did receive ->forceDelete, not what ->forceDelete actually did.

This should be a hotfix. ->forceDelete doesn't really makes sense for a model that doesn't use softdeletes, so it should throw an exception. I'm still in disbelief that the considered option was to delete the whole table.

[tylermenezes]

Can anyone actually figure out what change caused this issue? I'm not sure if I'm missing something in the chain of metaprogramming, but I can't even find where forceDelete or anything that looks like it's touching it is being defined outside of the SoftDeleting trait.

[maktouch]

/**
 * Run the default delete function on the builder.
 *
 * @return mixed
 */
public function forceDelete()
{
    return $this->query->delete();
}

In Illuminate/Database/Eloquent/Builder. IMHO, it should be removed since it doesn't make sense in a non soft-delete scenario.

[tylermenezes]

Ah, and it's getting there by the magic getter's ability to return Builders!

I can't imagine this was intentional, right!?

[trip-somers]

Correct. The Query\Builder::forceDelete() method is called magically by the Eloquent model when it doesn't exist on the model itself (as it does when it has softDeletingTrait).

Using the query builder like this is part of what makes Eloquent great, but I don't know to what extent people have intentionally used the query builder's forceDelete() method (or in which release it was first added, etc.).

Simply removing that method from the query builder would be the easiest solution, but that presents a breaking change to anyone intentionally using it. I'm not sure there is a way to avoid introducing a breaking change here.

[dwightwatson]

Ouch. forceDelete() should definitely not delete an entire table if called on a single model, regardless of whether or not it has the SoftDeletingTrait.

[maktouch]

Model::delete() should delete all the models, because Model::where('id', '>', 10)->delete() should delete all id higher than 10. That's part of the ORM.

What's not okay is when you specifically find a model, ignores it, and instead deletes the whole table.

I just finished putting out all fires, turns out I setup foreign keys on some tables and it cascaded the deletes. Fun times :-)

[taylorotwell]

Fixed in 4.2.11.