From 916150317dc49de59cd85843ac3698b6d2ed7afe Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Thu, 9 Sep 2021 09:44:19 -0700 Subject: [PATCH] Require the original column to be set on the column diff --- UPGRADE.md | 2 + src/Platforms/PostgreSQLPlatform.php | 36 ++------- src/Platforms/SQLServerPlatform.php | 34 +++----- src/Platforms/SqlitePlatform.php | 1 - src/Schema/ColumnDiff.php | 12 ++- src/Schema/Comparator.php | 2 +- tests/Platforms/AbstractPlatformTestCase.php | 37 ++++----- .../AbstractPostgreSQLPlatformTestCase.php | 12 ++- tests/Platforms/DB2PlatformTest.php | 2 +- tests/Platforms/OraclePlatformTest.php | 78 +++++++++++-------- tests/Schema/ColumnDiffTest.php | 3 - tests/Schema/ComparatorTest.php | 20 +++-- 12 files changed, 108 insertions(+), 131 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index cc1ccdbba62..575cd914003 100644 --- 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 index 9d96e16dd6b..fb736e473cc 100644 --- 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 index 0244cd4b15a..22ac067fca2 100644 --- 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 index ae28b5284ff..a1da8349e83 100644 --- 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 index 692a8425562..99dd7a1db2c 100644 --- a/src/Schema/ColumnDiff.php +++ b/src/Schema/ColumnDiff.php @@ -16,9 +16,9 @@ class ColumnDiff public Column $column; /** @var array */ - public array $changedProperties = []; + public array $changedProperties; - public ?Column $fromColumn = null; + public Column $fromColumn; /** * @param array $changedProperties @@ -26,8 +26,8 @@ class ColumnDiff 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 index b815befaae2..655f2a42afe 100644 --- 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 index 7614d492e77..91f84c6a35b 100644 --- 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 index 5e0eb11ee82..a82dac6993e 100644 --- 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 index d38f9006dbe..04f7692250e 100644 --- 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 index 908284a06ac..ca87dd38473 100644 --- 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 = [ diff --git a/tests/Schema/ColumnDiffTest.php b/tests/Schema/ColumnDiffTest.php index 649872ce6dd..86c8bce71a9 100644 --- a/tests/Schema/ColumnDiffTest.php +++ b/tests/Schema/ColumnDiffTest.php @@ -17,9 +17,6 @@ public function testPreservesOldColumnNameQuotation(): void $fromColumn = new Column('"foo"', Type::getType(Types::INTEGER)); $toColumn = new Column('bar', Type::getType(Types::INTEGER)); - $columnDiff = new ColumnDiff('"foo"', $toColumn, []); - self::assertTrue($columnDiff->getOldColumnName()->isQuoted()); - $columnDiff = new ColumnDiff('"foo"', $toColumn, [], $fromColumn); self::assertTrue($columnDiff->getOldColumnName()->isQuoted()); diff --git a/tests/Schema/ComparatorTest.php b/tests/Schema/ComparatorTest.php index 7dbb3ea63b6..29704f0ad74 100644 --- a/tests/Schema/ComparatorTest.php +++ b/tests/Schema/ComparatorTest.php @@ -1016,10 +1016,12 @@ public function testCompareChangedColumn(): void $tableDiff = $expected->changedTables['foo'] = new TableDiff('foo'); $tableDiff->fromTable = $tableFoo; - $columnDiff = $tableDiff->changedColumns['id'] = new ColumnDiff('id', $table->getColumn('id')); - - $columnDiff->fromColumn = $tableFoo->getColumn('id'); - $columnDiff->changedProperties = ['type']; + $tableDiff->changedColumns['id'] = new ColumnDiff( + 'id', + $table->getColumn('id'), + ['type'], + $tableFoo->getColumn('id') + ); self::assertEquals($expected, $this->comparator->compareSchemas($oldSchema, $newSchema)); } @@ -1041,10 +1043,12 @@ public function testCompareChangedBinaryColumn(): void $tableDiff = $expected->changedTables['foo'] = new TableDiff('foo'); $tableDiff->fromTable = $tableFoo; - $columnDiff = $tableDiff->changedColumns['id'] = new ColumnDiff('id', $table->getColumn('id')); - - $columnDiff->fromColumn = $tableFoo->getColumn('id'); - $columnDiff->changedProperties = ['length', 'fixed']; + $tableDiff->changedColumns['id'] = new ColumnDiff( + 'id', + $table->getColumn('id'), + ['length', 'fixed'], + $tableFoo->getColumn('id') + ); self::assertEquals($expected, $this->comparator->compareSchemas($oldSchema, $newSchema)); }