From 832f51a5f7ac186998ad25802da540df37439873 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Mon, 5 Sep 2022 11:30:04 -0700 Subject: [PATCH 1/3] Reduce duplication of collation declaration code It is safe to quote collation in the default implementation of getColumnCollationDeclarationSQL() since those platforms where the case sensitivity of an identifier depends on whether it's quoted (Oracle, IBM DB2) do not support column collation anyways. --- src/Platforms/AbstractMySQLPlatform.php | 10 ---------- src/Platforms/AbstractPlatform.php | 2 +- src/Platforms/PostgreSQLPlatform.php | 10 ---------- src/Platforms/SQLServerPlatform.php | 10 ++++++++++ tests/Platforms/SqlitePlatformTest.php | 10 +--------- 5 files changed, 12 insertions(+), 30 deletions(-) diff --git a/src/Platforms/AbstractMySQLPlatform.php b/src/Platforms/AbstractMySQLPlatform.php index eef22ff85c6..1a8179b2d1b 100644 --- a/src/Platforms/AbstractMySQLPlatform.php +++ b/src/Platforms/AbstractMySQLPlatform.php @@ -1028,16 +1028,6 @@ public function getColumnCharsetDeclarationSQL($charset) return 'CHARACTER SET ' . $charset; } - /** - * {@inheritDoc} - * - * @internal The method should be only used from within the {@see AbstractPlatform} class hierarchy. - */ - public function getColumnCollationDeclarationSQL($collation) - { - return 'COLLATE ' . $this->quoteSingleIdentifier($collation); - } - /** * {@inheritDoc} * diff --git a/src/Platforms/AbstractPlatform.php b/src/Platforms/AbstractPlatform.php index 18d40e17c90..3265532fbe4 100644 --- a/src/Platforms/AbstractPlatform.php +++ b/src/Platforms/AbstractPlatform.php @@ -3321,7 +3321,7 @@ public function getColumnCharsetDeclarationSQL($charset) */ public function getColumnCollationDeclarationSQL($collation) { - return $this->supportsColumnCollation() ? 'COLLATE ' . $collation : ''; + return $this->supportsColumnCollation() ? 'COLLATE ' . $this->quoteSingleIdentifier($collation) : ''; } /** diff --git a/src/Platforms/PostgreSQLPlatform.php b/src/Platforms/PostgreSQLPlatform.php index a2e026ffb99..cbeb75b5851 100644 --- a/src/Platforms/PostgreSQLPlatform.php +++ b/src/Platforms/PostgreSQLPlatform.php @@ -1302,16 +1302,6 @@ public function supportsColumnCollation() return true; } - /** - * {@inheritdoc} - * - * @internal The method should be only used from within the {@see AbstractPlatform} class hierarchy. - */ - public function getColumnCollationDeclarationSQL($collation) - { - return 'COLLATE ' . $this->quoteSingleIdentifier($collation); - } - /** * {@inheritdoc} */ diff --git a/src/Platforms/SQLServerPlatform.php b/src/Platforms/SQLServerPlatform.php index 57abac932bd..711635b14cf 100644 --- a/src/Platforms/SQLServerPlatform.php +++ b/src/Platforms/SQLServerPlatform.php @@ -1650,6 +1650,16 @@ public function getColumnDeclarationSQL($name, array $column) return $name . ' ' . $columnDef; } + /** + * {@inheritDoc} + * + * SQL Server does not support quoting collation identifiers. + */ + public function getColumnCollationDeclarationSQL($collation) + { + return 'COLLATE ' . $collation; + } + public function columnsEqual(Column $column1, Column $column2): bool { if (! parent::columnsEqual($column1, $column2)) { diff --git a/tests/Platforms/SqlitePlatformTest.php b/tests/Platforms/SqlitePlatformTest.php index 3f392996eee..fcc7538f572 100644 --- a/tests/Platforms/SqlitePlatformTest.php +++ b/tests/Platforms/SqlitePlatformTest.php @@ -787,14 +787,6 @@ public function testSupportsColumnCollation(): void self::assertTrue($this->platform->supportsColumnCollation()); } - public function testColumnCollationDeclarationSQL(): void - { - self::assertSame( - 'COLLATE NOCASE', - $this->platform->getColumnCollationDeclarationSQL('NOCASE'), - ); - } - public function testGetCreateTableSQLWithColumnCollation(): void { $table = new Table('foo'); @@ -804,7 +796,7 @@ public function testGetCreateTableSQLWithColumnCollation(): void self::assertSame( [ 'CREATE TABLE foo (no_collation VARCHAR(255) NOT NULL, ' - . 'column_collation VARCHAR(255) NOT NULL COLLATE NOCASE)', + . 'column_collation VARCHAR(255) NOT NULL COLLATE "NOCASE")', ], $this->platform->getCreateTableSQL($table), ); From 79f94b548d6c278d3a5788005843e22df8d77453 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Mon, 5 Sep 2022 16:03:25 -0700 Subject: [PATCH 2/3] Mark ColumnDiff public properties as internal --- UPGRADE.md | 5 ++ src/Platforms/AbstractMySQLPlatform.php | 37 ++++++------ src/Platforms/DB2Platform.php | 20 +++---- src/Platforms/OraclePlatform.php | 14 ++--- src/Platforms/PostgreSQLPlatform.php | 76 +++++++++++------------ src/Platforms/SQLServerPlatform.php | 80 ++++++++++++++----------- src/Platforms/SqlitePlatform.php | 27 +++++---- src/Schema/ColumnDiff.php | 22 ++++++- 8 files changed, 161 insertions(+), 120 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index 6e7020f9778..18a2cd94658 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -23,6 +23,11 @@ The following `Comparator` methods have been marked as internal: The `diffColumn()` method has been deprecated. Use `diffTable()` instead. +## Marked `ColumnDiff` public properties as internal. + +The `$fromColumn` and `$column` properties of the `ColumnDiff` class have been marked as internal. Use the +`getOldColumn()` and `getNewColumn()` methods instead. + ## Deprecated `ColumnDiff::$changedProperties` and `::hasChanged()`. The `ColumnDiff::$changedProperties` property and the `hasChanged()` method have been deprecated. Use one of the diff --git a/src/Platforms/AbstractMySQLPlatform.php b/src/Platforms/AbstractMySQLPlatform.php index eef22ff85c6..0b307592dbb 100644 --- a/src/Platforms/AbstractMySQLPlatform.php +++ b/src/Platforms/AbstractMySQLPlatform.php @@ -618,11 +618,14 @@ public function getAlterTableSQL(TableDiff $diff) continue; } - $columnArray = array_merge($column->toArray(), [ + $columnProperties = array_merge($column->toArray(), [ 'comment' => $this->getColumnComment($column), ]); - $queryParts[] = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnArray); + $queryParts[] = 'ADD ' . $this->getColumnDeclarationSQL( + $column->getQuotedName($this), + $columnProperties, + ); } foreach ($diff->removedColumns as $column) { @@ -638,19 +641,16 @@ public function getAlterTableSQL(TableDiff $diff) continue; } - $column = $columnDiff->column; - $columnArray = $column->toArray(); + $newColumn = $columnDiff->getNewColumn(); - $columnArray['comment'] = $this->getColumnComment($column); + $newColumnProperties = array_merge($newColumn->toArray(), [ + 'comment' => $this->getColumnComment($newColumn), + ]); - if ($columnDiff->fromColumn !== null) { - $fromColumn = $columnDiff->fromColumn; - } else { - $fromColumn = $columnDiff->getOldColumnName(); - } + $oldColumn = $columnDiff->getOldColumn() ?? $columnDiff->getOldColumnName(); - $queryParts[] = 'CHANGE ' . $fromColumn->getQuotedName($this) . ' ' - . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnArray); + $queryParts[] = 'CHANGE ' . $oldColumn->getQuotedName($this) . ' ' + . $this->getColumnDeclarationSQL($newColumn->getQuotedName($this), $newColumnProperties); } foreach ($diff->renamedColumns as $oldColumnName => $column) { @@ -658,11 +658,14 @@ public function getAlterTableSQL(TableDiff $diff) continue; } - $oldColumnName = new Identifier($oldColumnName); - $columnArray = $column->toArray(); - $columnArray['comment'] = $this->getColumnComment($column); - $queryParts[] = 'CHANGE ' . $oldColumnName->getQuotedName($this) . ' ' - . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnArray); + $oldColumnName = new Identifier($oldColumnName); + + $columnProperties = array_merge($column->toArray(), [ + 'comment' => $this->getColumnComment($column), + ]); + + $queryParts[] = 'CHANGE ' . $oldColumnName->getQuotedName($this) . ' ' + . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnProperties); } if (isset($diff->addedIndexes['primary'])) { diff --git a/src/Platforms/DB2Platform.php b/src/Platforms/DB2Platform.php index aa15c31a4b5..ce3c5ad456b 100644 --- a/src/Platforms/DB2Platform.php +++ b/src/Platforms/DB2Platform.php @@ -607,8 +607,8 @@ public function getAlterTableSQL(TableDiff $diff) if ($columnDiff->hasCommentChanged()) { $commentsSQL[] = $this->getCommentOnColumnSQL( $diff->getName($this)->getQuotedName($this), - $columnDiff->column->getQuotedName($this), - $this->getColumnComment($columnDiff->column), + $columnDiff->getNewColumn()->getQuotedName($this), + $this->getColumnComment($columnDiff->getNewColumn()), ); } @@ -702,12 +702,12 @@ private function gatherAlterColumnSQL( */ private function getAlterColumnClausesSQL(ColumnDiff $columnDiff): array { - $column = $columnDiff->column->toArray(); + $newColumn = $columnDiff->getNewColumn()->toArray(); - $alterClause = 'ALTER COLUMN ' . $columnDiff->column->getQuotedName($this); + $alterClause = 'ALTER COLUMN ' . $columnDiff->getNewColumn()->getQuotedName($this); - if ($column['columnDefinition'] !== null) { - return [$alterClause . ' ' . $column['columnDefinition']]; + if ($newColumn['columnDefinition'] !== null) { + return [$alterClause . ' ' . $newColumn['columnDefinition']]; } $clauses = []; @@ -719,16 +719,16 @@ private function getAlterColumnClausesSQL(ColumnDiff $columnDiff): array $columnDiff->hasScaleChanged() || $columnDiff->hasFixedChanged() ) { - $clauses[] = $alterClause . ' SET DATA TYPE ' . $column['type']->getSQLDeclaration($column, $this); + $clauses[] = $alterClause . ' SET DATA TYPE ' . $newColumn['type']->getSQLDeclaration($newColumn, $this); } if ($columnDiff->hasNotNullChanged()) { - $clauses[] = $column['notnull'] ? $alterClause . ' SET NOT NULL' : $alterClause . ' DROP NOT NULL'; + $clauses[] = $newColumn['notnull'] ? $alterClause . ' SET NOT NULL' : $alterClause . ' DROP NOT NULL'; } if ($columnDiff->hasDefaultChanged()) { - if (isset($column['default'])) { - $defaultClause = $this->getDefaultValueDeclarationSQL($column); + if (isset($newColumn['default'])) { + $defaultClause = $this->getDefaultValueDeclarationSQL($newColumn); if ($defaultClause !== '') { $clauses[] = $alterClause . ' SET' . $defaultClause; diff --git a/src/Platforms/OraclePlatform.php b/src/Platforms/OraclePlatform.php index 39ed91b3d08..35165c9464a 100644 --- a/src/Platforms/OraclePlatform.php +++ b/src/Platforms/OraclePlatform.php @@ -900,13 +900,13 @@ public function getAlterTableSQL(TableDiff $diff) continue; } - $column = $columnDiff->column; + $newColumn = $columnDiff->getNewColumn(); // Do not generate column alteration clause if type is binary and only fixed property has changed. // Oracle only supports binary type columns with variable length. // Avoids unnecessary table alteration statements. if ( - $column->getType() instanceof BinaryType && + $newColumn->getType() instanceof BinaryType && $columnDiff->hasFixedChanged() && count($columnDiff->changedProperties) === 1 ) { @@ -919,13 +919,13 @@ public function getAlterTableSQL(TableDiff $diff) * Do not add query part if only comment has changed */ if (! ($columnHasChangedComment && count($columnDiff->changedProperties) === 1)) { - $columnInfo = $column->toArray(); + $newColumnProperties = $newColumn->toArray(); if (! $columnDiff->hasNotNullChanged()) { - unset($columnInfo['notnull']); + unset($newColumnProperties['notnull']); } - $fields[] = $column->getQuotedName($this) . $this->getColumnDeclarationSQL('', $columnInfo); + $fields[] = $newColumn->getQuotedName($this) . $this->getColumnDeclarationSQL('', $newColumnProperties); } if (! $columnHasChangedComment) { @@ -934,8 +934,8 @@ public function getAlterTableSQL(TableDiff $diff) $commentsSQL[] = $this->getCommentOnColumnSQL( $diff->getName($this)->getQuotedName($this), - $column->getQuotedName($this), - $this->getColumnComment($column), + $newColumn->getQuotedName($this), + $this->getColumnComment($newColumn), ); } diff --git a/src/Platforms/PostgreSQLPlatform.php b/src/Platforms/PostgreSQLPlatform.php index a2e026ffb99..7da64ee2fc8 100644 --- a/src/Platforms/PostgreSQLPlatform.php +++ b/src/Platforms/PostgreSQLPlatform.php @@ -522,15 +522,15 @@ public function getAlterTableSQL(TableDiff $diff) $commentsSQL = []; $columnSql = []; - foreach ($diff->addedColumns as $column) { - if ($this->onSchemaAlterTableAddColumn($column, $diff, $columnSql)) { + foreach ($diff->addedColumns as $newColumn) { + if ($this->onSchemaAlterTableAddColumn($newColumn, $diff, $columnSql)) { continue; } - $query = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $column->toArray()); + $query = 'ADD ' . $this->getColumnDeclarationSQL($newColumn->getQuotedName($this), $newColumn->toArray()); $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; - $comment = $this->getColumnComment($column); + $comment = $this->getColumnComment($newColumn); if ($comment === null || $comment === '') { continue; @@ -538,17 +538,17 @@ public function getAlterTableSQL(TableDiff $diff) $commentsSQL[] = $this->getCommentOnColumnSQL( $diff->getName($this)->getQuotedName($this), - $column->getQuotedName($this), + $newColumn->getQuotedName($this), $comment, ); } - foreach ($diff->removedColumns as $column) { - if ($this->onSchemaAlterTableRemoveColumn($column, $diff, $columnSql)) { + foreach ($diff->removedColumns as $newColumn) { + if ($this->onSchemaAlterTableRemoveColumn($newColumn, $diff, $columnSql)) { continue; } - $query = 'DROP ' . $column->getQuotedName($this); + $query = 'DROP ' . $newColumn->getQuotedName($this); $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; } @@ -561,15 +561,10 @@ public function getAlterTableSQL(TableDiff $diff) continue; } - if ($columnDiff->fromColumn !== null) { - $fromColumn = $columnDiff->fromColumn; - } else { - $fromColumn = $columnDiff->getOldColumnName(); - } - - $oldColumnName = $fromColumn->getQuotedName($this); + $oldColumn = $columnDiff->getOldColumn() ?? $columnDiff->getOldColumnName(); + $newColumn = $columnDiff->getNewColumn(); - $column = $columnDiff->column; + $oldColumnName = $oldColumn->getQuotedName($this); if ( $columnDiff->hasTypeChanged() @@ -577,10 +572,10 @@ public function getAlterTableSQL(TableDiff $diff) || $columnDiff->hasScaleChanged() || $columnDiff->hasFixedChanged() ) { - $type = $column->getType(); + $type = $newColumn->getType(); // SERIAL/BIGSERIAL are not "real" types and we can't alter a column to that type - $columnDefinition = $column->toArray(); + $columnDefinition = $newColumn->toArray(); $columnDefinition['autoincrement'] = false; // here was a server version check before, but DBAL API does not support this anymore. @@ -589,20 +584,21 @@ public function getAlterTableSQL(TableDiff $diff) } if ($columnDiff->hasDefaultChanged()) { - $defaultClause = $column->getDefault() === null + $defaultClause = $newColumn->getDefault() === null ? ' DROP DEFAULT' - : ' SET' . $this->getDefaultValueDeclarationSQL($column->toArray()); - $query = 'ALTER ' . $oldColumnName . $defaultClause; - $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; + : ' SET' . $this->getDefaultValueDeclarationSQL($newColumn->toArray()); + + $query = 'ALTER ' . $oldColumnName . $defaultClause; + $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; } if ($columnDiff->hasNotNullChanged()) { - $query = 'ALTER ' . $oldColumnName . ' ' . ($column->getNotnull() ? 'SET' : 'DROP') . ' NOT NULL'; + $query = 'ALTER ' . $oldColumnName . ' ' . ($newColumn->getNotnull() ? 'SET' : 'DROP') . ' NOT NULL'; $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; } if ($columnDiff->hasAutoIncrementChanged()) { - if ($column->getAutoincrement()) { + if ($newColumn->getAutoincrement()) { // add autoincrement $seqName = $this->getIdentitySequenceName($diff->name, $oldColumnName); @@ -610,24 +606,24 @@ public function getAlterTableSQL(TableDiff $diff) $sql[] = "SELECT setval('" . $seqName . "', (SELECT MAX(" . $oldColumnName . ') FROM ' . $diff->getName($this)->getQuotedName($this) . '))'; $query = 'ALTER ' . $oldColumnName . " SET DEFAULT nextval('" . $seqName . "')"; - $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; } else { // Drop autoincrement, but do NOT drop the sequence. It might be re-used by other tables or have $query = 'ALTER ' . $oldColumnName . ' DROP DEFAULT'; - $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; } + + $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; } - $newComment = $this->getColumnComment($column); $oldComment = $this->getOldColumnComment($columnDiff); + $newComment = $this->getColumnComment($newColumn); if ( $columnDiff->hasCommentChanged() - || ($columnDiff->fromColumn !== null && $oldComment !== $newComment) + || ($columnDiff->getOldColumn() !== null && $oldComment !== $newComment) ) { $commentsSQL[] = $this->getCommentOnColumnSQL( $diff->getName($this)->getQuotedName($this), - $column->getQuotedName($this), + $newColumn->getQuotedName($this), $newComment, ); } @@ -637,7 +633,7 @@ public function getAlterTableSQL(TableDiff $diff) } $query = 'ALTER ' . $oldColumnName . ' TYPE ' - . $column->getType()->getSQLDeclaration($column->toArray(), $this); + . $newColumn->getType()->getSQLDeclaration($newColumn->toArray(), $this); $sql[] = 'ALTER TABLE ' . $diff->getName($this)->getQuotedName($this) . ' ' . $query; } @@ -688,18 +684,18 @@ public function getAlterTableSQL(TableDiff $diff) */ private function isUnchangedBinaryColumn(ColumnDiff $columnDiff): bool { - $columnType = $columnDiff->column->getType(); + $newColumnType = $columnDiff->getNewColumn()->getType(); - if (! $columnType instanceof BinaryType && ! $columnType instanceof BlobType) { + if (! $newColumnType instanceof BinaryType && ! $newColumnType instanceof BlobType) { return false; } - $fromColumn = $columnDiff->fromColumn instanceof Column ? $columnDiff->fromColumn : null; + $oldColumn = $columnDiff->getOldColumn() instanceof Column ? $columnDiff->getOldColumn() : null; - if ($fromColumn !== null) { - $fromColumnType = $fromColumn->getType(); + if ($oldColumn !== null) { + $oldColumnType = $oldColumn->getType(); - if (! $fromColumnType instanceof BinaryType && ! $fromColumnType instanceof BlobType) { + if (! $oldColumnType instanceof BinaryType && ! $oldColumnType instanceof BlobType) { return false; } @@ -1326,7 +1322,13 @@ public function getJsonTypeDeclarationSQL(array $column) private function getOldColumnComment(ColumnDiff $columnDiff): ?string { - return $columnDiff->fromColumn !== null ? $this->getColumnComment($columnDiff->fromColumn) : null; + $oldColumn = $columnDiff->getOldColumn(); + + if ($oldColumn !== null) { + return $this->getColumnComment($oldColumn); + } + + return null; } /** @deprecated The SQL used for schema introspection is an implementation detail and should not be relied upon. */ diff --git a/src/Platforms/SQLServerPlatform.php b/src/Platforms/SQLServerPlatform.php index 57abac932bd..84a92e1a5ef 100644 --- a/src/Platforms/SQLServerPlatform.php +++ b/src/Platforms/SQLServerPlatform.php @@ -521,12 +521,14 @@ public function getAlterTableSQL(TableDiff $diff) continue; } - $columnDef = $column->toArray(); - $addColumnSql = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef); - if (isset($columnDef['default'])) { + $columnProperties = $column->toArray(); + + $addColumnSql = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnProperties); + + if (isset($columnProperties['default'])) { $addColumnSql .= ' CONSTRAINT ' . $this->generateDefaultConstraintName($diff->name, $column->getQuotedName($this)) . - $this->getDefaultValueDeclarationSQL($columnDef); + $this->getDefaultValueDeclarationSQL($columnProperties); } $queryParts[] = $addColumnSql; @@ -557,27 +559,29 @@ public function getAlterTableSQL(TableDiff $diff) continue; } - $column = $columnDiff->column; - $comment = $this->getColumnComment($column); - $hasComment = ! empty($comment) || is_numeric($comment); + $newColumn = $columnDiff->getNewColumn(); + $newComment = $this->getColumnComment($newColumn); + $hasNewComment = ! empty($newComment) || is_numeric($newComment); + + $oldColumn = $columnDiff->getOldColumn(); - if ($columnDiff->fromColumn instanceof Column) { - $fromComment = $this->getColumnComment($columnDiff->fromColumn); - $hasFromComment = ! empty($fromComment) || is_numeric($fromComment); + if ($oldColumn instanceof Column) { + $oldComment = $this->getColumnComment($oldColumn); + $hasOldComment = ! empty($oldComment) || is_numeric($oldComment); - if ($hasFromComment && $hasComment && $fromComment !== $comment) { + if ($hasOldComment && $hasNewComment && $oldComment !== $newComment) { $commentsSql[] = $this->getAlterColumnCommentSQL( $diff->name, - $column->getQuotedName($this), - $comment, + $newColumn->getQuotedName($this), + $newComment, ); - } elseif ($hasFromComment && ! $hasComment) { - $commentsSql[] = $this->getDropColumnCommentSQL($diff->name, $column->getQuotedName($this)); - } elseif (! $hasFromComment && $hasComment) { + } elseif ($hasOldComment && ! $hasNewComment) { + $commentsSql[] = $this->getDropColumnCommentSQL($diff->name, $newColumn->getQuotedName($this)); + } elseif (! $hasOldComment && $hasNewComment) { $commentsSql[] = $this->getCreateColumnCommentSQL( $diff->name, - $column->getQuotedName($this), - $comment, + $newColumn->getQuotedName($this), + $newComment, ); } } @@ -590,54 +594,56 @@ public function getAlterTableSQL(TableDiff $diff) $requireDropDefaultConstraint = $this->alterColumnRequiresDropDefaultConstraint($columnDiff); if ($requireDropDefaultConstraint) { - if ($columnDiff->fromColumn !== null) { - $fromColumnName = $columnDiff->fromColumn->getName(); + $oldColumn = $columnDiff->getOldColumn(); + + if ($oldColumn !== null) { + $oldColumnName = $oldColumn->getName(); } else { - $fromColumnName = $columnDiff->oldColumnName; + $oldColumnName = $columnDiff->oldColumnName; } $queryParts[] = $this->getAlterTableDropDefaultConstraintClause( $diff->name, - $fromColumnName, + $oldColumnName, ); } - $columnDef = $column->toArray(); + $columnProperties = $newColumn->toArray(); $queryParts[] = 'ALTER COLUMN ' . - $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef); + $this->getColumnDeclarationSQL($newColumn->getQuotedName($this), $columnProperties); if ( - ! isset($columnDef['default']) + ! isset($columnProperties['default']) || (! $requireDropDefaultConstraint && ! $columnDiff->hasDefaultChanged()) ) { continue; } - $queryParts[] = $this->getAlterTableAddDefaultConstraintClause($diff->name, $column); + $queryParts[] = $this->getAlterTableAddDefaultConstraintClause($diff->name, $newColumn); } - foreach ($diff->renamedColumns as $fromColumnName => $column) { - if ($this->onSchemaAlterTableRenameColumn($fromColumnName, $column, $diff, $columnSql)) { + foreach ($diff->renamedColumns as $oldColumnName => $newColumn) { + if ($this->onSchemaAlterTableRenameColumn($oldColumnName, $newColumn, $diff, $columnSql)) { continue; } - $fromColumnName = new Identifier($fromColumnName); + $oldColumnName = new Identifier($oldColumnName); $sql[] = "sp_rename '" . - $diff->getName($this)->getQuotedName($this) . '.' . $fromColumnName->getQuotedName($this) . - "', '" . $column->getQuotedName($this) . "', 'COLUMN'"; + $diff->getName($this)->getQuotedName($this) . '.' . $oldColumnName->getQuotedName($this) . + "', '" . $newColumn->getQuotedName($this) . "', 'COLUMN'"; // Recreate default constraint with new column name if necessary (for future reference). - if ($column->getDefault() === null) { + if ($newColumn->getDefault() === null) { continue; } $queryParts[] = $this->getAlterTableDropDefaultConstraintClause( $diff->name, - $fromColumnName->getQuotedName($this), + $oldColumnName->getQuotedName($this), ); - $queryParts[] = $this->getAlterTableAddDefaultConstraintClause($diff->name, $column); + $queryParts[] = $this->getAlterTableAddDefaultConstraintClause($diff->name, $newColumn); } $tableSql = []; @@ -719,15 +725,17 @@ private function getAlterTableDropDefaultConstraintClause($tableName, $columnNam */ private function alterColumnRequiresDropDefaultConstraint(ColumnDiff $columnDiff): bool { + $oldColumn = $columnDiff->getOldColumn(); + // We can only decide whether to drop an existing default constraint // if we know the original default value. - if (! $columnDiff->fromColumn instanceof Column) { + if (! $oldColumn 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) { + if ($oldColumn->getDefault() === null) { return false; } diff --git a/src/Platforms/SqlitePlatform.php b/src/Platforms/SqlitePlatform.php index 2ade8a9211f..f69858766f4 100644 --- a/src/Platforms/SqlitePlatform.php +++ b/src/Platforms/SqlitePlatform.php @@ -16,6 +16,7 @@ use Doctrine\DBAL\Schema\TableDiff; use Doctrine\DBAL\TransactionIsolationLevel; use Doctrine\DBAL\Types; +use Doctrine\DBAL\Types\IntegerType; use Doctrine\Deprecations\Deprecation; use function array_combine; @@ -1048,13 +1049,13 @@ public function getAlterTableSQL(TableDiff $diff) } $oldColumnName = strtolower($oldColumnName); - $columns = $this->replaceColumn($diff->name, $columns, $oldColumnName, $columnDiff->column); + $columns = $this->replaceColumn($diff->name, $columns, $oldColumnName, $columnDiff->getNewColumn()); if (! isset($newColumnNames[$oldColumnName])) { continue; } - $newColumnNames[$oldColumnName] = $columnDiff->column->getQuotedName($this); + $newColumnNames[$oldColumnName] = $columnDiff->getNewColumn()->getQuotedName($this); } foreach ($diff->addedColumns as $columnName => $column) { @@ -1153,11 +1154,15 @@ 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 - ) { + $oldColumn = $columnDiff->getOldColumn(); + + if ($oldColumn === null) { + continue; + } + + $newColumn = $columnDiff->getNewColumn(); + + if (! $newColumn->getAutoincrement() || ! $newColumn->getType() instanceof IntegerType) { continue; } @@ -1167,7 +1172,7 @@ private function getSimpleAlterTableSQL(TableDiff $diff) continue; } - $fromColumnType = $columnDiff->fromColumn->getType(); + $fromColumnType = $oldColumn->getType(); if (! ($fromColumnType instanceof Types\SmallIntType) && ! ($fromColumnType instanceof Types\BigIntType)) { continue; @@ -1264,9 +1269,9 @@ private function getColumnNamesInAlteredTable(TableDiff $diff, Table $fromTable) } foreach ($diff->changedColumns as $oldColumnName => $columnDiff) { - $columnName = $columnDiff->column->getName(); - $columns[strtolower($oldColumnName)] = $columnName; - $columns[strtolower($columnName)] = $columnName; + $newColumnName = $columnDiff->getNewColumn()->getName(); + $columns[strtolower($oldColumnName)] = $newColumnName; + $columns[strtolower($newColumnName)] = $newColumnName; } foreach ($diff->addedColumns as $column) { diff --git a/src/Schema/ColumnDiff.php b/src/Schema/ColumnDiff.php index b7c07ee115b..bd1b0eee05f 100644 --- a/src/Schema/ColumnDiff.php +++ b/src/Schema/ColumnDiff.php @@ -18,7 +18,11 @@ class ColumnDiff */ public $oldColumnName; - /** @var Column */ + /** + * @internal Use {@see getNewColumn()} instead. + * + * @var Column + */ public $column; /** @@ -30,7 +34,11 @@ class ColumnDiff */ public $changedProperties = []; - /** @var Column|null */ + /** + * @internal Use {@see getOldColumn()} instead. + * + * @var Column|null + */ public $fromColumn; /** @@ -60,6 +68,16 @@ public function __construct( $this->fromColumn = $fromColumn; } + public function getOldColumn(): ?Column + { + return $this->fromColumn; + } + + public function getNewColumn(): Column + { + return $this->column; + } + public function hasTypeChanged(): bool { return $this->hasChanged('type'); From 852af9e1799340184addae4e884637986d62525e Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Tue, 6 Sep 2022 18:17:32 -0700 Subject: [PATCH 3/3] Remove implementation of alterTable() for SQLite --- src/Schema/SqliteSchemaManager.php | 11 ----------- tests/Functional/Schema/OracleSchemaManagerTest.php | 11 ----------- .../Schema/SchemaManagerFunctionalTestCase.php | 9 +++++++++ tests/Functional/Schema/SqliteSchemaManagerTest.php | 10 ---------- 4 files changed, 9 insertions(+), 32 deletions(-) diff --git a/src/Schema/SqliteSchemaManager.php b/src/Schema/SqliteSchemaManager.php index 701cd7fb847..771c70e5be7 100644 --- a/src/Schema/SqliteSchemaManager.php +++ b/src/Schema/SqliteSchemaManager.php @@ -150,17 +150,6 @@ public function createDatabase($database) $conn->close(); } - /** - * {@inheritdoc} - */ - public function renameTable($name, $newName) - { - $tableDiff = new TableDiff($name); - $tableDiff->fromTable = $this->listTableDetails($name); - $tableDiff->newName = $newName; - $this->alterTable($tableDiff); - } - /** * {@inheritdoc} */ diff --git a/tests/Functional/Schema/OracleSchemaManagerTest.php b/tests/Functional/Schema/OracleSchemaManagerTest.php index 79f54d88a21..7bab45ad3e9 100644 --- a/tests/Functional/Schema/OracleSchemaManagerTest.php +++ b/tests/Functional/Schema/OracleSchemaManagerTest.php @@ -22,17 +22,6 @@ protected function supportsPlatform(AbstractPlatform $platform): bool return $platform instanceof OraclePlatform; } - public function testRenameTable(): void - { - $this->createTestTable('list_tables_test'); - $this->dropTableIfExists('list_tables_test_new_name'); - $this->schemaManager->renameTable('list_tables_test', 'list_tables_test_new_name'); - - $tables = $this->schemaManager->listTables(); - - self::assertHasTable($tables); - } - /** * Oracle currently stores VARBINARY columns as RAW (fixed-size) */ diff --git a/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php index 7cbcb3ba9ce..b7e6093e317 100644 --- a/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php +++ b/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php @@ -261,6 +261,15 @@ public static function tableFilterProvider(): iterable yield 'Two tables' => ['filter_test_', 2]; } + public function testRenameTable(): void + { + $this->createTestTable('old_name'); + $this->schemaManager->renameTable('old_name', 'new_name'); + + self::assertFalse($this->schemaManager->tablesExist(['old_name'])); + self::assertTrue($this->schemaManager->tablesExist(['new_name'])); + } + public function createListTableColumns(): Table { $table = new Table('list_table_columns'); diff --git a/tests/Functional/Schema/SqliteSchemaManagerTest.php b/tests/Functional/Schema/SqliteSchemaManagerTest.php index 8f586a32845..203f6360493 100644 --- a/tests/Functional/Schema/SqliteSchemaManagerTest.php +++ b/tests/Functional/Schema/SqliteSchemaManagerTest.php @@ -41,16 +41,6 @@ public function testCreateAndDropDatabase(): void self::assertFileDoesNotExist($path); } - public function testRenameTable(): void - { - $this->createTestTable('oldname'); - $this->schemaManager->renameTable('oldname', 'newname'); - - $tables = $this->schemaManager->listTableNames(); - self::assertContains('newname', $tables); - self::assertNotContains('oldname', $tables); - } - public function createListTableColumns(): Table { $table = parent::createListTableColumns();