Merge pull request #692 from deeky666/DBAL-1009

[DBAL-1009] Fix column comment lifecycle

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -1574,8 +1574,10 @@ abstract class AbstractPlatform
         $sql = $this->_getCreateTableSQL($tableName, $columns, $options);
         if ($this->supportsCommentOnStatement()) {
             foreach ($table->getColumns() as $column) {
-                if ($this->getColumnComment($column)) {
-                    $sql[] = $this->getCommentOnColumnSQL($tableName, $column->getQuotedName($this), $this->getColumnComment($column));
+                $comment = $this->getColumnComment($column);
+
+                if (null !== $comment && '' !== $comment) {
+                    $sql[] = $this->getCommentOnColumnSQL($tableName, $column->getQuotedName($this), $comment);
                 }
             }
         }
@@ -2205,7 +2207,7 @@ abstract class AbstractPlatform
             $columnDef = $typeDecl . $charset . $default . $notnull . $unique . $check . $collation;
         }
 
-        if ($this->supportsInlineColumnComments() && isset($field['comment']) && $field['comment']) {
+        if ($this->supportsInlineColumnComments() && isset($field['comment']) && $field['comment'] !== '') {
             $columnDef .= " COMMENT " . $this->quoteStringLiteral($field['comment']);
         }
 

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

@@ -454,7 +454,10 @@ class PostgreSqlPlatform extends AbstractPlatform
 
             $query = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $column->toArray());
             $sql[] = 'ALTER TABLE ' . $diff->getName()->getQuotedName($this) . ' ' . $query;
-            if ($comment = $this->getColumnComment($column)) {
+
+            $comment = $this->getColumnComment($column);
+
+            if (null !== $comment && '' !== $comment) {
                 $commentsSQL[] = $this->getCommentOnColumnSQL($diff->name, $column->getName(), $comment);
             }
         }
@@ -817,7 +820,7 @@ class PostgreSqlPlatform extends AbstractPlatform
             }
         );
     }
-    
+
     /**
      * {@inheritDoc}
      */
@@ -825,8 +828,8 @@ class PostgreSqlPlatform extends AbstractPlatform
     {
         if (in_array(strtolower($item), $this->booleanLiterals['false'], true)) {
             return false;
-        } 
-          
+        }
+
         return parent::convertFromBoolean($item);
     }
 

lib/Doctrine/DBAL/Platforms/SQLAnywherePlatform.php

@@ -143,7 +143,7 @@ class SQLAnywherePlatform extends AbstractPlatform
 
             $comment = $this->getColumnComment($column);
 
-            if ($comment) {
+            if (null !== $comment && '' !== $comment) {
                 $commentsSQL[] = $this->getCommentOnColumnSQL($diff->name, $column->getQuotedName($this), $comment);
             }
         }

lib/Doctrine/DBAL/Schema/Comparator.php

@@ -421,8 +421,11 @@ class Comparator
             }
         }
 
-        // only allow to delete comment if its set to '' not to null.
-        if ($properties1['comment'] !== null && $properties1['comment'] != $properties2['comment']) {
+        // A null value and an empty string are actually equal for a comment so they should not trigger a change.
+        if ($properties1['comment'] !== $properties2['comment'] &&
+            ! (null === $properties1['comment'] && '' === $properties2['comment']) &&
+            ! (null === $properties2['comment'] && '' === $properties1['comment'])
+        ) {
             $changedProperties[] = 'comment';
         }
 

lib/Doctrine/DBAL/Schema/DrizzleSchemaManager.php

@@ -46,7 +46,9 @@ class DrizzleSchemaManager extends AbstractSchemaManager
             'autoincrement' => (bool)$tableColumn['IS_AUTO_INCREMENT'],
             'scale' => (int)$tableColumn['NUMERIC_SCALE'],
             'precision' => (int)$tableColumn['NUMERIC_PRECISION'],
-            'comment' => (isset($tableColumn['COLUMN_COMMENT']) ? $tableColumn['COLUMN_COMMENT'] : null),
+            'comment' => isset($tableColumn['COLUMN_COMMENT']) && '' !== $tableColumn['COLUMN_COMMENT']
+                ? $tableColumn['COLUMN_COMMENT']
+                : null,
         );
 
         $column = new Column($tableColumn['COLUMN_NAME'], Type::getType($type), $options);

lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php

@@ -187,7 +187,9 @@ class MySqlSchemaManager extends AbstractSchemaManager
             'scale'         => null,
             'precision'     => null,
             'autoincrement' => (bool) (strpos($tableColumn['extra'], 'auto_increment') !== false),
-            'comment'       => isset($tableColumn['comment']) ? $tableColumn['comment'] : null,
+            'comment'       => isset($tableColumn['comment']) && $tableColumn['comment'] !== ''
+                ? $tableColumn['comment']
+                : null,
         );
 
         if ($scale !== null && $precision !== null) {

lib/Doctrine/DBAL/Schema/OracleSchemaManager.php

@@ -205,7 +205,9 @@ class OracleSchemaManager extends AbstractSchemaManager
             'length'     => $length,
             'precision'  => $precision,
             'scale'      => $scale,
-            'comment'       => (isset($tableColumn['comments'])) ? $tableColumn['comments'] : null,
+            'comment'    => isset($tableColumn['comments']) && '' !== $tableColumn['comments']
+                ? $tableColumn['comments']
+                : null,
             'platformDetails' => array(),
         );
 

lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php

@@ -436,7 +436,9 @@ class PostgreSqlSchemaManager extends AbstractSchemaManager
             'fixed'         => $fixed,
             'unsigned'      => false,
             'autoincrement' => $autoincrement,
-            'comment'       => $tableColumn['comment'],
+            'comment'       => isset($tableColumn['comment']) && $tableColumn['comment'] !== ''
+                ? $tableColumn['comment']
+                : null,
         );
 
         $column = new Column($tableColumn['field'], Type::getType($type), $options);

lib/Doctrine/DBAL/Schema/SQLAnywhereSchemaManager.php

@@ -144,7 +144,9 @@ class SQLAnywhereSchemaManager extends AbstractSchemaManager
                 'notnull'       => (bool) $tableColumn['notnull'],
                 'default'       => $default,
                 'autoincrement' => (bool) $tableColumn['autoincrement'],
-                'comment'       => $tableColumn['comment']
+                'comment'       => isset($tableColumn['comment']) && '' !== $tableColumn['comment']
+                    ? $tableColumn['comment']
+                    : null,
         ));
     }
 

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

@@ -887,4 +887,71 @@ class SchemaManagerFunctionalTestCase extends \Doctrine\Tests\DbalFunctionalTest
         $columns = $this->_sm->listTableColumns("my_table");
         $this->assertEquals("It's a comment with a quote", $columns['id']->getComment());
     }
+
+    /**
+     * @group DBAL-1009
+     *
+     * @dataProvider getAlterColumnComment
+     */
+    public function testAlterColumnComment($comment1, $expectedComment1, $comment2, $expectedComment2)
+    {
+        if ( ! $this->_conn->getDatabasePlatform()->supportsInlineColumnComments() &&
+            ! $this->_conn->getDatabasePlatform()->supportsCommentOnStatement() &&
+            $this->_conn->getDatabasePlatform()->getName() != 'mssql') {
+            $this->markTestSkipped('Database does not support column comments.');
+        }
+
+        $offlineTable = new Table('alter_column_comment_test');
+        $offlineTable->addColumn('comment1', 'integer', array('comment' => $comment1));
+        $offlineTable->addColumn('comment2', 'integer', array('comment' => $comment2));
+        $offlineTable->addColumn('no_comment1', 'integer');
+        $offlineTable->addColumn('no_comment2', 'integer');
+
+        $this->_sm->dropAndCreateTable($offlineTable);
+
+        $onlineTable = $this->_sm->listTableDetails("alter_column_comment_test");
+
+        $this->assertSame($expectedComment1, $onlineTable->getColumn('comment1')->getComment());
+        $this->assertSame($expectedComment2, $onlineTable->getColumn('comment2')->getComment());
+        $this->assertNull($onlineTable->getColumn('no_comment1')->getComment());
+        $this->assertNull($onlineTable->getColumn('no_comment2')->getComment());
+
+        $onlineTable->changeColumn('comment1', array('comment' => $comment2));
+        $onlineTable->changeColumn('comment2', array('comment' => $comment1));
+        $onlineTable->changeColumn('no_comment1', array('comment' => $comment1));
+        $onlineTable->changeColumn('no_comment2', array('comment' => $comment2));
+
+        $comparator = new Comparator();
+
+        $tableDiff = $comparator->diffTable($offlineTable, $onlineTable);
+
+        $this->assertInstanceOf('Doctrine\DBAL\Schema\TableDiff', $tableDiff);
+
+        $this->_sm->alterTable($tableDiff);
+
+        $onlineTable = $this->_sm->listTableDetails("alter_column_comment_test");
+
+        $this->assertSame($expectedComment2, $onlineTable->getColumn('comment1')->getComment());
+        $this->assertSame($expectedComment1, $onlineTable->getColumn('comment2')->getComment());
+        $this->assertSame($expectedComment1, $onlineTable->getColumn('no_comment1')->getComment());
+        $this->assertSame($expectedComment2, $onlineTable->getColumn('no_comment2')->getComment());
+    }
+
+    public function getAlterColumnComment()
+    {
+        return array(
+            array(null, null, ' ', ' '),
+            array(null, null, '0', '0'),
+            array(null, null, 'foo', 'foo'),
+
+            array('', null, ' ', ' '),
+            array('', null, '0', '0'),
+            array('', null, 'foo', 'foo'),
+
+            array(' ', ' ', '0', '0'),
+            array(' ', ' ', 'foo', 'foo'),
+
+            array('0', '0', 'foo', 'foo'),
+        );
+    }
 }

tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php

@@ -1122,4 +1122,52 @@ class ComparatorTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(array('notnull', 'default', 'comment'), $comparator->diffColumn($column1, $column2));
         $this->assertEquals(array('notnull', 'default', 'comment'), $comparator->diffColumn($column2, $column1));
     }
+
+    /**
+     * @group DBAL-1009
+     *
+     * @dataProvider getCompareColumnComments
+     */
+    public function testCompareColumnComments($comment1, $comment2, $equals)
+    {
+        $column1 = new Column('foo', Type::getType('integer'), array('comment' => $comment1));
+        $column2 = new Column('foo', Type::getType('integer'), array('comment' => $comment2));
+
+        $comparator = new Comparator();
+
+        $expectedDiff = $equals ? array() : array('comment');
+
+        $actualDiff = $comparator->diffColumn($column1, $column2);
+
+        $this->assertSame($expectedDiff, $actualDiff);
+
+        $actualDiff = $comparator->diffColumn($column2, $column1);
+
+        $this->assertSame($expectedDiff, $actualDiff);
+    }
+
+    public function getCompareColumnComments()
+    {
+        return array(
+            array(null, null, true),
+            array('', '', true),
+            array(' ', ' ', true),
+            array('0', '0', true),
+            array('foo', 'foo', true),
+
+            array(null, '', true),
+            array(null, ' ', false),
+            array(null, '0', false),
+            array(null, 'foo', false),
+
+            array('', ' ', false),
+            array('', '0', false),
+            array('', 'foo', false),
+
+            array(' ', '0', false),
+            array(' ', 'foo', false),
+
+            array('0', 'foo', false),
+        );
+    }
 }