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

Ensure the default is declared in the same statement for new columns #4447

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

duncan3dc
Copy link
Contributor

Q A
Type bug
BC Break no
Fixed issues #2265

Summary

When adding columns to tables that already contain data, SQL Server requires that the default constraint is declared in the same statement, Doctrine currently generates a separate ADD CONSTRAINT statement, which means the add column statement errors with something like:

SQLSTATE [42000, 4901]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]
ALTER TABLE only allows columns to be added that can contain nulls, or have a DEFAULT
definition specified, or the column being added is an identity or timestamp column,
or alternatively if none of the previous conditions are satisfied the table must be
empty to allow addition of this column. Column 'addcolumn' cannot be added
to non-empty table 'mytable' because it does not satisfy these conditions.

greg0ire
greg0ire previously approved these changes Nov 24, 2020
@morozov
Copy link
Member

morozov commented Nov 24, 2020

Thank you for the patch, @duncan3dc. In addition to the unit test, it would be good to have an integration test that confirms that the fixed SQL also works and produces the expected results. See the testing guidelines for more details.

@duncan3dc duncan3dc force-pushed the sql-server-add-column-default branch 2 times, most recently from a27f64c to 2ea06d1 Compare November 24, 2020 21:22
@duncan3dc
Copy link
Contributor Author

Of course, I've had a go at that in 2ea06d1 it fails against the 2.12.x branch and passes with this PR

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

Looks good. Please squash into one commit since the code change and both tests belong together.

@@ -484,12 +484,15 @@ public function getAlterTableSQL(TableDiff $diff)
}

$columnDef = $column->toArray();
$queryParts[] = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef);

$addColumnSql = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef);
if (isset($columnDef['default'])) {
Copy link
Member

Choose a reason for hiding this comment

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

This is out of the scope of this issue but it looks like this code incorrectly handles the DEFAULT NULL case.

@morozov morozov merged commit 2d1f747 into doctrine:2.12.x Nov 25, 2020
@morozov
Copy link
Member

morozov commented Nov 25, 2020

Thanks, @duncan3dc!

@morozov morozov added this to the 2.13.0 milestone Feb 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 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