[2.0] DDC-169 - Finished ALTER Table TableDiff refactoring, adding code to...

[2.0] DDC-169 - Finished ALTER Table TableDiff refactoring, adding code to handle index and FK changes. Added a general functional test for alter table against all platforms.

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -794,6 +794,42 @@ abstract class AbstractPlatform
         throw DBALException::notSupported(__METHOD__);
     }
 
+    /**
+     * Common code for alter table statement generation that updates the changed Index and Foreign Key definitions.
+     *
+     * @param TableDiff $diff
+     * @return array
+     */
+    protected function _getAlterTableIndexForeignKeySql(TableDiff $diff)
+    {
+        $sql = array();
+        if ($this->supportsForeignKeyConstraints()) {
+            foreach ($diff->addedForeignKeys AS $foreignKey) {
+                $sql[] = $this->getCreateForeignKeySql($foreignKey, $diff->name);
+            }
+            foreach ($diff->removedForeignKeys AS $foreignKey) {
+                $sql[] = $this->getDropForeignKeySql($foreignKey, $diff->name);
+            }
+            foreach ($diff->changedForeignKeys AS $foreignKey) {
+                $sql[] = $this->getDropForeignKeySql($foreignKey, $diff->name);
+                $sql[] = $this->getCreateForeignKeySql($foreignKey, $diff->name);
+            }
+        }
+
+        foreach ($diff->addedIndexes AS $index) {
+            $sql[] = $this->getCreateIndexSql($index, $diff->name);
+        }
+        foreach ($diff->removedIndexes AS $index) {
+            $sql[] = $this->getDropIndexSql($index, $diff->name);
+        }
+        foreach ($diff->changedIndexes AS $index) {
+            $sql[] = $this->getDropIndexSql($index, $diff->name);
+            $sql[] = $this->getCreateIndexSql($index, $diff->name);
+        }
+
+        return $sql;
+    }
+
     /**
      * Get declaration of a number of fields in bulk
      *
@@ -955,7 +991,14 @@ abstract class AbstractPlatform
         $default = empty($field['notnull']) ? ' DEFAULT NULL' : '';
 
         if (isset($field['default'])) {
-            $default = ' DEFAULT ' . $field['default'];
+            $default = " DEFAULT '".$field['default']."'";
+            if (isset($field['type'])) {
+                if (in_array((string)$field['type'], array("Integer", "BigInteger", "SmallInteger"))) {
+                    $default = " DEFAULT ".$field['default'];
+                } else if ((string)$field['type'] == 'DateTime' && $field['default'] == $this->getCurrentTimestampSql()) {
+                    $default = " DEFAULT ".$this->getCurrentTimestampSql();
+                }
+            }
         }
         return $default;
     }
@@ -1541,6 +1584,11 @@ abstract class AbstractPlatform
         return true;
     }
 
+    public function supportsAlterTable()
+    {
+        return true;
+    }
+
     /**
      * Whether the platform supports transactions.
      *

lib/Doctrine/DBAL/Platforms/MsSqlPlatform.php

@@ -125,11 +125,11 @@ class MsSqlPlatform extends AbstractPlatform
                     . $this->getColumnDeclarationSql($column->getName(), $column->toArray());
         }
 
-        if (count($queryParts) == 0) {
-            return false;
+        $sql = $this->_getAlterTableIndexForeignKeySql($diff);
+        if (count($queryParts) > 0) {
+            $sql[] = 'ALTER TABLE ' . $diff->name . ' ' . implode(", ", $queryParts);
         }
-
-        return array('ALTER TABLE ' . $diff->name . ' ' . implode(", ", $queryParts));
+        return $sql;
     }
     
     /**

lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

@@ -620,11 +620,11 @@ class MySqlPlatform extends AbstractPlatform
                     . $this->getColumnDeclarationSql($column->getName(), $column->toArray());
         }
 
-        if (count($queryParts) == 0) {
-            return false;
+        $sql = $this->_getAlterTableIndexForeignKeySql($diff);
+        if (count($queryParts) > 0) {
+            $sql[] = 'ALTER TABLE ' . $diff->name . ' ' . implode(", ", $queryParts);
         }
-
-        return array('ALTER TABLE ' . $diff->name . ' ' . implode(", ", $queryParts));
+        return $sql;
     }
     
     /**

lib/Doctrine/DBAL/Platforms/OraclePlatform.php

@@ -319,7 +319,7 @@ class OraclePlatform extends AbstractPlatform
              "       uind_col.column_position AS column_pos, " .
              "       (SELECT ucon.constraint_type FROM user_constraints ucon WHERE ucon.constraint_name = uind.index_name) AS is_primary ".
              "FROM user_indexes uind, user_ind_columns uind_col " .
-             "WHERE uind.index_name = uind_col.index_name AND uind_col.table_name = '$table'";
+             "WHERE uind.index_name = uind_col.index_name AND uind_col.table_name = '$table' ORDER BY uind_col.column_position ASC";
     }
 
     public function getListTablesSql()
@@ -477,7 +477,7 @@ END;';
      */
     public function getAlterTableSql(TableDiff $diff)
     {
-        $sql = array();
+        $sql = $this->_getAlterTableIndexForeignKeySql($diff);
 
         $fields = array();
         foreach ($diff->addedColumns AS $column) {

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

@@ -493,7 +493,7 @@ class PostgreSqlPlatform extends AbstractPlatform
      */
     public function getAlterTableSql(TableDiff $diff)
     {
-        $sql = array();
+        $sql = $this->_getAlterTableIndexForeignKeySql($diff);
 
         foreach ($diff->addedColumns as $column) {
             $query = 'ADD ' . $this->getColumnDeclarationSql($column->getName(), $column->toArray());
@@ -517,7 +517,7 @@ class PostgreSqlPlatform extends AbstractPlatform
                 $sql[] = 'ALTER TABLE ' . $diff->name . ' ' . $query;
             }
             if ($columnDiff->hasChanged('default')) {
-                $query = 'ALTER ' . $oldColumnName . ' SET DEFAULT ' . $column->getDefault();
+                $query = 'ALTER ' . $oldColumnName . ' SET ' . $this->getDefaultValueDeclarationSql($column->toArray());
                 $sql[] = 'ALTER TABLE ' . $diff->name . ' ' . $query;
             }
             if ($columnDiff->hasChanged('notnull')) {

lib/Doctrine/DBAL/Platforms/SqlitePlatform.php

@@ -437,6 +437,11 @@ class SqlitePlatform extends AbstractPlatform
         return false;
     }
 
+    public function supportsAlterTable()
+    {
+        return false;
+    }
+
     public function supportsSequences()
     {
         return false;

lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php

@@ -241,7 +241,7 @@ abstract class AbstractSchemaManager
     /**
      * List the tables for this connection
      *
-     * @return array(int => Table)
+     * @return Table[]
      */
     public function listTables()
     {
@@ -267,12 +267,35 @@ abstract class AbstractSchemaManager
                 }
             }
 
-            $tables[] = new Table($tableName, $columns, $indexes, $foreignKeys, $idGeneratorType, array());
+            $tables[] = $this->listTableDetails($tableName);
         }
 
         return $tables;
     }
 
+    /**
+     * @param  string $tableName
+     * @return Table
+     */
+    public function listTableDetails($tableName)
+    {
+        $columns = $this->listTableColumns($tableName);
+        $foreignKeys = array();
+        if ($this->_platform->supportsForeignKeyConstraints()) {
+            $foreignKeys = $this->listTableForeignKeys($tableName);
+        }
+        $indexes = $this->listTableIndexes($tableName);
+
+        $idGeneratorType = Table::ID_NONE;
+        foreach ($columns AS $column) {
+            if ($column->hasPlatformOption('autoincrement') && $column->getPlatformOption('autoincrement')) {
+                $idGeneratorType = Table::ID_IDENTITY;
+            }
+        }
+
+        return new Table($tableName, $columns, $indexes, $foreignKeys, $idGeneratorType, array());
+    }
+
     /**
      * List the users this connection has
      *
@@ -575,95 +598,15 @@ abstract class AbstractSchemaManager
     /**
      * Alter an existing tables schema
      *
-     * @param string $name         name of the table that is intended to be changed.
-     * @param array $changes     associative array that contains the details of each type
-     *                             of change that is intended to be performed. The types of
-     *                             changes that are currently supported are defined as follows:
-     *
-     *                             name
-     *
-     *                                New name for the table.
-     *
-     *                            add
-     *
-     *                                Associative array with the names of fields to be added as
-     *                                 indexes of the array. The value of each entry of the array
-     *                                 should be set to another associative array with the properties
-     *                                 of the fields to be added. The properties of the fields should
-     *                                 be the same as defined by the MDB2 parser.
-     *
-     *
-     *                            remove
-     *
-     *                                Associative array with the names of fields to be removed as indexes
-     *                                 of the array. Currently the values assigned to each entry are ignored.
-     *                                 An empty array should be used for future compatibility.
-     *
-     *                            rename
-     *
-     *                                Associative array with the names of fields to be renamed as indexes
-     *                                 of the array. The value of each entry of the array should be set to
-     *                                 another associative array with the entry named name with the new
-     *                                 field name and the entry named Declaration that is expected to contain
-     *                                 the portion of the field declaration already in DBMS specific SQL code
-     *                                 as it is used in the CREATE TABLE statement.
-     *
-     *                            change
-     *
-     *                                Associative array with the names of the fields to be changed as indexes
-     *                                 of the array. Keep in mind that if it is intended to change either the
-     *                                 name of a field and any other properties, the change array entries
-     *                                 should have the new names of the fields as array indexes.
-     *
-     *                                The value of each entry of the array should be set to another associative
-     *                                 array with the properties of the fields to that are meant to be changed as
-     *                                 array entries. These entries should be assigned to the new values of the
-     *                                 respective properties. The properties of the fields should be the same
-     *                                 as defined by the MDB2 parser.
-     *
-     *                            Example
-     *                                array(
-     *                                    'name' => 'userlist',
-     *                                    'add' => array(
-     *                                        'quota' => array(
-     *                                            'type' => 'integer',
-     *                                            'unsigned' => 1
-     *                                        )
-     *                                    ),
-     *                                    'remove' => array(
-     *                                        'file_limit' => array(),
-     *                                        'time_limit' => array()
-     *                                    ),
-     *                                    'change' => array(
-     *                                        'name' => array(
-     *                                            'length' => '20',
-     *                                            'definition' => array(
-     *                                                'type' => 'text',
-     *                                                'length' => 20,
-     *                                            ),
-     *                                        )
-     *                                    ),
-     *                                    'rename' => array(
-     *                                        'sex' => array(
-     *                                            'name' => 'gender',
-     *                                            'definition' => array(
-     *                                                'type' => 'text',
-     *                                                'length' => 1,
-     *                                                'default' => 'M',
-     *                                            ),
-     *                                        )
-     *                                    )
-     *                                )
-     *
-     * @param boolean $check     indicates whether the function should just check if the DBMS driver
-     *                             can perform the requested table alterations if the value is true or
-     *                             actually perform them otherwise.
-     */
-    public function alterTable($name, array $changes, $check = false)
-    {
-        $queries = $this->_platform->getAlterTableSql($name, $changes, $check);
-        foreach($queries AS $ddlQuery) {
-            $this->_execSql($ddlQuery);
+     * @param TableDiff $tableDiff
+     */
+    public function alterTable(TableDiff $tableDiff)
+    {
+        $queries = $this->_platform->getAlterTableSql($tableDiff);
+        if (is_array($queries) && count($queries)) {
+            foreach ($queries AS $ddlQuery) {
+                $this->_execSql($ddlQuery);
+            }
         }
     }
 
@@ -675,10 +618,9 @@ abstract class AbstractSchemaManager
      */
     public function renameTable($name, $newName)
     {
-        $change = array(
-            'name' => $newName
-        );
-        $this->alterTable($name, $change);
+        $tableDiff = new TableDiff($name);
+        $tableDiff->newName = $newName;
+        $this->alterTable($tableDiff);
     }
 
     /**

lib/Doctrine/DBAL/Schema/Comparator.php

@@ -379,17 +379,4 @@ class Comparator
 
         return false;
     }
-
-    /**
-     * @param Schema $fromSchema
-     * @param Schema $toSchema
-     * @param AbstractSchemaManager $sm
-     * @return array
-     */
-    public function toSql(Schema $fromSchema, Schema $toSchema, AbstractSchemaManager $sm)
-    {
-        $diffSchema = $this->compare($fromSchema, $toSchema);
-
-
-    }
 }
\ No newline at end of file

lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php

@@ -128,19 +128,24 @@ class PostgreSqlSchemaManager extends AbstractSchemaManager
         foreach($tableIndexes AS $row) {
             $colNumbers = explode( ' ', $row['indkey'] );
             $colNumbersSql = 'IN (' . join( ' ,', $colNumbers ) . ' )';
-            $columnNameSql = "SELECT attname FROM pg_attribute
-                WHERE attrelid={$row['indrelid']} AND attnum $colNumbersSql;";
+            $columnNameSql = "SELECT attnum, attname FROM pg_attribute
+                WHERE attrelid={$row['indrelid']} AND attnum $colNumbersSql ORDER BY attnum ASC;";
                 
             $stmt = $this->_conn->execute($columnNameSql);
             $indexColumns = $stmt->fetchAll();
 
-            foreach ( $indexColumns as $colRow ) {
-                $buffer[] = array(
-                    'key_name' => $row['relname'],
-                    'column_name' => $colRow['attname'],
-                    'non_unique' => !$row['indisunique'],
-                    'primary' => $row['indisprimary']
-                );
+            // required for getting the order of the columns right.
+            foreach ($colNumbers AS $colNum) {
+                foreach ( $indexColumns as $colRow ) {
+                    if ($colNum == $colRow['attnum']) {
+                        $buffer[] = array(
+                            'key_name' => $row['relname'],
+                            'column_name' => $colRow['attname'],
+                            'non_unique' => !$row['indisunique'],
+                            'primary' => $row['indisprimary']
+                        );
+                    }
+                }
             }
         }
         return parent::_getPortableTableIndexesList($buffer);

lib/Doctrine/DBAL/Schema/SchemaDiff.php

@@ -21,6 +21,8 @@
 
 namespace Doctrine\DBAL\Schema;
 
+use \Doctrine\DBAL\Platforms\AbstractPlatform;
+
 /**
  * Schema Diff
  *
@@ -83,4 +85,47 @@ class SchemaDiff
         $this->changedTables = $changedTables;
         $this->removedTables = $removedTables;
     }
+
+    /**
+     * @param Schema $fromSchema
+     * @param Schema $toSchema
+     * @param AbstractPlatform $platform
+     * @return array
+     */
+    public function toSql(AbstractPlatform $platform)
+    {
+        $sql = array();
+
+        if ($platform->supportsSequences() == true) {
+            foreach ($this->changedSequences AS $sequence) {
+                $sql[] = $platform->getDropSequenceSql($sequence);
+                $sql[] = $platform->getCreateSequenceSql($sequence);
+            }
+
+            foreach ($this->removedSequences AS $sequence) {
+                $sql[] = $platform->getDropSequenceSql($sequence);
+            }
+
+            foreach ($this->newSequences AS $sequence) {
+                $sql[] = $platform->getCreateSequenceSql($sequence);
+            }
+        }
+
+        foreach ($this->newTables AS $table) {
+            $sql = array_merge(
+                $sql,
+                $platform->getCreateTableSql($table, AbstractPlatform::CREATE_FOREIGNKEYS|AbstractPlatform::CREATE_INDEXES)
+            );
+        }
+
+        foreach ($this->removedTables AS $table) {
+            $sql[] = $platform->getDropTableSql($table);
+        }
+
+        foreach ($this->changedTables AS $tableDiff) {
+            $sql = array_merge($sql, $platform->getAlterTableSql($tableDiff));
+        }
+
+        return $sql;
+    }
 }

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

@@ -286,6 +286,72 @@ class SchemaManagerFunctionalTestCase extends \Doctrine\Tests\DbalFunctionalTest
         $this->assertTrue($schema->hasTable('test_table'));
     }
 
+    public function testAlterTableScenario()
+    {
+        if(!$this->_sm->getDatabasePlatform()->supportsAlterTable()) {
+            $this->markTestSkipped('Alter Table is not supported by this platform.');
+        }
+
+        $this->createTestTable('alter_table');
+        $this->createTestTable('alter_table_foreign');
+
+        $table = $this->_sm->listTableDetails('alter_table');
+        $this->assertTrue($table->hasColumn('id'));
+        $this->assertTrue($table->hasColumn('test'));
+        $this->assertTrue($table->hasColumn('foreign_key_test'));
+        $this->assertEquals(0, count($table->getForeignKeys()));
+        $this->assertEquals(1, count($table->getIndexes()));
+
+        $tableDiff = new \Doctrine\DBAL\Schema\TableDiff("alter_table");
+        $tableDiff->addedColumns['foo'] = new \Doctrine\DBAL\Schema\Column('foo', Type::getType('integer'));
+        $tableDiff->removedColumns['test'] = $table->getColumn('test');
+
+        $this->_sm->alterTable($tableDiff);
+
+        $table = $this->_sm->listTableDetails('alter_table');
+        $this->assertFalse($table->hasColumn('test'));
+        $this->assertTrue($table->hasColumn('foo'));
+
+        $tableDiff = new \Doctrine\DBAL\Schema\TableDiff("alter_table");
+        $tableDiff->addedIndexes[] = new \Doctrine\DBAL\Schema\Index('foo_idx', array('foo'));
+
+        $this->_sm->alterTable($tableDiff);
+
+        $table = $this->_sm->listTableDetails('alter_table');
+        $this->assertEquals(2, count($table->getIndexes()));
+        $this->assertTrue($table->hasIndex('foo_idx'));
+        $this->assertEquals(array('foo'), array_map('strtolower', $table->getIndex('foo_idx')->getColumns()));
+        $this->assertFalse($table->getIndex('foo_idx')->isPrimary());
+        $this->assertFalse($table->getIndex('foo_idx')->isUnique());
+
+        $tableDiff = new \Doctrine\DBAL\Schema\TableDiff("alter_table");
+        $tableDiff->changedIndexes[] = new \Doctrine\DBAL\Schema\Index('foo_idx', array('foo', 'foreign_key_test'));
+
+        $this->_sm->alterTable($tableDiff);
+
+        $table = $this->_sm->listTableDetails('alter_table');
+        $this->assertEquals(2, count($table->getIndexes()));
+        $this->assertTrue($table->hasIndex('foo_idx'));
+        $this->assertEquals(array('foo', 'foreign_key_test'), array_map('strtolower', $table->getIndex('foo_idx')->getColumns()));
+
+        $tableDiff = new \Doctrine\DBAL\Schema\TableDiff("alter_table");
+        $tableDiff->removedIndexes[] = new \Doctrine\DBAL\Schema\Index('foo_idx', array('foo', 'foreign_key_test'));
+        $fk = new \Doctrine\DBAL\Schema\ForeignKeyConstraint(array('foreign_key_test'), 'alter_table_foreign', array('id'));
+        $tableDiff->addedForeignKeys[] = $fk;
+
+        $this->_sm->alterTable($tableDiff);
+        $table = $this->_sm->listTableDetails('alter_table');
+
+        // dont check for index size here, some platforms automatically add indexes for foreign keys.
+        $this->assertFalse($table->hasIndex('foo_idx'));
+
+        $this->assertEquals(1, count($table->getForeignKeys()));
+        $foreignKey = current($table->getForeignKeys());
+        $this->assertEquals('alter_table_foreign', strtolower($foreignKey->getForeignTableName()));
+        $this->assertEquals(array('foreign_key_test'), array_map('strtolower', $foreignKey->getColumns()));
+        $this->assertEquals(array('id'), array_map('strtolower', $foreignKey->getForeignColumns()));
+    }
+
     /**
      * @var \Doctrine\DBAL\Schema\AbstractSchemaManager
      */

tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php

@@ -91,19 +91,25 @@ abstract class AbstractPlatformTestCase extends \Doctrine\Tests\DbalTestCase
 
     abstract public function getGenerateAlterTableSql();
 
-    public function testGeneratesTableAlterationSqlForAddingAndRenaming()
+    public function testGeneratesTableAlterationSql()
     {
         $expectedSql = $this->getGenerateAlterTableSql();
 
+        $columnDiff = new \Doctrine\DBAL\Schema\ColumnDiff(
+            'bar', new \Doctrine\DBAL\Schema\Column(
+                'baz', \Doctrine\DBAL\Types\Type::getType('string'), array('default' => 'def')
+            ),
+            array('type', 'notnull', 'default')
+        );
+
         $tableDiff = new \Doctrine\DBAL\Schema\TableDiff('mytable');
         $tableDiff->newName = 'userlist';
         $tableDiff->addedColumns['quota'] = new \Doctrine\DBAL\Schema\Column('quota', \Doctrine\DBAL\Types\Type::getType('integer'), array('notnull' => false));
+        $tableDiff->removedColumns['foo'] = new \Doctrine\DBAL\Schema\Column('foo', \Doctrine\DBAL\Types\Type::getType('integer'));
+        $tableDiff->changedColumns['bar'] = $columnDiff;
 
         $sql = $this->_platform->getAlterTableSql($tableDiff);
 
-        $this->assertEquals(count($expectedSql), count($sql), "Expecting the same number of sql queries for alter table failed.");
-        for ($i = 0; $i < count($expectedSql); $i++) {
-            $this->assertEquals($expectedSql[$i], $sql[$i], $i."th query of alter table does not match.");
-        }
+        $this->assertEquals($expectedSql, $sql);
     }
 }

tests/Doctrine/Tests/DBAL/Platforms/MsSqlPlatformTest.php

@@ -22,7 +22,7 @@ class MsSqlPlatformTest extends AbstractPlatformTestCase
     public function getGenerateAlterTableSql()
     {
         return array(
-            'ALTER TABLE mytable RENAME TO userlist, ADD quota INT DEFAULT NULL',
+            "ALTER TABLE mytable RENAME TO userlist, ADD quota INT DEFAULT NULL, DROP foo, CHANGE bar baz VARCHAR(255) DEFAULT 'def' NOT NULL"
         );
     }
 

tests/Doctrine/Tests/DBAL/Platforms/MySqlPlatformTest.php

@@ -31,7 +31,7 @@ class MySqlPlatformTest extends AbstractPlatformTestCase
     public function getGenerateAlterTableSql()
     {
         return array(
-            'ALTER TABLE mytable RENAME TO userlist, ADD quota INT DEFAULT NULL',
+            "ALTER TABLE mytable RENAME TO userlist, ADD quota INT DEFAULT NULL, DROP foo, CHANGE bar baz VARCHAR(255) DEFAULT 'def' NOT NULL"
         );
     }
 

tests/Doctrine/Tests/DBAL/Platforms/OraclePlatformTest.php

@@ -23,7 +23,9 @@ class OraclePlatformTest extends AbstractPlatformTestCase
     {
         return array(
             'ALTER TABLE mytable ADD (quota NUMBER(10) DEFAULT NULL)',
-            'ALTER TABLE mytable RENAME TO userlist',
+            "ALTER TABLE mytable MODIFY (baz  VARCHAR2(255) DEFAULT 'def' NOT NULL)",
+            "ALTER TABLE mytable DROP COLUMN foo",
+            "ALTER TABLE mytable RENAME TO userlist",
         );
     }
 

tests/Doctrine/Tests/DBAL/Platforms/PostgreSqlPlatformTest.php

@@ -24,6 +24,10 @@ class PostgreSqlPlatformTest extends AbstractPlatformTestCase
     {
         return array(
             '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 NOT NULL',
             'ALTER TABLE mytable RENAME TO userlist',
         );
     }