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

Remove TableDiff::$name and getName() #5683

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

morozov
Copy link
Member

@morozov morozov commented Sep 20, 2022

The API being removed was deprecated in #5678.

Additionally, this patch makes the reference from the table diff to the old table mandatory. In order to do that, it moves the corresponding constructor argument from the last position to the first, since all remaining arguments are still optional. This change should not affect the consumers of the DBAL API since the constructor is internal.

@morozov morozov added this to the 4.0.0 milestone Sep 20, 2022
@morozov morozov marked this pull request as ready for review September 20, 2022 15:54
@morozov morozov requested a review from greg0ire September 20, 2022 15:54
@morozov morozov force-pushed the remove-table-diff-name branch 2 times, most recently from 367c09e to 39e4ff8 Compare September 20, 2022 16:09
@@ -1224,7 +1228,7 @@ public static function getGeneratesIdentifierNamesInAlterTableSQL(): iterable
// Quoted identifiers non-reserved keywords.
[
new TableDiff(
'`mytable`',
new Table('mytable'),
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to keep the `?

Suggested change
new Table('mytable'),
new Table('`mytable`'),

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! The quotes got lost during refactoring.

@@ -1300,7 +1304,7 @@ public static function getGeneratesIdentifierNamesInAlterTableSQL(): iterable
// Quoted identifiers reserved keywords.
[
new TableDiff(
'`table`',
new Table('mytable'),
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to keep the `?

Suggested change
new Table('mytable'),
new Table('`mytable`'),

Copy link
Member Author

@morozov morozov Sep 20, 2022

Choose a reason for hiding this comment

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

The table name got accidentally changed during the refactoring but apparently, it's irrelevant to the test.

@morozov morozov force-pushed the remove-table-diff-name branch from 39e4ff8 to 6381aca Compare September 20, 2022 21:17
@morozov morozov merged commit 76635d3 into doctrine:4.0.x Sep 20, 2022
@morozov morozov deleted the remove-table-diff-name branch September 20, 2022 21:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 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.

3 participants