Repository: doctrine/migrations (php)

Issue:
Implement platform-aware schema comparison
### Feature Request

<!-- Fill in the relevant information below to help triage your issue. -->

|    Q        |   A
|------------ | ------
| New Feature | yes
| RFC         | no
| BC Break    | no

#### Summary

Since `doctrine/dbal` 3.2.0, regular comparison is deprecated in favor of platform-aware-schema comparison: https://www.doctrine-project.org/2021/11/26/dbal-3.2.0.html

This should fix a number of false-positive and false-negative issues.

This involves changing this piece of code:

https://github.com/doctrine/migrations/blob/1967775546df997eb97057911d80184c91c92b9a/lib/Doctrine/Migrations/Generator/DiffGenerator.php#L95-L107

as well as

https://github.com/doctrine/migrations/blob/1967775546df997eb97057911d80184c91c92b9a/lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php#L46-L49

Pull request:
Implement platform-aware schema comparison

This should fix many bugs because it eliminates a lot of false positive
and false negative diffs.


Closes #1227 


Patch:
diff --git a/lib/Doctrine/Migrations/Generator/DiffGenerator.php b/lib/Doctrine/Migrations/Generator/DiffGenerator.php
index bd5c7469..77775570 100644
--- a/lib/Doctrine/Migrations/Generator/DiffGenerator.php
+++ b/lib/Doctrine/Migrations/Generator/DiffGenerator.php
@@ -12,6 +12,7 @@ use Doctrine\DBAL\Schema\Schema;
 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;
@@ -95,15 +96,27 @@ class DiffGenerator
 
         $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
diff --git a/lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php b/lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php
index ccccb17f..ba9db677 100644
--- a/lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php
+++ b/lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php
@@ -8,6 +8,8 @@ use Doctrine\DBAL\Platforms\AbstractPlatform;
 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
@@ -48,6 +50,13 @@ class DBALSchemaDiffProvider implements SchemaDiffProvider
     /** @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()->compareSchemas(
+            $fromSchema,
+            $toSchema
+        )->toSql($this->platform);
     }
 }
diff --git a/phpstan-dbal-3.neon.dist b/phpstan-dbal-3.neon.dist
index f94eca0c..bfa27b10 100644
--- a/phpstan-dbal-3.neon.dist
+++ b/phpstan-dbal-3.neon.dist
@@ -23,13 +23,27 @@ parameters:
                 - tests/Doctrine/Migrations/Tests/Tools/Console/legacy-config-dbal/cli-config.php
                 - tests/Doctrine/Migrations/Tests/Tools/Console/legacy-config-wrong/cli-config.php
 
-        # We need to implement platform-aware comparison
+        # Compatibility with DBAL 2
         -
             message: '~Call to deprecated method getMigrate(From|ToSql)~'
             paths:
                 - lib/Doctrine/Migrations/Generator/DiffGenerator.php
                 - lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php
 
+        -
+            message: "~Call to function method_exists.*'createComparator' will always evaluate to true.~"
+            paths:
+                - lib/Doctrine/Migrations/Generator/DiffGenerator.php
+                - lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php
+                - tests/Doctrine/Migrations/Tests/Generator/DiffGeneratorTest.php
+
+        # That method will no longer be static in the future
+        -
+            message: '~Dynamic call to static method Doctrine\\DBAL\\Schema\\Comparator\:\:compareSchemas\(\)\.~'
+            paths:
+                - lib/Doctrine/Migrations/Generator/DiffGenerator.php
+                - lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php
+
         # Switch to Logging\Connection after dropping compatibility with DBAL 2
         -
             message: '~deprecated.*DebugStack~'


Test patch:
diff --git a/tests/Doctrine/Migrations/Tests/Generator/DiffGeneratorTest.php b/tests/Doctrine/Migrations/Tests/Generator/DiffGeneratorTest.php
index 8fe37ca0..a614e104 100644
--- a/tests/Doctrine/Migrations/Tests/Generator/DiffGeneratorTest.php
+++ b/tests/Doctrine/Migrations/Tests/Generator/DiffGeneratorTest.php
@@ -7,7 +7,9 @@ namespace Doctrine\Migrations\Tests\Generator;
 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;
@@ -16,6 +18,8 @@ use Doctrine\Migrations\Provider\SchemaProvider;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 
+use function method_exists;
+
 class DiffGeneratorTest extends TestCase
 {
     /** @var DBALConfiguration|MockObject */
@@ -92,15 +96,42 @@ class DiffGeneratorTest extends TestCase
             ->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')
@@ -115,7 +146,12 @@ class DiffGeneratorTest extends TestCase
             ->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
@@ -147,15 +183,42 @@ class DiffGeneratorTest extends TestCase
         $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')