Skip to content

Commit

Permalink
Make it possible to addSql() that is executed as a statement
Browse files Browse the repository at this point in the history
|      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()`, not `executeQuery()`.
  • Loading branch information
mpdude committed Feb 23, 2023
1 parent dc441bb commit 8b0ac22
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 9 deletions.
5 changes: 3 additions & 2 deletions lib/Doctrine/Migrations/AbstractMigration.php
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,10 @@ public function down(Schema $schema): void
protected function addSql(
string $sql,
array $params = [],
array $types = []
array $types = [],
bool $executeAsStatement = false
): void {
$this->plannedSql[] = new Query($sql, $params, $types);
$this->plannedSql[] = new Query($sql, $params, $types, $executeAsStatement);
}

/**
Expand Down
16 changes: 12 additions & 4 deletions lib/Doctrine/Migrations/Query/Query.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,22 @@ final class Query
/** @var mixed[] */
private array $types;

private bool $executeAsStatement;

/**
* @param mixed[] $parameters
* @param mixed[] $types
*/
public function __construct(string $statement, array $parameters = [], array $types = [])
public function __construct(string $statement, array $parameters = [], array $types = [], bool $executeAsStatement = false)
{
if (count($types) > count($parameters)) {
throw InvalidArguments::wrongTypesArgumentCount($statement, count($parameters), count($types));
}

$this->statement = $statement;
$this->parameters = $parameters;
$this->types = $types;
$this->statement = $statement;
$this->parameters = $parameters;
$this->types = $types;
$this->executeAsStatement = $executeAsStatement;
}

public function __toString(): string
Expand All @@ -57,4 +60,9 @@ public function getTypes(): array
{
return $this->types;
}

public function getExecuteAsStatement(): bool
{
return $this->executeAsStatement;
}
}
9 changes: 7 additions & 2 deletions lib/Doctrine/Migrations/Version/DbalExecutor.php
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,13 @@ private function executeResult(MigratorConfiguration $configuration): void
$this->outputSqlQuery($query, $configuration);

$stopwatchEvent = $this->stopwatch->start('query');
// executeQuery() must be used here because $query might return a result set, for instance REPAIR does
$this->connection->executeQuery($query->getStatement(), $query->getParameters(), $query->getTypes());

if ($query->getExecuteAsStatement()) {
$this->connection->executeStatement($query->getStatement(), $query->getParameters(), $query->getTypes());
} else {
$this->connection->executeQuery($query->getStatement(), $query->getParameters(), $query->getTypes());
}

$stopwatchEvent->stop();

if (! $configuration->getTimeAllQueries()) {
Expand Down
32 changes: 31 additions & 1 deletion tests/Doctrine/Migrations/Tests/Version/ExecutorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Doctrine\Migrations\Query\Query;
use Doctrine\Migrations\Tests\TestLogger;
use Doctrine\Migrations\Tests\Version\Fixture\EmptyTestMigration;
use Doctrine\Migrations\Tests\Version\Fixture\TestMigrationWithStatement;
use Doctrine\Migrations\Tests\Version\Fixture\VersionExecutorTestMigration;
use Doctrine\Migrations\Version\DbalExecutor;
use Doctrine\Migrations\Version\Direction;
Expand Down Expand Up @@ -94,6 +95,25 @@ public function testExecuteWithNoQueries(): void
], $this->logger->logs);
}

public function testExecuteWithStatement(): void
{
$migratorConfiguration = new MigratorConfiguration();

$migration = new TestMigrationWithStatement($this->connection, $this->logger);
$version = new Version('xx');
$plan = new MigrationPlan($version, $migration, Direction::UP);

$this->connection
->expects(self::once())
->method('executeStatement')
->with('CREATE TRIGGER', [], []);

$this->versionExecutor->execute(
$plan,
$migratorConfiguration
);
}

public function testExecuteUp(): void
{
$this->metadataStorage
Expand Down Expand Up @@ -216,6 +236,7 @@ public function testExecuteDryRun(): void
self::assertSame(Direction::UP, $result->getDirection());

yield new Query('INSERT INTO doctrine_migration_versions (version, executed_at, execution_time) VALUE (' . $result->getVersion() . ', NOW(), 0)');
yield new Query('CREATE TRIGGER something...', executeAsStatement: true);
});

$this->connection
Expand All @@ -226,6 +247,10 @@ public function testExecuteDryRun(): void
->expects(self::never())
->method('executeUpdate');

$this->connection
->expects(self::never())
->method('executeStatement');

$migratorConfiguration = (new MigratorConfiguration())
->setDryRun(true)
->setTimeAllQueries(true);
Expand All @@ -239,7 +264,7 @@ public function testExecuteDryRun(): void

$queries = $result->getSql();

self::assertCount(3, $queries);
self::assertCount(4, $queries);
self::assertSame('SELECT 1', $queries[0]->getStatement());
self::assertSame([1], $queries[0]->getParameters());
self::assertSame([3], $queries[0]->getTypes());
Expand All @@ -252,6 +277,11 @@ public function testExecuteDryRun(): void
self::assertSame([], $queries[2]->getParameters());
self::assertSame([], $queries[2]->getTypes());

self::assertSame('CREATE TRIGGER something...', $queries[3]->getStatement());
self::assertSame([], $queries[3]->getParameters());
self::assertSame([], $queries[3]->getTypes());
self::assertTrue($queries[3]->getExecuteAsStatement());

self::assertNotNull($result->getTime());
self::assertSame(State::NONE, $result->getState());
self::assertTrue($this->migration->preUpExecuted);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

declare(strict_types=1);

namespace Doctrine\Migrations\Tests\Version\Fixture;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

class TestMigrationWithStatement extends AbstractMigration
{
public function up(Schema $schema): void
{
$this->addSql('CREATE TRIGGER', executeAsStatement: true);
}
}

0 comments on commit 8b0ac22

Please sign in to comment.