migrations

migrations copied to clipboard

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().

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

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.

Regarding the DBAL issue, it seems that the implementation uses prepare() since the beginning (12 years ago): https://github.com/doctrine/dbal/pull/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).

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.

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

@stof #1325 is specific to MySQLi. The issue you linked (https://github.com/doctrine/migrations/pull/888#issuecomment-722318249) is the opposite one: it's about needing to call executeQuery(), when using REPAIR. I believe that issue is solved right now, BTW.

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

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.