Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[DBAL-44] Fix column default value lifecycle #456

Merged
merged 1 commit into from

4 participants

@deeky666
Collaborator

This is an approach to make the "lifecycle" of a column's default value more consistent and less error prone. It also fixes some platform implementations, especially when a column's default value has to be dropped (which requires an explicit drop clause on some vendors).
This implementation implies a default value of null as no default value set. All other values get evaluated as a default value.

@doctrinebot
Collaborator

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-720

We use Jira to track the state of pull requests and the versions they got
included in.

@beberlei beberlei merged commit 8fe7410 into from
@AdamWill

Can this go to 2.4 branch, or is it too large? Owncloud is carrying an earlier version / subset of this fix downstream:

owncloud/3rdparty@12d8399

we (Fedora) would be happy if we can get all of OC's downstream patches to DBAL merged into the 2.4 branch, then update our DBAL to 2.4 and have OC use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 22, 2013
  1. @deeky666
This page is out of date. Refresh to see the latest.
View
5 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;
}
View
10 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;
}
}
View
14 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);
}
View
10 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';
}
View
2  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'],
View
4 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']) {
View
52 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());
+ }
}
View
4 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',
);
View
1  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''' " .
Something went wrong with that request. Please try again.