Deprecate and remove query builder delete with an argument, force users to use where clause

[madrussa]

Using the ->delete($id) method with an argument can be seriously dangerous. If your $id argument happens to evaluate to null you can wipe out an entire table and any foreign key constrained data. The safest pattern is obviously to use a where constraint but even having this undocumented feature is just asking for trouble. I've come across a bunch of its usage in a project I've got involved with and have had removed all instances.

With the nullsafe operator (https://wiki.php.net/rfc/nullsafe_operator) added in PHP 8, this seems like something that is likely to catch a lot of people out.

It would be good to remove this feature in future versions of Laravel and highlight how dangerous it is to currently use in the documentation. It should be a simple delete() without any arguments.

[dennisprudlo]

Wouldn't User::delete(); delete all users as well?

[madrussa]

Yes, but the problem here is that the default behaviour is expansive, whilst using the argument it is constrained. If your variable is null you get the expansive behaviour by default which is bad by design. Especially for something so destructive.

If you call User::delete(), without a variable to constrain, you are intentionally calling for the expansive behaviour. This is fine, you may want to do this.

My argument is for when you use User::delete($id) and for some reason the code before sets $id = null which could be unintentional. Most things default to null in PHP land. If we look at the delete method on query builder:

framework/src/Illuminate/Database/Query/Builder.php

Lines 3120 to 3136 in 51e9106

	public function delete($id = null)
	{
	// If an ID is passed to the method, we will set the where clause to check the
	// ID to let developers to simply and quickly remove a single row from this
	// database without manually specifying the "where" clauses on the query.
	if (! is_null($id)) {
	$this->where($this->from.'.id', '=', $id);
	}
	$this->applyBeforeQueryCallbacks();
	return $this->connection->delete(
	$this->grammar->compileDelete($this), $this->cleanBindings(
	$this->grammar->prepareBindingsForDelete($this->bindings)
	)
	);
	}

We can see it defaults the argument to null and if is null, you get the expansive behaviour. This is where the problem lies.

It is possible using the spread operator to catch these opportunities where you are passing in a constraint that could be null:

function delete(...$args) {
    if (count($args) === 1) {
        if ($args[0] === null) {
            throw new \InvalidArgumentException('Constrained delete expects a non null value');
        }
        echo 'Append where' . PHP_EOL;
    }
    
    echo 'Perform delete' . PHP_EOL;
}

echo 'Delete unconstrained' . PHP_EOL;
delete();

echo 'Delete constrained' . PHP_EOL;
delete(1);

echo 'Delete constrained unintentional nullish' . PHP_EOL;
delete(null); // Fatal Error

However there would need to be more tests around falsy or types that you wouldn't expect to resolve to a primary key.

[dennisprudlo]

I don't know the way I see it the ->delete() method should execute a delete query on the database. So if you explicitly call ->delete() I expect a delete query on my database. If I don't want to execute a delete query when the $id is null I have to check that by myself.

if (isset($id)) {
    User::delete($id);
}

I mean usually you build your query and then call delete like so:

User::where('id', $id)->delete();

Then the query would get 0 results for an id with the value null (except you have nulls among your ids) and the delete methods will delete nothing. This issue only occurs when deleting without any additional constrains like User::delete($id), right? I'd rather have people to check before executing this instead of throwing an exception. Maybe a global setting in some of the service providers could do the trick like the preventLazyLoading function (https://laravel.com/docs/8.x/eloquent-relationships#preventing-lazy-loading).

If then someone wants to prevent deleting the whole table when null is passed a simple Builder::preventNullConstrainedDelete() – or whatever it should be called – is enough.

But you should create a pull request and reference this discussion.

[madrussa]

I agree and I always use where clauses myself / defensively program. But I've seen this issue and I've seen this accidentally delete entire database tables. I'm not saying to remove the delete, just its optional argument and to force people down the route of having to add a where clause which is constrained by default. This is then safe by design.

I am referring to query builder usage and not the model...

DB::table('users')->delete($id);

As you cannot pass an argument to the delete method of the model. You must use the destroy method instead which is written to be safe and constrained by default. As per the docs https://laravel.com/docs/8.x/eloquent#deleting-models

Reading the docs for Query builder delete, it doesn't make any mention of passing in variables... probably to protect users from this scenario. As it is "undocumented" and is dangerous, it really makes sense to deprecate and remove.