Skip to content

Commit

Permalink
Implement platform-aware schema comparison
Browse files Browse the repository at this point in the history
This should fix many bugs because it eliminates a lot of false positive
and false negative diffs.
  • Loading branch information
greg0ire committed Jan 12, 2022
1 parent bdd1789 commit 9e44edc
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 21 deletions.
17 changes: 15 additions & 2 deletions lib/Doctrine/Migrations/Generator/DiffGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Doctrine\Migrations\Generator\Exception\NoChangesDetected;
use Doctrine\Migrations\Provider\SchemaProvider;

use function method_exists;
use function preg_match;
use function strpos;
use function substr;
Expand Down Expand Up @@ -95,15 +96,27 @@ static function ($assetName) use ($filterExpression) {

$toSchema = $this->createToSchema();

if (method_exists($this->schemaManager, 'createComparator')) {
$comparator = $this->schemaManager->createComparator();
}

$upSql = isset($comparator) ?
$comparator->compareSchemas($fromSchema, $toSchema)->toSql($this->platform) :
$fromSchema->getMigrateToSql($toSchema, $this->platform);

$up = $this->migrationSqlGenerator->generate(
$fromSchema->getMigrateToSql($toSchema, $this->platform),
$upSql,
$formatted,
$lineLength,
$checkDbPlatform
);

$downSql = isset($comparator) ?
$comparator->compareSchemas($toSchema, $fromSchema)->toSql($this->platform) :
$fromSchema->getMigrateFromSql($toSchema, $this->platform);

$down = $this->migrationSqlGenerator->generate(
$fromSchema->getMigrateFromSql($toSchema, $this->platform),
$downSql,
$formatted,
$lineLength,
$checkDbPlatform
Expand Down
11 changes: 10 additions & 1 deletion lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
use Doctrine\DBAL\Schema\AbstractSchemaManager;
use Doctrine\DBAL\Schema\Schema;

use function method_exists;

/**
* The SchemaDiffProvider class is responsible for providing a Doctrine\DBAL\Schema\Schema instance that
* represents the current state of your database. A clone of this Schema instance is passed to each of your migrations
Expand Down Expand Up @@ -48,6 +50,13 @@ public function createToSchema(Schema $fromSchema): Schema
/** @return string[] */
public function getSqlDiffToMigrate(Schema $fromSchema, Schema $toSchema): array
{
return $fromSchema->getMigrateToSql($toSchema, $this->platform);
if (! method_exists($this->schemaManager, 'createComparator')) {
return $fromSchema->getMigrateToSql($toSchema, $this->platform);
}

return $this->schemaManager->createComparator()->compare(
$fromSchema,
$toSchema
)->toSql($this->platform);
}
}
99 changes: 81 additions & 18 deletions tests/Doctrine/Migrations/Tests/Generator/DiffGeneratorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
use Doctrine\DBAL\Configuration as DBALConfiguration;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Schema\AbstractSchemaManager;
use Doctrine\DBAL\Schema\Comparator;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\DBAL\Schema\SchemaDiff;
use Doctrine\DBAL\Schema\Table;
use Doctrine\Migrations\Generator\DiffGenerator;
use Doctrine\Migrations\Generator\Generator;
Expand All @@ -16,6 +18,8 @@
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;

use function method_exists;

class DiffGeneratorTest extends TestCase
{
/** @var DBALConfiguration|MockObject */
Expand Down Expand Up @@ -92,15 +96,42 @@ static function ($name): bool {
->method('dropTable')
->will(self::onConsecutiveCalls('schema.table_name2', 'schema.table_name3'));

$fromSchema->expects(self::once())
->method('getMigrateToSql')
->with($toSchema, $this->platform)
->willReturn(['UPDATE table SET value = 2']);
if (method_exists($this->schemaManager, 'createComparator')) {
$schemaDiff = $this->createStub(SchemaDiff::class);
$schemaDiff->method('toSql')->willReturn(self::onConsecutiveCalls(
['UPDATE table SET value = 2'],
['UPDATE table SET value = 1']
));

// regular mocks cannot be used here, because the method is static
$comparator = new class extends Comparator {
/** @var SchemaDiff */
public static $schemaDiff;

public static function compareSchemas(
Schema $fromSchema,
Schema $toSchema
): SchemaDiff {
return self::$schemaDiff;
}
};

$comparator::$schemaDiff = $schemaDiff;

$fromSchema->expects(self::once())
->method('getMigrateFromSql')
->with($toSchema, $this->platform)
->willReturn(['UPDATE table SET value = 1']);
$this->schemaManager->expects(self::once())
->method('createComparator')
->willReturn($comparator);
} else {
$fromSchema->expects(self::once())
->method('getMigrateToSql')
->with($toSchema, $this->platform)
->willReturn(['UPDATE table SET value = 2']);

$fromSchema->expects(self::once())
->method('getMigrateFromSql')
->with($toSchema, $this->platform)
->willReturn(['UPDATE table SET value = 1']);
}

$this->migrationSqlGenerator->expects(self::exactly(2))
->method('generate')
Expand All @@ -115,7 +146,12 @@ static function ($name): bool {
->with('1234', 'test1', 'test2')
->willReturn('path');

self::assertSame('path', $this->migrationDiffGenerator->generate('1234', '/table_name1/', true, 80));
self::assertSame('path', $this->migrationDiffGenerator->generate(
'1234',
'/table_name1/',
true,
80
));
}

public function testGenerateFromEmptySchema(): void
Expand Down Expand Up @@ -147,15 +183,42 @@ public function testGenerateFromEmptySchema(): void
$toSchema->expects(self::never())
->method('dropTable');

$emptySchema->expects(self::once())
->method('getMigrateToSql')
->with($toSchema, $this->platform)
->willReturn(['CREATE TABLE table_name']);

$emptySchema->expects(self::once())
->method('getMigrateFromSql')
->with($toSchema, $this->platform)
->willReturn(['DROP TABLE table_name']);
if (method_exists($this->schemaManager, 'createComparator')) {
$schemaDiff = $this->createStub(SchemaDiff::class);
$schemaDiff->method('toSql')->willReturn(self::onConsecutiveCalls(
['CREATE TABLE table_name'],
['DROP TABLE table_name']
));

// regular mocks cannot be used here, because the method is static
$comparator = new class extends Comparator {
/** @var SchemaDiff */
public static $schemaDiff;

public static function compareSchemas(
Schema $fromSchema,
Schema $toSchema
): SchemaDiff {
return self::$schemaDiff;
}
};

$comparator::$schemaDiff = $schemaDiff;

$this->schemaManager->expects(self::once())
->method('createComparator')
->willReturn($comparator);
} else {
$emptySchema->expects(self::once())
->method('getMigrateToSql')
->with($toSchema, $this->platform)
->willReturn(['CREATE TABLE table_name']);

$emptySchema->expects(self::once())
->method('getMigrateFromSql')
->with($toSchema, $this->platform)
->willReturn(['DROP TABLE table_name']);
}

$this->migrationSqlGenerator->expects(self::exactly(2))
->method('generate')
Expand Down

0 comments on commit 9e44edc

Please sign in to comment.