From 17edd0d54a16338738274161c422ba5486e2766b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Steve=20M=C3=BCller?= Date: Fri, 20 Dec 2013 16:29:30 +0100 Subject: [PATCH] fix column default value lifecycle --- .../DBAL/Platforms/PostgreSqlPlatform.php | 5 +- .../DBAL/Platforms/SQLAnywherePlatform.php | 10 +++- .../DBAL/Platforms/SQLServerPlatform.php | 14 ++--- lib/Doctrine/DBAL/Schema/Comparator.php | 10 +++- .../DBAL/Schema/DrizzleSchemaManager.php | 2 +- .../DBAL/Schema/SQLAnywhereSchemaManager.php | 4 ++ .../SchemaManagerFunctionalTestCase.php | 52 +++++++++++++++++++ .../DBAL/Platforms/PostgreSqlPlatformTest.php | 4 +- .../DBAL/Platforms/SQLServerPlatformTest.php | 1 + 9 files changed, 88 insertions(+), 14 deletions(-) diff --git a/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php b/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php index 6f9cc6c742a..d5fe343501d 100644 --- a/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php @@ -470,7 +470,10 @@ public function getAlterTableSQL(TableDiff $diff) } if ($columnDiff->hasChanged('default')) { - $query = 'ALTER ' . $oldColumnName . ' SET ' . $this->getDefaultValueDeclarationSQL($column->toArray()); + $defaultClause = null === $column->getDefault() + ? ' DROP DEFAULT' + : ' SET' . $this->getDefaultValueDeclarationSQL($column->toArray()); + $query = 'ALTER ' . $oldColumnName . $defaultClause; $sql[] = 'ALTER TABLE ' . $diff->getName()->getQuotedName($this) . ' ' . $query; } diff --git a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php index 60217cad69f..dfb9573fba6 100644 --- a/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php @@ -22,7 +22,6 @@ use Doctrine\DBAL\Connection; use Doctrine\DBAL\DBALException; use Doctrine\DBAL\LockMode; -use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Schema\Column; use Doctrine\DBAL\Schema\ColumnDiff; use Doctrine\DBAL\Schema\Constraint; @@ -287,7 +286,14 @@ protected function getAlterTableChangeColumnClause(ColumnDiff $columnDiff) // Do not return alter clause if only comment has changed. if ( ! ($columnDiff->hasChanged('comment') && count($columnDiff->changedProperties) === 1)) { - return 'ALTER ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $column->toArray()); + $columnAlterationClause = 'ALTER ' . + $this->getColumnDeclarationSQL($column->getQuotedName($this), $column->toArray()); + + if ($columnDiff->hasChanged('default') && null === $column->getDefault()) { + $columnAlterationClause .= ', ALTER ' . $column->getQuotedName($this) . ' DROP DEFAULT'; + } + + return $columnAlterationClause; } } diff --git a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php index bc28b018fd6..aaf63387492 100644 --- a/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php +++ b/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php @@ -246,7 +246,7 @@ protected function _getCreateTableSQL($tableName, array $columns, array $options } // Build default constraints SQL statements. - if ( ! empty($column['default']) || is_numeric($column['default'])) { + if (isset($column['default'])) { $defaultConstraintsSql[] = 'ALTER TABLE ' . $tableName . ' ADD' . $this->getDefaultConstraintDeclarationSQL($tableName, $column); } @@ -352,7 +352,7 @@ protected function getCreateColumnCommentSQL($tableName, $columnName, $comment) */ public function getDefaultConstraintDeclarationSQL($table, array $column) { - if (empty($column['default']) && ! is_numeric($column['default'])) { + if ( ! isset($column['default'])) { throw new \InvalidArgumentException("Incomplete column definition. 'default' required."); } @@ -446,7 +446,7 @@ public function getAlterTableSQL(TableDiff $diff) $columnDef = $column->toArray(); $queryParts[] = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef); - if ( ! empty($columnDef['default']) || is_numeric($columnDef['default'])) { + if (isset($columnDef['default'])) { $columnDef['name'] = $column->getQuotedName($this); $queryParts[] = 'ADD' . $this->getDefaultConstraintDeclarationSQL($diff->name, $columnDef); } @@ -517,7 +517,7 @@ public function getAlterTableSQL(TableDiff $diff) * if default value has changed and another * default constraint already exists for the column. */ - if ($columnDefaultHasChanged && ( ! empty($fromColumnDefault) || is_numeric($fromColumnDefault))) { + if ($columnDefaultHasChanged && null !== $fromColumnDefault) { $queryParts[] = 'DROP CONSTRAINT ' . $this->generateDefaultConstraintName($diff->name, $columnDiff->oldColumnName); } @@ -525,7 +525,7 @@ public function getAlterTableSQL(TableDiff $diff) $queryParts[] = 'ALTER COLUMN ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef); - if ($columnDefaultHasChanged && (! empty($columnDef['default']) || is_numeric($columnDef['default']))) { + if ($columnDefaultHasChanged && isset($columnDef['default'])) { $columnDef['name'] = $column->getQuotedName($this); $queryParts[] = 'ADD' . $this->getDefaultConstraintDeclarationSQL($diff->name, $columnDef); } @@ -546,7 +546,7 @@ public function getAlterTableSQL(TableDiff $diff) * Drop existing default constraint for the old column name * if column has default value. */ - if ( ! empty($columnDef['default']) || is_numeric($columnDef['default'])) { + if (isset($columnDef['default'])) { $queryParts[] = 'DROP CONSTRAINT ' . $this->generateDefaultConstraintName($diff->name, $oldColumnName); } @@ -557,7 +557,7 @@ public function getAlterTableSQL(TableDiff $diff) /** * Readd default constraint for the new column name. */ - if ( ! empty($columnDef['default']) || is_numeric($columnDef['default'])) { + if (isset($columnDef['default'])) { $columnDef['name'] = $column->getQuotedName($this); $queryParts[] = 'ADD' . $this->getDefaultConstraintDeclarationSQL($diff->name, $columnDef); } diff --git a/lib/Doctrine/DBAL/Schema/Comparator.php b/lib/Doctrine/DBAL/Schema/Comparator.php index cc66d0337f8..e4acf7a7a50 100644 --- a/lib/Doctrine/DBAL/Schema/Comparator.php +++ b/lib/Doctrine/DBAL/Schema/Comparator.php @@ -369,7 +369,15 @@ public function diffColumn(Column $column1, Column $column2) $changedProperties[] = 'notnull'; } - if ($column1->getDefault() != $column2->getDefault()) { + $column1Default = $column1->getDefault(); + $column2Default = $column2->getDefault(); + + if ($column1Default != $column2Default || + // Null values need to be checked additionally as they tell whether to create or drop a default value. + // null != 0, null != false, null != '' etc. This affects platform's table alteration SQL generation. + (null === $column1Default && null !== $column2Default) || + (null === $column2Default && null !== $column1Default) + ) { $changedProperties[] = 'default'; } diff --git a/lib/Doctrine/DBAL/Schema/DrizzleSchemaManager.php b/lib/Doctrine/DBAL/Schema/DrizzleSchemaManager.php index cffd40e0676..fa978983775 100644 --- a/lib/Doctrine/DBAL/Schema/DrizzleSchemaManager.php +++ b/lib/Doctrine/DBAL/Schema/DrizzleSchemaManager.php @@ -41,7 +41,7 @@ protected function _getPortableTableColumnDefinition($tableColumn) $options = array( 'notnull' => !(bool)$tableColumn['IS_NULLABLE'], 'length' => (int)$tableColumn['CHARACTER_MAXIMUM_LENGTH'], - 'default' => empty($tableColumn['COLUMN_DEFAULT']) ? null : $tableColumn['COLUMN_DEFAULT'], + 'default' => isset($tableColumn['COLUMN_DEFAULT']) ? $tableColumn['COLUMN_DEFAULT'] : null, 'autoincrement' => (bool)$tableColumn['IS_AUTO_INCREMENT'], 'scale' => (int)$tableColumn['NUMERIC_SCALE'], 'precision' => (int)$tableColumn['NUMERIC_PRECISION'], diff --git a/lib/Doctrine/DBAL/Schema/SQLAnywhereSchemaManager.php b/lib/Doctrine/DBAL/Schema/SQLAnywhereSchemaManager.php index 4eebef6d9b6..252869cbe3b 100644 --- a/lib/Doctrine/DBAL/Schema/SQLAnywhereSchemaManager.php +++ b/lib/Doctrine/DBAL/Schema/SQLAnywhereSchemaManager.php @@ -112,6 +112,10 @@ protected function _getPortableTableColumnDefinition($tableColumn) if ($tableColumn['default']) { // Strip quotes from default value. $default = preg_replace(array("/^'(.*)'$/", "/''/"), array("$1", "'"), $tableColumn['default']); + + if ('autoincrement' == $default) { + $default = null; + } } switch ($tableColumn['type']) { diff --git a/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php index 685ab886bc7..7f076caafb7 100644 --- a/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php +++ b/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php @@ -2,6 +2,8 @@ namespace Doctrine\Tests\DBAL\Functional\Schema; +use Doctrine\DBAL\Schema\Comparator; +use Doctrine\DBAL\Schema\Table; use Doctrine\DBAL\Types\Type, Doctrine\DBAL\Schema\AbstractSchemaManager; use Doctrine\DBAL\Platforms\AbstractPlatform; @@ -670,4 +672,54 @@ public function testListForeignKeysComposite() $this->assertEquals(array('id', 'foreign_key_test'), array_map('strtolower', $fkeys[0]->getLocalColumns())); $this->assertEquals(array('id', 'other_id'), array_map('strtolower', $fkeys[0]->getForeignColumns())); } + + /** + * @group DBAL-44 + */ + public function testColumnDefaultLifecycle() + { + $table = new Table("col_def_lifecycle"); + $table->addColumn('id', 'integer', array('primary' => true, 'autoincrement' => true)); + $table->addColumn('column1', 'string', array('default' => null)); + $table->addColumn('column2', 'string', array('default' => false)); + $table->addColumn('column3', 'string', array('default' => true)); + $table->addColumn('column4', 'string', array('default' => 0)); + $table->addColumn('column5', 'string', array('default' => '')); + $table->addColumn('column6', 'string', array('default' => 'def')); + $table->setPrimaryKey(array('id')); + + $this->_sm->dropAndCreateTable($table); + + $columns = $this->_sm->listTableColumns('col_def_lifecycle'); + + $this->assertNull($columns['id']->getDefault()); + $this->assertNull($columns['column1']->getDefault()); + $this->assertSame('', $columns['column2']->getDefault()); + $this->assertSame('1', $columns['column3']->getDefault()); + $this->assertSame('0', $columns['column4']->getDefault()); + $this->assertSame('', $columns['column5']->getDefault()); + $this->assertSame('def', $columns['column6']->getDefault()); + + $diffTable = clone $table; + + $diffTable->changeColumn('column1', array('default' => false)); + $diffTable->changeColumn('column2', array('default' => null)); + $diffTable->changeColumn('column3', array('default' => false)); + $diffTable->changeColumn('column4', array('default' => null)); + $diffTable->changeColumn('column5', array('default' => false)); + $diffTable->changeColumn('column6', array('default' => 666)); + + $comparator = new Comparator(); + + $this->_sm->alterTable($comparator->diffTable($table, $diffTable)); + + $columns = $this->_sm->listTableColumns('col_def_lifecycle'); + + $this->assertSame('', $columns['column1']->getDefault()); + $this->assertNull($columns['column2']->getDefault()); + $this->assertSame('', $columns['column3']->getDefault()); + $this->assertNull($columns['column4']->getDefault()); + $this->assertSame('', $columns['column5']->getDefault()); + $this->assertSame('666', $columns['column6']->getDefault()); + } } diff --git a/tests/Doctrine/Tests/DBAL/Platforms/PostgreSqlPlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/PostgreSqlPlatformTest.php index fdfca6ed912..bd43a0d0dbf 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/PostgreSqlPlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/PostgreSqlPlatformTest.php @@ -33,10 +33,10 @@ public function getGenerateAlterTableSql() 'ALTER TABLE mytable ADD quota INT DEFAULT NULL', 'ALTER TABLE mytable DROP foo', 'ALTER TABLE mytable ALTER bar TYPE VARCHAR(255)', - "ALTER TABLE mytable ALTER bar SET DEFAULT 'def'", + "ALTER TABLE mytable ALTER bar SET DEFAULT 'def'", 'ALTER TABLE mytable ALTER bar SET NOT NULL', 'ALTER TABLE mytable ALTER bloo TYPE BOOLEAN', - "ALTER TABLE mytable ALTER bloo SET DEFAULT 'false'", + "ALTER TABLE mytable ALTER bloo SET DEFAULT 'false'", 'ALTER TABLE mytable ALTER bloo SET NOT NULL', 'ALTER TABLE mytable RENAME TO userlist', ); diff --git a/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php b/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php index dcd53220ec2..54e51f75f03 100644 --- a/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php +++ b/tests/Doctrine/Tests/DBAL/Platforms/SQLServerPlatformTest.php @@ -37,6 +37,7 @@ public function getGenerateAlterTableSql() 'ALTER TABLE mytable ALTER COLUMN baz NVARCHAR(255) NOT NULL', "ALTER TABLE mytable ADD CONSTRAINT DF_6B2BD609_78240498 DEFAULT 'def' FOR baz", 'ALTER TABLE mytable ALTER COLUMN bloo BIT NOT NULL', + "ALTER TABLE mytable ADD CONSTRAINT DF_6B2BD609_CECED971 DEFAULT '0' FOR bloo", "sp_RENAME 'mytable', 'userlist'", "DECLARE @sql NVARCHAR(MAX) = N''; " . "SELECT @sql += N'EXEC sp_rename N''' + dc.name + ''', N''' " .