Wrong "from" statement if delete query with query builder

[NikitaKharkov]

Bug Report

Invalid forming delete SQL part when I set table name in the from SQL part.

Q	A
BC Break	no
Version	v2.9.2

Summary

So, easy example:

$qb->delete()
    ->from($tableName)
    ->where('uuid IN (:uuids)')
    ->setParameter('uuids', $uuids)
    ->execute();

Current behaviour

I got next structure (dump of $sqlParts)

...
  "from" => array:1 [
    0 => array:2 [
      "table" => "some_table_name"
      "alias" => null
    ]
  ]
...

How to reproduce

Just write a query with any table

Expected behaviour

instead of this (this is correctly formed if I put $tablename in delete SQL part )

...
  "from" => array:2 [
    "table" => "some_table_name"
    "alias" => null
  ]
...

[Ocramius]

Could you write a test case showing a broken usage of this? I don't get where the bug actually is, nor what it leads to.

[nreynis]

This is a blocking issue for us, we've got to empty a table. We can't use connection->delete without identifiers, and we can't use the query builder neither. Which means we've got to opt out of DBAL and use a raw query.

[greg0ire]

@nreynis I might be wrong, but I think OP pointed out that this would work:

this is correctly formed if I put $tablename in delete SQL part

$qb->delete($tableName)
    ->from()
    ->where('uuid IN (:uuids)')
    ->setParameter('uuids', $uuids)
    ->execute();

Does that not work for you?

Here is how the code looks:

dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php

Lines 547 to 559 in 4768496

	public function delete($delete = null, $alias = null)
	{
	$this->type = self::DELETE;
	if (! $delete) {
	return $this;
	}
	return $this->add('from', [
	'table' => $delete,
	'alias' => $alias,
	]);
	}

dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php

Lines 636 to 642 in 4768496

	public function from($from, $alias = null)
	{
	return $this->add('from', [
	'table' => $from,
	'alias' => $alias,
	], true);
	}

As you can see, delete() already calls from(), so I'm not sure this is the intended use of this API. Can you point us to documentation that would say otherwise? I do concede that it results in a poor user experience (the test you created errors out with

Undefined index: table

/home/runner/work/dbal/dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php:1235

, which is this line:

dbal/lib/Doctrine/DBAL/Query/QueryBuilder.php

Lines 1233 to 1240 in 4768496

	private function getSQLForDelete()
	{
	$table = $this->sqlParts['from']['table']
	. ($this->sqlParts['from']['alias'] ? ' ' . $this->sqlParts['from']['alias'] : '');
	return 'DELETE FROM ' . $table
	. ($this->sqlParts['where'] !== null ? ' WHERE ' . ((string) $this->sqlParts['where']) : '');
	}

).

I think you should in fact write

$qb->delete($tableName)
    ->where('uuid IN (:uuids)')
    ->setParameter('uuids', $uuids)
    ->execute();

I also think an acceptable solution would be to trigger a deprecation error when somebody calls delete and then from, and to throw an exception in the next major version of the DBAL.

[nreynis]

@greg0ire Indeed using

$qb->delete($tableName)->execute();

Works for me. I just saw this syntax when I wrote the test.

I'm not sure this is the intended use of this API. Can you point us to documentation that would say otherwise? I do concede that it results in a poor user experience

I did not found any example, it just felt natural because that mimic the target SQL query (DELETE FROM 'table_name').

I also think an acceptable solution would be to trigger a deprecation error when somebody calls delete and then from, and to throw an exception in the next major version of the DBAL.

👍 you could throw a QueryException when you access/resolve an uninitialized (empty array) from property.

[nreynis]

I'm not sure deprecating is even needed. It don't work at all, you won't broke anyone code.

[greg0ire]

Good point! Would you like to contribute that?

[nreynis]

Done, but the CI seems to stumble on something unrelated. Is there a way to rerun the failing job? I'm more used to gitlab than GitHub...

[greg0ire]

I'm going to take a look.