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.

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

Deprecate renaming tables via TableDiff #5663

Merged
merged 1 commit into from
Sep 18, 2022

Conversation

morozov
Copy link
Member

@morozov morozov commented Sep 10, 2022

Schema comparators do not detect renamed tables, so they do not populate TableDiff::$newName. Schema managers doing so in their renameTable() methods is a hack.

For comparison, ColumnDiff does not represent a change in the column name, it is tracked at a higher level, in TableDiff. By the same token, TableDiff should not represent a change in the table name. If it will be ever supported, it should be tracked in SchemaDiff.

Getting rid of handling the new name in the table diff is a prerequisite for future refactoring of the TableDiff API. Eventually, I want to get rid of public properties there and make the object immutable similar to what was done in ColumnDiff (#5657).

@morozov morozov marked this pull request as ready for review September 10, 2022 19:19
Comment on lines 714 to 722
"DECLARE @sql NVARCHAR(MAX) = N''; "
. "SELECT @sql += N'EXEC sp_rename N''' + dc.name + ''', N''' "
. "+ REPLACE(dc.name, '" . $this->generateIdentifierName($oldName)
. "', "
. "'" . $this->generateIdentifierName($newName) . "') + ''', ''OBJECT'';' "
. 'FROM sys.default_constraints dc '
. 'JOIN sys.tables tbl ON dc.parent_object_id = tbl.object_id '
. 'WHERE tbl.name = ' . $this->quoteStringLiteral($newName) . ';'
. 'EXEC sp_executesql @sql',
Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to have linebreaks inside this string and that this looks a job for nowdoc + sprintf.

Suggested change
"DECLARE @sql NVARCHAR(MAX) = N''; "
. "SELECT @sql += N'EXEC sp_rename N''' + dc.name + ''', N''' "
. "+ REPLACE(dc.name, '" . $this->generateIdentifierName($oldName)
. "', "
. "'" . $this->generateIdentifierName($newName) . "') + ''', ''OBJECT'';' "
. 'FROM sys.default_constraints dc '
. 'JOIN sys.tables tbl ON dc.parent_object_id = tbl.object_id '
. 'WHERE tbl.name = ' . $this->quoteStringLiteral($newName) . ';'
. 'EXEC sp_executesql @sql',
sprintf(
<<<'SQL'
DECLARE @sql NVARCHAR(MAX) = N'';
SELECT @sql += N'EXEC sp_rename N''' + dc.name + ''', N'''
+ REPLACE(dc.name, '%s', '%s') + ''', ''OBJECT'';'
FROM sys.default_constraints dc
JOIN sys.tables tbl ON dc.parent_object_id = tbl.object_id
WHERE tbl.name = %s;
EXEC sp_executesql @sql'
SQL,
$this->generateIdentifierName($oldName),
$this->generateIdentifierName($newName),
$this->quoteStringLiteral($newName)
)

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a copy-paste from getAlterTableSQL() which actually didn't need to be duplicated. I removed the original code and made the logic of the new method reused.

@morozov morozov force-pushed the deprecate-table-diff-new-name branch from 634a403 to 217ce8d Compare September 18, 2022 14:10
@morozov morozov requested a review from greg0ire September 18, 2022 14:40
@morozov morozov added this to the 3.5.0 milestone Sep 18, 2022
@morozov morozov merged commit 01e7f90 into doctrine:3.5.x Sep 18, 2022
@morozov morozov deleted the deprecate-table-diff-new-name branch September 18, 2022 14:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants