Dbal: 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: https://github.com/doctrine/dbal/blob/4768496328aae65232b19419fc1a75c91efbaf84/lib/Doctrine/DBAL/Query/QueryBuilder.php#L547-L559

https://github.com/doctrine/dbal/blob/4768496328aae65232b19419fc1a75c91efbaf84/lib/Doctrine/DBAL/Query/QueryBuilder.php#L636-L642

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: https://github.com/doctrine/dbal/blob/4768496328aae65232b19419fc1a75c91efbaf84/lib/Doctrine/DBAL/Query/QueryBuilder.php#L1233-L1240).

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.