Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add explicit renameColumn method (4.x) #6280

Open
wants to merge 4 commits into
base: 4.1.x
Choose a base branch
from
Open

Add explicit renameColumn method (4.x) #6280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e48825
Add explicit renameColumn method (4.x)
Tofandel Jan 26, 2024
da6c67b
Update src/Schema/SQLiteSchemaManager.php
Tofandel Feb 6, 2024
1be377a
Move deprecated method getRenamedColumn to test class
Tofandel Feb 20, 2024
af0a153
Merge branch '4.1.x' into 4.0.x
Tofandel Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 14 additions & 31 deletions src/Platforms/AbstractMySQLPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
use Doctrine\DBAL\Platforms\Keywords\MySQLKeywords;
use Doctrine\DBAL\Schema\AbstractAsset;
use Doctrine\DBAL\Schema\ForeignKeyConstraint;
use Doctrine\DBAL\Schema\Identifier;
use Doctrine\DBAL\Schema\Index;
use Doctrine\DBAL\Schema\MySQLSchemaManager;
use Doctrine\DBAL\Schema\TableDiff;
Expand Down Expand Up @@ -335,7 +334,6 @@ private function buildTableOptions(array $options): string
*/
public function getAlterTableSQL(TableDiff $diff): array
{
$columnSql = [];
$queryParts = [];

foreach ($diff->getAddedColumns() as $column) {
Expand All @@ -353,7 +351,7 @@ public function getAlterTableSQL(TableDiff $diff): array
$queryParts[] = 'DROP ' . $column->getQuotedName($this);
}

foreach ($diff->getModifiedColumns() as $columnDiff) {
foreach ($diff->getChangedColumns() as $columnDiff) {
$newColumn = $columnDiff->getNewColumn();

$newColumnProperties = array_merge($newColumn->toArray(), [
Expand All @@ -366,17 +364,6 @@ public function getAlterTableSQL(TableDiff $diff): array
. $this->getColumnDeclarationSQL($newColumn->getQuotedName($this), $newColumnProperties);
}

foreach ($diff->getRenamedColumns() as $oldColumnName => $column) {
$oldColumnName = new Identifier($oldColumnName);

$columnProperties = array_merge($column->toArray(), [
'comment' => $column->getComment(),
]);

$queryParts[] = 'CHANGE ' . $oldColumnName->getQuotedName($this) . ' '
. $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnProperties);
}

$addedIndexes = $this->indexAssetsByLowerCaseName($diff->getAddedIndexes());
$modifiedIndexes = $this->indexAssetsByLowerCaseName($diff->getModifiedIndexes());
$diffModified = false;
Expand Down Expand Up @@ -405,35 +392,31 @@ public function getAlterTableSQL(TableDiff $diff): array
if ($diffModified) {
$diff = new TableDiff(
$diff->getOldTable(),
$diff->getAddedColumns(),
$diff->getModifiedColumns(),
$diff->getDroppedColumns(),
$diff->getRenamedColumns(),
array_values($addedIndexes),
array_values($modifiedIndexes),
$diff->getDroppedIndexes(),
$diff->getRenamedIndexes(),
$diff->getAddedForeignKeys(),
$diff->getModifiedForeignKeys(),
$diff->getDroppedForeignKeys(),
addedColumns: $diff->getAddedColumns(),
changedColumns: $diff->getChangedColumns(),
droppedColumns: $diff->getDroppedColumns(),
addedIndexes: array_values($addedIndexes),
modifiedIndexes: array_values($modifiedIndexes),
droppedIndexes: $diff->getDroppedIndexes(),
renamedIndexes: $diff->getRenamedIndexes(),
addedForeignKeys: $diff->getAddedForeignKeys(),
modifiedForeignKeys: $diff->getModifiedForeignKeys(),
droppedForeignKeys: $diff->getDroppedForeignKeys(),
);
}

$sql = [];
$tableSql = [];

if (count($queryParts) > 0) {
$sql[] = 'ALTER TABLE ' . $diff->getOldTable()->getQuotedName($this) . ' '
$tableSql[] = 'ALTER TABLE ' . $diff->getOldTable()->getQuotedName($this) . ' '
. implode(', ', $queryParts);
}

$sql = array_merge(
return array_merge(
$this->getPreAlterTableIndexForeignKeySQL($diff),
$sql,
$tableSql,
$this->getPostAlterTableIndexForeignKeySQL($diff),
);

return array_merge($sql, $tableSql, $columnSql);
}

/**
Expand Down
23 changes: 18 additions & 5 deletions src/Platforms/AbstractPlatform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,8 +815,7 @@
}
}

$columnSql = [];
$columns = [];
$columns = [];

foreach ($table->getColumns() as $column) {
$columnData = $this->columnToArray($column);
Expand Down Expand Up @@ -846,7 +845,7 @@
}
}

return array_merge($sql, $columnSql);
return $sql;
}

/**
Expand Down Expand Up @@ -944,7 +943,7 @@
* @param mixed[][] $columns
* @param mixed[] $options
*
* @return array<int, string>
* @return list<string>
*/
protected function _getCreateTableSQL(string $name, array $columns, array $options = []): array
{
Expand All @@ -961,7 +960,7 @@
}

if (isset($options['indexes']) && ! empty($options['indexes'])) {
foreach ($options['indexes'] as $index => $definition) {
foreach ($options['indexes'] as $definition) {

Check warning on line 963 in src/Platforms/AbstractPlatform.php

View check run for this annotation

Codecov / codecov/patch

src/Platforms/AbstractPlatform.php#L963 

Added line #L963 was not covered by tests
$columnListSql .= ', ' . $this->getIndexDeclarationSQL($definition);
}
}
Expand Down Expand Up @@ -1290,6 +1289,20 @@
];
}

/**
* Returns the SQL for renaming a column
*
* @param string $tableName The table to rename the column on.
* @param string $oldColumnName The name of the column we want to rename.
* @param string $newColumnName The name we should rename it to.
*
* @return list<string> The sequence of SQL statements for renaming the given column.
*/
protected function getRenameColumnSQL(string $tableName, string $oldColumnName, string $newColumnName): array

Check warning on line 1301 in src/Platforms/AbstractPlatform.php

View check run for this annotation

Codecov / codecov/patch

src/Platforms/AbstractPlatform.php#L1301 

Added line #L1301 was not covered by tests
{
return [sprintf('ALTER TABLE %s RENAME COLUMN %s TO %s', $tableName, $oldColumnName, $newColumnName)];

Check warning on line 1303 in src/Platforms/AbstractPlatform.php

View check run for this annotation

Codecov / codecov/patch

src/Platforms/AbstractPlatform.php#L1303 

Added line #L1303 was not covered by tests
}

/**
* Gets declaration of a number of columns in bulk.
*
Expand Down
68 changes: 39 additions & 29 deletions src/Platforms/DB2Platform.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Doctrine\DBAL\SQL\Builder\DefaultSelectSQLBuilder;
use Doctrine\DBAL\SQL\Builder\SelectSQLBuilder;
use Doctrine\DBAL\TransactionIsolationLevel;
use Doctrine\DBAL\Types\DateTimeType;
use Doctrine\DBAL\Types\Types;

use function array_merge;
Expand Down Expand Up @@ -262,7 +263,6 @@
public function getAlterTableSQL(TableDiff $diff): array
{
$sql = [];
$columnSql = [];
$commentsSQL = [];

$tableNameSQL = $diff->getOldTable()->getQuotedName($this);
Expand Down Expand Up @@ -296,11 +296,13 @@
);
}

$needsReorg = false;
foreach ($diff->getDroppedColumns() as $column) {
$queryParts[] = 'DROP COLUMN ' . $column->getQuotedName($this);
$needsReorg = true;

Check warning on line 302 in src/Platforms/DB2Platform.php

View check run for this annotation

Codecov / codecov/patch

src/Platforms/DB2Platform.php#L302 

Added line #L302 was not covered by tests
}

foreach ($diff->getModifiedColumns() as $columnDiff) {
foreach ($diff->getChangedColumns() as $columnDiff) {
if ($columnDiff->hasCommentChanged()) {
$newColumn = $columnDiff->getNewColumn();
$commentsSQL[] = $this->getCommentOnColumnSQL(
Expand All @@ -315,33 +317,25 @@
$columnDiff,
$sql,
$queryParts,
$needsReorg,
);
}

foreach ($diff->getRenamedColumns() as $oldColumnName => $column) {
$oldColumnName = new Identifier($oldColumnName);

$queryParts[] = 'RENAME COLUMN ' . $oldColumnName->getQuotedName($this) .
' TO ' . $column->getQuotedName($this);
}

if (count($queryParts) > 0) {
$sql[] = 'ALTER TABLE ' . $tableNameSQL . ' ' . implode(' ', $queryParts);
}

// Some table alteration operations require a table reorganization.
if (count($diff->getDroppedColumns()) > 0 || count($diff->getModifiedColumns()) > 0) {
if ($needsReorg) {
$sql[] = "CALL SYSPROC.ADMIN_CMD ('REORG TABLE " . $tableNameSQL . "')";
}

$sql = array_merge(
return array_merge(
$this->getPreAlterTableIndexForeignKeySQL($diff),
$sql,
$commentsSQL,
$this->getPostAlterTableIndexForeignKeySQL($diff),
);

return array_merge($sql, $columnSql);
}

public function getRenameTableSQL(string $oldName, string $newName): string
Expand All @@ -362,10 +356,11 @@
ColumnDiff $columnDiff,
array &$sql,
array &$queryParts,
bool &$needsReorg,
): void {
$alterColumnClauses = $this->getAlterColumnClausesSQL($columnDiff);
$alterColumnClauses = $this->getAlterColumnClausesSQL($columnDiff, $needsReorg);

if (empty($alterColumnClauses)) {
if (count($alterColumnClauses) < 1) {
return;
}

Expand All @@ -389,41 +384,56 @@
*
* @return string[]
*/
private function getAlterColumnClausesSQL(ColumnDiff $columnDiff): array
private function getAlterColumnClausesSQL(ColumnDiff $columnDiff, bool &$needsReorg): array
{
$newColumn = $columnDiff->getNewColumn()->toArray();
$newColumn = $columnDiff->getNewColumn();
$columnArray = $newColumn->toArray();

$newName = $columnDiff->getNewColumn()->getQuotedName($this);
$oldName = $columnDiff->getOldColumn()->getQuotedName($this);

$alterClause = 'ALTER COLUMN ' . $columnDiff->getNewColumn()->getQuotedName($this);
$alterClause = 'ALTER COLUMN ' . $newName;

if ($newColumn['columnDefinition'] !== null) {
return [$alterClause . ' ' . $newColumn['columnDefinition']];
if ($newColumn->getColumnDefinition() !== null) {
$needsReorg = true;

return [$alterClause . ' ' . $newColumn->getColumnDefinition()];
}

$clauses = [];

if ($columnDiff->hasNameChanged()) {
$clauses[] = 'RENAME COLUMN ' . $oldName . ' TO ' . $newName;

Check warning on line 406 in src/Platforms/DB2Platform.php

View check run for this annotation

Codecov / codecov/patch

src/Platforms/DB2Platform.php#L406 

Added line #L406 was not covered by tests
}

if (
$columnDiff->hasTypeChanged() ||
$columnDiff->hasLengthChanged() ||
$columnDiff->hasPrecisionChanged() ||
$columnDiff->hasScaleChanged() ||
$columnDiff->hasFixedChanged()
) {
$clauses[] = $alterClause . ' SET DATA TYPE ' . $newColumn['type']->getSQLDeclaration($newColumn, $this);
$needsReorg = true;
$clauses[] = $alterClause . ' SET DATA TYPE ' . $newColumn->getType()
->getSQLDeclaration($columnArray, $this);
}

if ($columnDiff->hasNotNullChanged()) {
$clauses[] = $newColumn['notnull'] ? $alterClause . ' SET NOT NULL' : $alterClause . ' DROP NOT NULL';
$needsReorg = true;
$clauses[] = $newColumn->getNotnull() ? $alterClause . ' SET NOT NULL' : $alterClause . ' DROP NOT NULL';
}

if ($columnDiff->hasDefaultChanged()) {
if (isset($newColumn['default'])) {
$defaultClause = $this->getDefaultValueDeclarationSQL($newColumn);
if ($newColumn->getDefault() !== null) {
$defaultClause = $this->getDefaultValueDeclarationSQL($columnArray);

if ($defaultClause !== '') {
$clauses[] = $alterClause . ' SET' . $defaultClause;
$needsReorg = true;
$clauses[] = $alterClause . ' SET' . $defaultClause;
}
} else {
$clauses[] = $alterClause . ' DROP DEFAULT';
$needsReorg = true;
$clauses[] = $alterClause . ' DROP DEFAULT';
}
}

Expand Down Expand Up @@ -485,12 +495,12 @@
*/
public function getDefaultValueDeclarationSQL(array $column): string
{
if (! empty($column['autoincrement'])) {
if (isset($column['autoincrement']) && $column['autoincrement'] === true) {
return '';
}

if (! empty($column['version'])) {
if ((string) $column['type'] !== 'DateTime') {
Copy link
Contributor Author

@Tofandel Tofandel Jan 26, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be covered by tests, when I messed around and removed the if there was an error that Dbal/Types/DateTimeType cannot be converted to string, which makes sense because 'type' in column is a DbalType which doesn't have a __toString(), might need to be backported to 3.8 as well

I also think in 4.x we should abolish column converted to arrays in all the AbstractPlatform methods and use the Column class to get the correct type hints and static analysis (for a different PR)

if (isset($column['version']) && $column['version'] === true) {
if ($column['type'] instanceof DateTimeType) {

Check warning on line 503 in src/Platforms/DB2Platform.php

View check run for this annotation

Codecov / codecov/patch

src/Platforms/DB2Platform.php#L503 

Added line #L503 was not covered by tests
$column['default'] = '1';
}
}
Expand Down