DBAL-29 - Detect ambiguouties in column renamings of the diffTable() method, doing nothing instead

lib/Doctrine/DBAL/Schema/Comparator.php

@@ -178,16 +178,7 @@ class Comparator
             }
         }
 
-        // Try to find columns that only changed their name, rename operations maybe cheaper than add/drop
-        foreach ($tableDifferences->addedColumns AS $addedColumnName => $addedColumn) {
-            foreach ($tableDifferences->removedColumns AS $removedColumnName => $removedColumn) {
-                if (count($this->diffColumn($addedColumn, $removedColumn)) == 0) {
-                    $tableDifferences->renamedColumns[$removedColumn->getName()] = $addedColumn;
-                    unset($tableDifferences->addedColumns[$addedColumnName]);
-                    unset($tableDifferences->removedColumns[$removedColumnName]);
-                }
-            }
-        }
+        $this->detectColumnRenamings($tableDifferences);
 
         $table1Indexes = $table1->getIndexes();
         $table2Indexes = $table2->getIndexes();
@@ -250,6 +241,34 @@ class Comparator
         return $changes ? $tableDifferences : false;
     }
 
+    /**
+     * Try to find columns that only changed their name, rename operations maybe cheaper than add/drop
+     * however ambiguouties between different possibilites should not lead to renaming at all.
+     * 
+     * @param TableDiff $tableDifferences
+     */
+    private function detectColumnRenamings(TableDiff $tableDifferences)
+    {
+        $renameCandidates = array();
+        foreach ($tableDifferences->addedColumns AS $addedColumnName => $addedColumn) {
+            foreach ($tableDifferences->removedColumns AS $removedColumnName => $removedColumn) {
+                if (count($this->diffColumn($addedColumn, $removedColumn)) == 0) {
+                    $renameCandidates[$addedColumn->getName()][] = array($removedColumn, $addedColumn);
+                }
+            }
+        }
+
+        foreach ($renameCandidates AS $candidate => $candidateColumns) {
+            if (count($candidateColumns) == 1) {
+                list($removedColumn, $addedColumn) = $candidateColumns[0];
+
+                $tableDifferences->renamedColumns[$removedColumn->getName()] = $addedColumn;
+                unset($tableDifferences->addedColumns[$addedColumnName]);
+                unset($tableDifferences->removedColumns[$removedColumnName]);
+            }
+        }
+    }
+
     /**
      * @param ForeignKeyConstraint $key1
      * @param ForeignKeyConstraint $key2

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

@@ -581,6 +581,33 @@ class ComparatorTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('bar', $tableDiff->renamedColumns['foo']->getName());
     }
 
+    /**
+     * You can easily have ambiguouties in the column renaming. If these
+     * are detected no renaming should take place, instead adding and dropping
+     * should be used exclusively.
+     *
+     * @group DBAL-24
+     */
+    public function testDetectRenameColumnAmbiguous()
+    {
+        $tableA = new Table("foo");
+        $tableA->addColumn('foo', 'integer');
+        $tableA->addColumn('bar', 'integer');
+
+        $tableB = new Table("foo");
+        $tableB->addColumn('baz', 'integer');
+        
+        $c = new Comparator();
+        $tableDiff = $c->diffTable($tableA, $tableB);
+
+        $this->assertEquals(1, count($tableDiff->addedColumns), "'baz' should be added, not created through renaming!");
+        $this->assertArrayHasKey('baz', $tableDiff->addedColumns, "'baz' should be added, not created through renaming!");
+        $this->assertEquals(2, count($tableDiff->removedColumns), "'foo' and 'bar' should both be dropped, an ambigouty exists which one could be renamed to 'baz'.");
+        $this->assertArrayHasKey('foo', $tableDiff->removedColumns, "'foo' should be removed.");
+        $this->assertArrayHasKey('bar', $tableDiff->removedColumns, "'bar' should be removed.");
+        $this->assertEquals(0, count($tableDiff->renamedColumns), "no renamings should take place.");
+    }
+
     public function testDetectChangeIdentifierType()
     {
         $this->markTestSkipped('DBAL-2 was reopened, this test cannot work anymore.');