[2.0] DDC-169 - Fix several complications in update and drop schema code.

bin/doctrine.php

@@ -5,5 +5,5 @@ require 'Doctrine/Common/GlobalClassLoader.php';
 $classLoader = new \Doctrine\Common\GlobalClassLoader();
 $classLoader->register();
 
-$cli = new \Doctrine\ORM\Tools\Cli();
+$cli = new \Doctrine\ORM\Tools\Cli\CliController();
 $cli->run($_SERVER['argv']);
\ No newline at end of file

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -1655,6 +1655,14 @@ abstract class AbstractPlatform
         return false;
     }
 
+    /**
+     * @return bool
+     */
+    public function createsExplicitIndexForForeignKeys()
+    {
+        return false;
+    }
+
     /**
      * Whether the platform supports getting the affected rows of a recent
      * update/delete type query.

lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

@@ -771,6 +771,12 @@ class MySqlPlatform extends AbstractPlatform
      */
     public function getDropTableSql($table)
     {
+        if ($table instanceof \Doctrine\DBAL\Schema\Table) {
+            $table = $table->getName();
+        } else if(!is_string($table)) {
+            throw new \InvalidArgumentException('MysqlPlatform::getDropTableSql() expects $table parameter to be string or \Doctrine\DBAL\Schema\Table.');
+        }
+
         return 'DROP TABLE ' . $table;
     }
 

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

@@ -566,6 +566,16 @@ class PostgreSqlPlatform extends AbstractPlatform
         return 'DROP SEQUENCE ' . $sequence;
     }
 
+    /**
+     * @param  ForeignKeyConstraint|string $foreignKey
+     * @param  Table|string $table
+     * @return string
+     */
+    public function getDropForeignKeySql($foreignKey, $table)
+    {
+        return $this->getDropConstraintSql($foreignKey, $table);
+    }
+    
     /**
      * Gets the SQL used to create a table.
      *

lib/Doctrine/DBAL/Schema/Comparator.php

@@ -70,11 +70,13 @@ class Comparator
      *
      * @return SchemaDiff
      */
-    public function compare( Schema $fromSchema, Schema $toSchema )
+    public function compare(Schema $fromSchema, Schema $toSchema)
     {
         $diff = new SchemaDiff();
 
-        foreach ( $toSchema->getTables() as $tableName => $table ) {
+        $foreignKeysToTable = array();
+
+        foreach ( $toSchema->getTables() AS $tableName => $table ) {
             if ( !$fromSchema->hasTable($tableName) ) {
                 $diff->newTables[$tableName] = $table;
             } else {
@@ -86,10 +88,25 @@ class Comparator
         }
 
         /* Check if there are tables removed */
-        foreach ( $fromSchema->getTables() as $tableName => $table ) {
+        foreach ( $fromSchema->getTables() AS $tableName => $table ) {
             if ( !$toSchema->hasTable($tableName) ) {
                 $diff->removedTables[$tableName] = $table;
             }
+
+            // also remember all foreign keys that point to a specific table
+            foreach ($table->getForeignKeys() AS $foreignKey) {
+                $foreignTable = strtolower($foreignKey->getForeignTableName());
+                if (!isset($foreignKeysToTable[$foreignTable])) {
+                    $foreignKeysToTable[$foreignTable] = array();
+                }
+                $foreignKeysToTable[$foreignTable][] = $foreignKey;
+            }
+        }
+
+        foreach ($diff->removedTables AS $tableName => $table) {
+            if (isset($foreignKeysToTable[$tableName])) {
+                $diff->orphanedForeignKeys = array_merge($diff->orphanedForeignKeys, $foreignKeysToTable[$tableName]);
+            }
         }
 
         foreach ( $toSchema->getSequences() AS $sequenceName => $sequence) {
@@ -116,7 +133,7 @@ class Comparator
      * @param Sequence $sequence1
      * @param Sequence $sequence2
      */
-    public function diffSequence($sequence1, $sequence2)
+    public function diffSequence(Sequence $sequence1, Sequence $sequence2)
     {
         if($sequence1->getAllocationSize() != $sequence2->getAllocationSize()) {
             return true;
@@ -139,7 +156,7 @@ class Comparator
      *
      * @return bool|TableDiff
      */
-    public function diffTable( Table $table1, Table $table2 )
+    public function diffTable(Table $table1, Table $table2)
     {
         $changes = 0;
         $tableDifferences = new TableDiff($table1->getName());
@@ -249,7 +266,7 @@ class Comparator
      * @param ForeignKeyConstraint $key2
      * @return bool
      */
-    public function diffForeignKey($key1, $key2)
+    public function diffForeignKey(ForeignKeyConstraint $key1, ForeignKeyConstraint $key2)
     {
         if ($key1->getLocalColumns() != $key2->getLocalColumns()) {
             return true;
@@ -289,7 +306,7 @@ class Comparator
      *
      * @return array
      */
-    public function diffColumn( Column $column1, Column $column2 )
+    public function diffColumn(Column $column1, Column $column2)
     {
         $changedProperties = array();
         if ( $column1->getType() != $column2->getType() ) {
@@ -350,7 +367,7 @@ class Comparator
      * @param Index $index2
      * @return bool
      */
-    public function diffIndex( Index $index1, Index $index2 )
+    public function diffIndex(Index $index1, Index $index2)
     {
         if($index1->isPrimary() != $index2->isPrimary()) {
             return true;

lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php

@@ -25,6 +25,11 @@ use Doctrine\DBAL\Schema\Visitor\Visitor;
 
 class ForeignKeyConstraint extends AbstractAsset implements Constraint
 {
+    /**
+     * @var Table
+     */
+    protected $_localTable;
+
     /**
      * @var array
      */
@@ -67,6 +72,22 @@ class ForeignKeyConstraint extends AbstractAsset implements Constraint
         $this->_options = $options;
     }
 
+    /**
+     * @return string
+     */
+    public function getLocalTableName()
+    {
+        return $this->_localTable->getName();
+    }
+
+    /**
+     * @param Table $table
+     */
+    public function setLocalTable(Table $table)
+    {
+        $this->_localTable = $table;
+    }
+
     /**
      * @return array
      */

lib/Doctrine/DBAL/Schema/SchemaDiff.php

@@ -72,6 +72,11 @@ class SchemaDiff
      */
     public $removedSequences = array();
 
+    /**
+     * @var array
+     */
+    public $orphanedForeignKeys = array();
+
     /**
      * Constructs an SchemaDiff object.
      *
@@ -87,24 +92,57 @@ class SchemaDiff
     }
 
     /**
-     * @param Schema $fromSchema
-     * @param Schema $toSchema
+     * The to save sql mode ensures that the following things don't happen:
+     *
+     * 1. Tables are deleted
+     * 2. Sequences are deleted
+     * 3. Foreign Keys which reference tables that would otherwise be deleted.
+     *
+     * This way it is ensured that assets are deleted which might not be relevant to the metadata schema at all.
+     *
+     * @param AbstractPlatform $platform
+     * @return array
+     */
+    public function toSaveSql(AbstractPlatform $platform)
+    {
+        return $this->_toSql($platform, true);
+    }
+
+    /**
      * @param AbstractPlatform $platform
      * @return array
      */
     public function toSql(AbstractPlatform $platform)
+    {
+        return $this->_toSql($platform, false);
+    }
+
+    /**
+     * @param AbstractPlatform $platform
+     * @param bool $saveMode
+     * @return array
+     */
+    protected function _toSql(AbstractPlatform $platform, $saveMode = false)
     {
         $sql = array();
 
+        if ($platform->supportsForeignKeyConstraints()) {
+            foreach ($this->orphanedForeignKeys AS $orphanedForeignKey) {
+                $sql[] = $platform->getDropForeignKeySql($orphanedForeignKey, $orphanedForeignKey->getLocalTableName());
+            }
+        }
+
         if ($platform->supportsSequences() == true) {
             foreach ($this->changedSequences AS $sequence) {
                 $sql[] = $platform->getDropSequenceSql($sequence);
                 $sql[] = $platform->getCreateSequenceSql($sequence);
             }
 
+            if ($saveMode === false) {
                 foreach ($this->removedSequences AS $sequence) {
                     $sql[] = $platform->getDropSequenceSql($sequence);
                 }
+            }
 
             foreach ($this->newSequences AS $sequence) {
                 $sql[] = $platform->getCreateSequenceSql($sequence);
@@ -118,9 +156,11 @@ class SchemaDiff
             );
         }
 
+        if ($saveMode === true) {
             foreach ($this->removedTables AS $table) {
                 $sql[] = $platform->getDropTableSql($table);
             }
+        }
 
         foreach ($this->changedTables AS $tableDiff) {
             $sql = array_merge($sql, $platform->getAlterTableSql($tableDiff));

lib/Doctrine/DBAL/Schema/SchemaException.php

@@ -114,4 +114,14 @@ class SchemaException extends \Doctrine\DBAL\DBALException
     {
         return new self("Invalid case mode given to Schema Asset.");
     }
+
+    static public function namedForeignKeyRequired($localTable, $foreignKey)
+    {
+        return new self(
+            "The performed schema operation on ".$localTable->getName()." requires a named foreign key, ".
+            "but the given foreign key from (".implode(", ", $foreignKey->getColumns()).") onto foreign table ".
+            "'".$foreignKey->getForeignTableName()."' (".implode(", ", $foreignKey->getForeignColumns()).") is currently ".
+            "unnamed."
+        );
+    }
 }
\ No newline at end of file

lib/Doctrine/DBAL/Schema/Table.php

@@ -144,7 +144,9 @@ class Table extends AbstractAsset
     public function addIndex(array $columnNames, $indexName=null)
     {
         if($indexName == null) {
-            $indexName = $this->_generateIdentifierName($columnNames, "idx");
+            $indexName = $this->_generateIdentifierName(
+                array_merge(array($this->getName()), $columnNames), "idx"
+            );
         }
 
         return $this->_createIndex($columnNames, $indexName, false, false);
@@ -159,7 +161,9 @@ class Table extends AbstractAsset
     public function addUniqueIndex(array $columnNames, $indexName=null)
     {
         if ($indexName == null) {
-            $indexName = $this->_generateIdentifierName($columnNames, "uniq");
+            $indexName = $this->_generateIdentifierName(
+                array_merge(array($this->getName()), $columnNames), "uniq"
+            );
         }
 
         return $this->_createIndex($columnNames, $indexName, true, false);
@@ -318,6 +322,7 @@ class Table extends AbstractAsset
             $localColumnNames, $foreignTableName, $foreignColumnNames, $name, $options
         );
         $this->_addForeignKeyConstraint($constraint);
+
         return $this;
     }
 
@@ -355,6 +360,14 @@ class Table extends AbstractAsset
      */
     protected function _addIndex(Index $index)
     {
+        // check for duplicates
+        $c = new Comparator();
+        foreach ($this->_indexes AS $existingIndex) {
+            if ($c->diffIndex($index, $existingIndex) == false) {
+                return $this;
+            }
+        }
+
         $indexName = $index->getName();
         $indexName = strtolower($indexName);
 
@@ -375,6 +388,8 @@ class Table extends AbstractAsset
      */
     protected function _addForeignKeyConstraint(ForeignKeyConstraint $constraint)
     {
+        $constraint->setLocalTable($this);
+        
         if(strlen($constraint->getName())) {
             $name = $constraint->getName();
         } else {

lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php

@@ -83,7 +83,7 @@ class DropSchemaSqlCollector implements Visitor
      */
     public function acceptTable(Table $table)
     {
-        $this->_tables = array_merge($this->_tables, $this->_platform->getDropTableSql($table->getName()));
+        $this->_tables[] = $this->_platform->getDropTableSql($table->getName());
     }
 
     /**
@@ -100,11 +100,11 @@ class DropSchemaSqlCollector implements Visitor
      */
     public function acceptForeignKey(Table $localTable, ForeignKeyConstraint $fkConstraint)
     {
-        $this->_constraints = array_merge($this->_constraints,
-            $this->_platform->getDropForeignKeySql(
-                $localTable->getName(), $fkConstraint->getName()
-            )
-        );
+        if (strlen($fkConstraint->getName()) == 0) {
+            throw SchemaException::namedForeignKeyRequired($localTable, $fkConstraint);
+        }
+
+        $this->_constraints[] = $this->_platform->getDropForeignKeySql($fkConstraint->getName(), $localTable->getName());
     }
 
     /**
@@ -121,10 +121,7 @@ class DropSchemaSqlCollector implements Visitor
      */
     public function acceptSequence(Sequence $sequence)
     {
-        $this->_sequences = array_merge(
-            $this->_sequences, 
-            $this->_platform->getDropSequenceSql($sequence->getName())
-        );
+        $this->_sequences[] = $this->_platform->getDropSequenceSql($sequence->getName());
     }
 
     /**

lib/Doctrine/ORM/Tools/SchemaTool.php

@@ -446,28 +446,15 @@ class SchemaTool
      * @param string $mode
      * @return array
      */
-    public function getDropSchemaSql(array $classes, $mode=self::DROP_METADATA)
+    public function getDropSchemaSql(array $classes)
     {
-        if($mode == self::DROP_METADATA) {
-            $tables = $this->_getDropSchemaTablesMetadataMode($classes);
-        } else if($mode == self::DROP_DATABASE) {
-            $tables = $this->_getDropSchemaTablesDatabaseMode($classes);
-        } else {
-            throw new \Doctrine\ORM\ORMException("Given Drop Schema Mode is not supported.");
-        }
-
         $sm = $this->_em->getConnection()->getSchemaManager();
-        /* @var $sm \Doctrine\DBAL\Schema\AbstractSchemaManager */
-        $allTables = $sm->listTables();
-        
-        $sql = array();
-        foreach($tables AS $tableName) {
-            if(in_array($tableName, $allTables)) {
-                $sql[] = $this->_platform->getDropTableSql($tableName);
-            }
-        }
+        $schema = $sm->createSchema();
 
-        return $sql;
+        $visitor = new \Doctrine\DBAL\Schema\Visitor\DropSchemaSqlCollector($this->_platform);
+        /* @var $schema \Doctrine\DBAL\Schema\Schema */
+        $schema->visit($visitor);
+        return $visitor->getQueries();
     }
 
     /**

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

@@ -109,8 +109,8 @@ class TableTest extends \PHPUnit_Framework_TestCase
         $table->addIndex(array("foo", "bar", "baz"));
         $table->addUniqueIndex(array("foo", "bar", "baz"));
 
-        $this->assertTrue($table->hasIndex("foo_bar_baz_idx"));
-        $this->assertTrue($table->hasIndex("foo_bar_baz_uniq"));
+        $this->assertTrue($table->hasIndex("foo_foo_bar_baz_idx"));
+        $this->assertTrue($table->hasIndex("foo_foo_bar_baz_uniq"));
     }
 
     public function testIndexCaseInsensitive()
@@ -158,10 +158,10 @@ class TableTest extends \PHPUnit_Framework_TestCase
         $this->setExpectedException("Doctrine\DBAL\Schema\SchemaException");
 
         $type = \Doctrine\DBAL\Types\Type::getType('integer');
-        $columns = array(new Column("foo", $type));
+        $columns = array(new Column("foo", $type), new Column("bar", $type));
         $indexes = array(
             new Index("the_primary", array("foo"), true, true),
-            new Index("other_primary", array("foo"), true, true),
+            new Index("other_primary", array("bar"), true, true),
         );
         $table = new Table("foo", $columns, $indexes, array());
     }
@@ -171,10 +171,10 @@ class TableTest extends \PHPUnit_Framework_TestCase
         $this->setExpectedException("Doctrine\DBAL\Schema\SchemaException");
 
         $type = \Doctrine\DBAL\Types\Type::getType('integer');
-        $columns = array(new Column("foo", $type));
+        $columns = array(new Column("foo", $type), new Column("bar", $type));
         $indexes = array(
             new Index("an_idx", array("foo"), false, false),
-            new Index("an_idx", array("foo"), false, false),
+            new Index("an_idx", array("bar"), false, false),
         );
         $table = new Table("foo", $columns, $indexes, array());
     }

tests/Doctrine/Tests/DBAL/Schema/Visitor/SchemaSqlCollectorTest.php

@@ -41,13 +41,13 @@ class SchemaSqlCollectorTest extends \PHPUnit_Framework_TestCase
         );
         $platformMock->expects($this->exactly(2))
                      ->method('getDropTableSql')
-                     ->will($this->returnValue(array("tbl")));
+                     ->will($this->returnValue("tbl"));
         $platformMock->expects($this->exactly(1))
                      ->method('getDropSequenceSql')
-                     ->will($this->returnValue(array("seq")));
+                     ->will($this->returnValue("seq"));
         $platformMock->expects($this->exactly(1))
                      ->method('getDropForeignKeySql')
-                     ->will($this->returnValue(array("fk")));
+                     ->will($this->returnValue("fk"));
 
         $schema = $this->createFixtureSchema();