Make it possible to addSql() that is executed as a statement

[mpdude]

Q	A
Type	improvement
BC Break	no
Fixed issues	fixes #1325

Summary

When the DBAL connection uses mysqli as the driver, prepared statements are sent to the MySQL server through a dedicated protocol mechanism. For example, CREATE TRIGGER statements are not possible in this case.

To use SQL statements like CREATE TRIGGER, the DbalExecutor may not (at least in the case of the mysqli driver) use Connection::executeQuery(), but has to call Connection::executeStatement(). See #1325 for more details.

This PR adds a new executeAsStatement parameter to \Doctrine\Migrations\AbstractMigration::addSql(), which defaults to false (current behaviour).

By setting it to true, a migration can pass the information to the DbalExecutor that the statement must be executed with executeStatement() instead of executeQuery().

[stof]

Ideally, most things should be executed as statements, but using a different default would be a BC break.

[greg0ire]

I think boolean flags are a code smell. Let's add a new method called addSqlStatement() instead?

[mpdude]

You mean, like a static constructor method ob Query?

[greg0ire]

No, I mean like a new method next to addSql, called addSqlStatement.

protected function addSqlStatement(
        string $sql,
        array $params = [],
        array $types = [],
): void {
    $this->plannedSql[] = new Query($sql, $params, $types, true);
}

[greg0ire]

This implies that this is an SQL query, but for some reason we want to execute it as a statement. Apparently, it's the opposite: https://stackoverflow.com/questions/4735856/difference-between-a-statement-and-a-query-in-sql

I would create a parent class called Statement to model this better, and then use instanceof.

[stof]

Well, the hard part being that even though queries are statements in the understanding of SQL in this SO answer, this is not entirely true for the DBAL API.
If you use executeStatement to execute a statement that returns a record set (i.e. to execute a query), some drivers will fail due to receiving a non expected record set. For instance, PDO only supports having 1 buffer at a time for record sets, so you must consume or close the result of a query before executing another query: #888 (comment)

[stof]

Also, DBAL (and also the ORM) seem to use Query as the more generic name (see QueryBuilder as name)

[greg0ire]

Still, it feels weird to leak a DBAL implementation detail in the public API of this value object, or even for that value object to have knowledge about it.

According to the SO answer, queries are statements, and not the other way around. According to DBAL, queries and statements are different things (so would be best modeled as 2 classes extending a common one I suppose?).

Also, DBAL (and also the ORM) seem to use Query as the more generic name (see QueryBuilder as name)

I don't think Doctrine\ORM\Query and Doctrine\ORM\Query\AST\*Statement can really be compared if that is what you are referring to, and how the ORM treats one as more generic as the other.

As for QueryBuilder… I guess it's badly named (although it's a mistake I could have made myself before today)? Other parts of the DBAL don't seem to treat query as the more generic name (otherwise Doctrine\DBAL\Statement would not have that name). Anyway, if there is a naming/modeling mistake in other packages, I don't see a huge benefit in mimicking it for the sake of consistency. It's IMO fine for doctrine/migrations to have its own model of statements and queries, to fulfill its own purposes, regardless of whether it's using DBAL or another API.

I suggest we model the SQL standard instead of mimicking the DBAL API, that way the Query class is not modeled after a lower-level API, but against a higher-level, common abstraction that the DBAL also knows about: SQL.

[greg0ire]

Looking at the code, it's hard to tell why this is needed at all. Please create a document under docs/en/explanation explaining this WTF. TL;DR: we need to make the distinction because there isn't an API that allows us to send statements without caring whether they are queries, and a buffer needs to be consumed, or whether they are just statements and we need to avoid preparing the statement. It might also be worth opening an issue on doctrine/dbal, because this issue you are facing seems to happen only because Mysqli\Connection uses prepare, unlike connections for other drivers.

[greg0ire]

Regarding the DBAL issue, it seems that the implementation uses prepare() since the beginning (12 years ago): doctrine/dbal#72

I don't know if that has been challenged since then, but maybe it should, and maybe this should be closed in favor of a DBAL PR calling mysqli::query() (which does not prepare anything, and can be used to execute statements I suppose).

[derrabus]

This is a MySQLi oddity and we should try to solve it in the DBAL.

I don't know if that has been challenged since then, but maybe it should, and maybe this should be closed in favor of a DBAL PR calling mysqli::query() (which does not prepare anything, and can be used to execute statements I suppose).

Yes. The problem with that is that we would basically need a second Result implementation for MySQLi. mysqli::query() returns a mysqli_result instance while our Result class operates on an executed mysqli_statement instance. Yay, MySQLi.

[stof]

@greg0ire this is not specific to mysqli. PDO also has this behavior (see the link I gave in the discussion)

[greg0ire]

@stof #1325 is specific to MySQLi. The issue you linked (#888 (comment)) is the opposite one: it's about needing to call executeQuery(), when using REPAIR. I believe that issue is solved right now, BTW.

[mpdude]

Feels like I stirred up a hornets' nest. I don't think I can follow your discussion, please advise what to do :-)

[derrabus]

please advise what to do

Fix this method so it does not prepare a statement.

https://github.com/doctrine/dbal/blob/63646ffd71d1676d2f747f871be31b7e921c7864/src/Driver/Mysqli/Connection.php#L68-L71

See my comment above on why this is more challenging that it might look at first glance.