Require the original column to be set on the column diff

doctrine · Sep 11, 2021 · 9161503 · 9161503
1 parent 2cb1c8c
commit 9161503
Show file tree

Hide file tree

Showing 12 changed files with 108 additions and 131 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -8,6 +8,8 @@ awareness about deprecated code.
 
 # Upgrade to 4.0
 
+## BC BREAK: The `$fromColumn` parameter of the `ColumnDiff` constructor made required
+
 ## BC BREAK: Changes in the return value of `Table::getColumns()`
 
 1. The columns are returned as a list, not as an associative array.

diff --git a/src/Platforms/PostgreSQLPlatform.php b/src/Platforms/PostgreSQLPlatform.php
@@ -449,12 +449,9 @@ public function getAlterTableSQL(TableDiff $diff): array
             }
 
             $newComment = $this->getColumnComment($column);
-            $oldComment = $this->getOldColumnComment($columnDiff);
+            $oldComment = $this->getColumnComment($columnDiff->fromColumn);
 
-            if (
-                $columnDiff->hasChanged('comment')
-                || ($columnDiff->fromColumn !== null && $oldComment !== $newComment)
-            ) {
+            if ($columnDiff->hasChanged('comment') || $oldComment !== $newComment) {
                 $commentsSQL[] = $this->getCommentOnColumnSQL(
                     $diff->getName($this)->getQuotedName($this),
                     $column->getQuotedName($this),
@@ -528,23 +525,13 @@ private function isUnchangedBinaryColumn(ColumnDiff $columnDiff): bool
             return false;
         }
 
-        $fromColumn = $columnDiff->fromColumn;
-
-        if ($fromColumn !== null) {
-            $fromColumnType = $fromColumn->getType();
-
-            if (! $fromColumnType instanceof BinaryType && ! $fromColumnType instanceof BlobType) {
-                return false;
-            }
-
-            return count(array_diff($columnDiff->changedProperties, ['type', 'length', 'fixed'])) === 0;
-        }
+        $fromColumnType = $columnDiff->fromColumn->getType();
 
-        if ($columnDiff->hasChanged('type')) {
+        if (! $fromColumnType instanceof BinaryType && ! $fromColumnType instanceof BlobType) {
             return false;
         }
 
-        return count(array_diff($columnDiff->changedProperties, ['length', 'fixed'])) === 0;
+        return count(array_diff($columnDiff->changedProperties, ['type', 'length', 'fixed'])) === 0;
     }
 
     /**
@@ -1051,10 +1038,6 @@ private function isSerialColumn(array $column): bool
      */
     private function typeChangeBreaksDefaultValue(ColumnDiff $columnDiff): bool
     {
-        if ($columnDiff->fromColumn === null) {
-            return $columnDiff->hasChanged('type');
-        }
-
         $oldTypeIsNumeric = $this->isNumericType($columnDiff->fromColumn->getType());
         $newTypeIsNumeric = $this->isNumericType($columnDiff->column->getType());
 
@@ -1068,15 +1051,6 @@ private function isNumericType(Type $type): bool
         return $type instanceof IntegerType || $type instanceof BigIntType;
     }
 
-    private function getOldColumnComment(ColumnDiff $columnDiff): ?string
-    {
-        if ($columnDiff->fromColumn === null) {
-            return null;
-        }
-
-        return $this->getColumnComment($columnDiff->fromColumn);
-    }
-
     public function getListTableMetadataSQL(string $table, ?string $schema = null): string
     {
         if ($schema !== null) {

diff --git a/src/Platforms/SQLServerPlatform.php b/src/Platforms/SQLServerPlatform.php
@@ -515,25 +515,15 @@ public function getAlterTableSQL(TableDiff $diff): array
             $comment    = $this->getColumnComment($column);
             $hasComment = $comment !== '';
 
-            if ($columnDiff->fromColumn instanceof Column) {
-                $fromComment    = $this->getColumnComment($columnDiff->fromColumn);
-                $hasFromComment = $fromComment !== '';
-
-                if ($hasFromComment && $hasComment && $fromComment !== $comment) {
-                    $commentsSql[] = $this->getAlterColumnCommentSQL(
-                        $diff->name,
-                        $column->getQuotedName($this),
-                        $comment
-                    );
-                } elseif ($hasFromComment && ! $hasComment) {
-                    $commentsSql[] = $this->getDropColumnCommentSQL($diff->name, $column->getQuotedName($this));
-                } elseif (! $hasFromComment && $hasComment) {
-                    $commentsSql[] = $this->getCreateColumnCommentSQL(
-                        $diff->name,
-                        $column->getQuotedName($this),
-                        $comment
-                    );
-                }
+            $fromComment    = $this->getColumnComment($columnDiff->fromColumn);
+            $hasFromComment = $fromComment !== '';
+
+            if ($hasFromComment && $hasComment && $fromComment !== $comment) {
+                $commentsSql[] = $this->getAlterColumnCommentSQL($diff->name, $column->getQuotedName($this), $comment);
+            } elseif ($hasFromComment && ! $hasComment) {
+                $commentsSql[] = $this->getDropColumnCommentSQL($diff->name, $column->getQuotedName($this));
+            } elseif (! $hasFromComment && $hasComment) {
+                $commentsSql[] = $this->getCreateColumnCommentSQL($diff->name, $column->getQuotedName($this), $comment);
             }
 
             // Do not add query part if only comment has changed.
@@ -671,12 +661,6 @@ private function getAlterTableDropDefaultConstraintClause(string $tableName, str
      */
     private function alterColumnRequiresDropDefaultConstraint(ColumnDiff $columnDiff): bool
     {
-        // We can only decide whether to drop an existing default constraint
-        // if we know the original default value.
-        if (! $columnDiff->fromColumn instanceof Column) {
-            return false;
-        }
-
         // We only need to drop an existing default constraint if we know the
         // column was defined with a default value before.
         if ($columnDiff->fromColumn->getDefault() === null) {

diff --git a/src/Platforms/SqlitePlatform.php b/src/Platforms/SqlitePlatform.php
@@ -896,7 +896,6 @@ private function getSimpleAlterTableSQL(TableDiff $diff)
         // Suppress changes on integer type autoincrement columns.
         foreach ($diff->changedColumns as $oldColumnName => $columnDiff) {
             if (
-                $columnDiff->fromColumn === null ||
                 ! $columnDiff->column->getAutoincrement() ||
                 ! $columnDiff->column->getType() instanceof Types\IntegerType
             ) {

diff --git a/src/Schema/ColumnDiff.php b/src/Schema/ColumnDiff.php
@@ -16,18 +16,18 @@ class ColumnDiff
     public Column $column;
 
     /** @var array<int, string> */
-    public array $changedProperties = [];
+    public array $changedProperties;
 
-    public ?Column $fromColumn = null;
+    public Column $fromColumn;
 
     /**
      * @param array<string> $changedProperties
      */
     public function __construct(
         string $oldColumnName,
         Column $column,
-        array $changedProperties = [],
-        ?Column $fromColumn = null
+        array $changedProperties,
+        Column $fromColumn
     ) {
         $this->oldColumnName     = $oldColumnName;
         $this->column            = $column;
@@ -42,8 +42,6 @@ public function hasChanged(string $propertyName): bool
 
     public function getOldColumnName(): Identifier
     {
-        $quote = $this->fromColumn !== null && $this->fromColumn->isQuoted();
-
-        return new Identifier($this->oldColumnName, $quote);
+        return new Identifier($this->oldColumnName, $this->fromColumn->isQuoted());
     }
 }
diff --git a/src/Schema/Comparator.php b/src/Schema/Comparator.php
@@ -224,7 +224,7 @@ public function diffTable(Table $fromTable, Table $toTable): ?TableDiff
 
             $changedProperties = $this->diffColumn($column, $toColumn);
 
-            $columnDiff = new ColumnDiff($column->getName(), $toColumn, $changedProperties);
+            $columnDiff = new ColumnDiff($column->getName(), $toColumn, $changedProperties, $column);
 
             $columnDiff->fromColumn                               = $column;
             $tableDifferences->changedColumns[$column->getName()] = $columnDiff;

diff --git a/tests/Platforms/AbstractPlatformTestCase.php b/tests/Platforms/AbstractPlatformTestCase.php
@@ -360,7 +360,8 @@ public function testGeneratesTableAlterationSql(): void
                     'default' => 'def',
                 ]
             ),
-            ['type', 'notnull', 'default']
+            ['type', 'notnull', 'default'],
+            $table->getColumn('bar')
         );
 
         $tableDiff->changedColumns['bloo'] = new ColumnDiff(
@@ -370,7 +371,8 @@ public function testGeneratesTableAlterationSql(): void
                 Type::getType('boolean'),
                 ['default' => false]
             ),
-            ['type', 'notnull', 'default']
+            ['type', 'notnull', 'default'],
+            $table->getColumn('bloo')
         );
 
         $sql = $this->platform->getAlterTableSQL($tableDiff);
@@ -468,7 +470,9 @@ public function testGetAlterTableSqlDispatchEvent(): void
         $tableDiff->removedColumns['removed'] = new Column('removed', Type::getType('integer'), []);
         $tableDiff->changedColumns['changed'] = new ColumnDiff(
             'changed',
-            new Column('changed2', Type::getType('string'), ['length' => 255])
+            new Column('changed2', Type::getType('string'), ['length' => 255]),
+            [],
+            $table->getColumn('changed')
         );
         $tableDiff->renamedColumns['renamed'] = new Column('renamed2', Type::getType('integer'));
 
@@ -486,21 +490,16 @@ public function testCreateTableColumnComments(): void
 
     public function testAlterTableColumnComments(): void
     {
+        $foo = new Column('foo', Type::getType('string'), ['length' => 255]);
+        $bar = new Column('baz', Type::getType('string'), [
+            'length'  => 255,
+            'comment' => 'B comment',
+        ]);
+
         $tableDiff                        = new TableDiff('mytable');
         $tableDiff->addedColumns['quota'] = new Column('quota', Type::getType('integer'), ['comment' => 'A comment']);
-        $tableDiff->changedColumns['foo'] = new ColumnDiff(
-            'foo',
-            new Column('foo', Type::getType('string'), ['length' => 255]),
-            ['comment']
-        );
-        $tableDiff->changedColumns['bar'] = new ColumnDiff(
-            'bar',
-            new Column('baz', Type::getType('string'), [
-                'length'  => 255,
-                'comment' => 'B comment',
-            ]),
-            ['comment']
-        );
+        $tableDiff->changedColumns['foo'] = new ColumnDiff('foo', $foo, ['comment'], $foo);
+        $tableDiff->changedColumns['bar'] = new ColumnDiff('bar', $bar, ['comment'], $bar);
 
         self::assertEquals($this->getAlterTableColumnCommentsSQL(), $this->platform->getAlterTableSQL($tableDiff));
     }
@@ -791,7 +790,8 @@ public function testAlterTableChangeQuotedColumn(): void
                 Type::getType('string'),
                 ['length' => 255]
             ),
-            ['type']
+            ['type'],
+            $table->getColumn('select')
         );
 
         self::assertStringContainsString(
@@ -1440,7 +1440,8 @@ public function testAlterStringToFixedString(): void
                 Type::getType('string'),
                 ['fixed' => true, 'length' => 2]
             ),
-            ['fixed']
+            ['fixed'],
+            $table->getColumn('name')
         );
 
         $sql = $this->platform->getAlterTableSQL($tableDiff);

diff --git a/tests/Platforms/AbstractPostgreSQLPlatformTestCase.php b/tests/Platforms/AbstractPostgreSQLPlatformTestCase.php
@@ -509,7 +509,8 @@ public function testAlterDecimalPrecisionScale(): void
                 Type::getType('decimal'),
                 ['precision' => 16, 'scale' => 6]
             ),
-            ['precision']
+            ['precision'],
+            $table->getColumn('dfoo1')
         );
         $tableDiff->changedColumns['dloo2'] = new ColumnDiff(
             'dloo2',
@@ -518,7 +519,8 @@ public function testAlterDecimalPrecisionScale(): void
                 Type::getType('decimal'),
                 ['precision' => 10, 'scale' => 4]
             ),
-            ['scale']
+            ['scale'],
+            $table->getColumn('dfoo2')
         );
         $tableDiff->changedColumns['dloo3'] = new ColumnDiff(
             'dloo3',
@@ -527,7 +529,8 @@ public function testAlterDecimalPrecisionScale(): void
                 Type::getType('decimal'),
                 ['precision' => 10, 'scale' => 6]
             ),
-            []
+            [],
+            $table->getColumn('dfoo3')
         );
         $tableDiff->changedColumns['dloo4'] = new ColumnDiff(
             'dloo4',
@@ -536,7 +539,8 @@ public function testAlterDecimalPrecisionScale(): void
                 Type::getType('decimal'),
                 ['precision' => 16, 'scale' => 8]
             ),
-            ['precision', 'scale']
+            ['precision', 'scale'],
+            $table->getColumn('dfoo4')
         );
 
         $sql = $this->platform->getAlterTableSQL($tableDiff);

diff --git a/tests/Platforms/DB2PlatformTest.php b/tests/Platforms/DB2PlatformTest.php
@@ -661,7 +661,7 @@ public function testGeneratesAlterColumnSQL(
     ): void {
         $tableDiff                        = new TableDiff('foo');
         $tableDiff->fromTable             = new Table('foo');
-        $tableDiff->changedColumns['bar'] = new ColumnDiff('bar', $column, [$changedProperty]);
+        $tableDiff->changedColumns['bar'] = new ColumnDiff('bar', $column, [$changedProperty], $column);
 
         $expectedSQL = [];
 

diff --git a/tests/Platforms/OraclePlatformTest.php b/tests/Platforms/OraclePlatformTest.php
@@ -474,45 +474,59 @@ public function testAlterTableNotNULL(): void
     {
         $tableDiff = new TableDiff('mytable');
 
-        $tableDiff->changedColumns['foo'] = new ColumnDiff(
+        $foo = new Column(
             'foo',
-            new Column(
-                'foo',
-                Type::getType('string'),
-                [
-                    'length' => 255,
-                    'default' => 'bla',
-                    'notnull' => true,
-                ]
-            ),
-            ['type']
+            Type::getType('string'),
+            [
+                'length' => 255,
+                'default' => 'bla',
+                'notnull' => true,
+            ]
         );
 
-        $tableDiff->changedColumns['bar'] = new ColumnDiff(
-            'bar',
-            new Column(
-                'baz',
-                Type::getType('string'),
-                [
-                    'length' => 255,
-                    'default' => 'bla',
-                    'notnull' => true,
-                ]
-            ),
-            ['type', 'notnull']
+        $bar = new Column(
+            'baz',
+            Type::getType('string'),
+            [
+                'length' => 255,
+                'default' => 'bla',
+                'notnull' => true,
+            ]
         );
 
+        $baz = new Column(
+            'baz',
+            Type::getType('string'),
+            [
+                'length' => 255,
+                'default' => 'bla',
+                'notnull' => true,
+            ]
+        );
+
+        $metar = new Column(
+            'metar',
+            Type::getType('string'),
+            [
+                'length' => 2000,
+                'notnull' => false,
+            ]
+        );
+
+        $tableDiff->changedColumns['foo'] = new ColumnDiff(
+            'foo',
+            $foo,
+            ['type'],
+            $foo
+        );
+
+        $tableDiff->changedColumns['bar'] = new ColumnDiff('bar', $bar, ['type', 'notnull'], $baz);
+
         $tableDiff->changedColumns['metar'] = new ColumnDiff(
             'metar',
-            new Column(
-                'metar',
-                Type::getType('string'),
-                [
-                    'length' => 2000,
-                    'notnull' => false,
-                ]
-            ),
-            ['notnull']
+            $metar,
+            ['notnull'],
+            $metar
         );
 
         $expectedSql = [