Merge branch 'hotfix/#764-DDC-1102-backport-to-2.5' into 2.5

UPGRADE.md

+# Upgrade to 2.5.1
+
+## MINOR BC BREAK: Doctrine\DBAL\Schema\Table
+
+When adding indexes to ``Doctrine\DBAL\Schema\Table`` via ``addIndex()`` or ``addUniqueIndex()``,
+duplicate indexes are not silently ignored/dropped anymore (based on semantics, not naming!).
+Duplicate indexes are considered indexes that pass ``isFullfilledBy()`` or ``overrules()``
+in ``Doctrine\DBAL\Schema\Index``.
+This is required to make the index renaming feature introduced in 2.5.0 work properly and avoid
+issues in the ORM schema tool / DBAL schema manager which pretends users from updating
+their schemas and migrate to DBAL 2.5.*.
+Additionally it offers more flexibility in declaring indexes for the user and potentially fixes
+related issues in the ORM.
+With this change, the responsibility to decide which index is a "duplicate" is completely deferred
+to the user.
+Please also note that adding foreign key constraints to a table via ``addForeignKeyConstraint()``,
+``addUnnamedForeignKeyConstraint()`` or ``addNamedForeignKeyConstraint()`` now first checks if an
+appropriate index is already present and avoids adding an additional auto-generated one eventually.
+
 # Upgrade to 2.5
 
 ## BC BREAK: time type resets date fields to UNIX epoch

lib/Doctrine/DBAL/Schema/Comparator.php

@@ -238,36 +238,36 @@ class Comparator
         $table1Indexes = $table1->getIndexes();
         $table2Indexes = $table2->getIndexes();
 
-        foreach ($table2Indexes as $index2Name => $index2Definition) {
-            foreach ($table1Indexes as $index1Name => $index1Definition) {
-                if ($this->diffIndex($index1Definition, $index2Definition) === false) {
-                    if ( ! $index1Definition->isPrimary() && $index1Name != $index2Name) {
-                        $tableDifferences->renamedIndexes[$index1Name] = $index2Definition;
-                        $changes++;
+        /* See if all the indexes in table 1 exist in table 2 */
+        foreach ($table2Indexes as $indexName => $index) {
+            if (($index->isPrimary() && $table1->hasPrimaryKey()) || $table1->hasIndex($indexName)) {
+                continue;
             }
 
-                    unset($table1Indexes[$index1Name]);
-                    unset($table2Indexes[$index2Name]);
-                } else {
-                    if ($index1Name == $index2Name) {
-                        $tableDifferences->changedIndexes[$index2Name] = $table2Indexes[$index2Name];
-                        unset($table1Indexes[$index1Name]);
-                        unset($table2Indexes[$index2Name]);
+            $tableDifferences->addedIndexes[$indexName] = $index;
             $changes++;
         }
-                }
-            }
-        }
-
-        foreach ($table1Indexes as $index1Name => $index1Definition) {
-            $tableDifferences->removedIndexes[$index1Name] = $index1Definition;
+        /* See if there are any removed indexes in table 2 */
+        foreach ($table1Indexes as $indexName => $index) {
+            // See if index is removed in table 2.
+            if (($index->isPrimary() && ! $table2->hasPrimaryKey()) ||
+                ! $index->isPrimary() && ! $table2->hasIndex($indexName)
+            ) {
+                $tableDifferences->removedIndexes[$indexName] = $index;
                 $changes++;
+                continue;
             }
 
-        foreach ($table2Indexes as $index2Name => $index2Definition) {
-            $tableDifferences->addedIndexes[$index2Name] = $index2Definition;
+            // See if index has changed in table 2.
+            $table2Index = $index->isPrimary() ? $table2->getPrimaryKey() : $table2->getIndex($indexName);
+
+            if ($this->diffIndex($index, $table2Index)) {
+                $tableDifferences->changedIndexes[$indexName] = $table2Index;
                 $changes++;
             }
+        }
+
+        $this->detectIndexRenamings($tableDifferences);
 
         $fromFkeys = $table1->getForeignKeys();
         $toFkeys = $table2->getForeignKeys();
@@ -335,6 +335,47 @@ class Comparator
         }
     }
 
+    /**
+     * Try to find indexes that only changed their name, rename operations maybe cheaper than add/drop
+     * however ambiguities between different possibilities should not lead to renaming at all.
+     *
+     * @param \Doctrine\DBAL\Schema\TableDiff $tableDifferences
+     *
+     * @return void
+     */
+    private function detectIndexRenamings(TableDiff $tableDifferences)
+    {
+        $renameCandidates = array();
+
+        // Gather possible rename candidates by comparing each added and removed index based on semantics.
+        foreach ($tableDifferences->addedIndexes as $addedIndexName => $addedIndex) {
+            foreach ($tableDifferences->removedIndexes as $removedIndex) {
+                if (! $this->diffIndex($addedIndex, $removedIndex)) {
+                    $renameCandidates[$addedIndex->getName()][] = array($removedIndex, $addedIndex, $addedIndexName);
+                }
+            }
+        }
+
+        foreach ($renameCandidates as $candidateIndexes) {
+            // If the current rename candidate contains exactly one semantically equal index,
+            // we can safely rename it.
+            // Otherwise it is unclear if a rename action is really intended,
+            // therefore we let those ambiguous indexes be added/dropped.
+            if (count($candidateIndexes) === 1) {
+                list($removedIndex, $addedIndex) = $candidateIndexes[0];
+
+                $removedIndexName = strtolower($removedIndex->getName());
+                $addedIndexName = strtolower($addedIndex->getName());
+
+                if (! isset($tableDifferences->renamedIndexes[$removedIndexName])) {
+                    $tableDifferences->renamedIndexes[$removedIndexName] = $addedIndex;
+                    unset($tableDifferences->addedIndexes[$addedIndexName]);
+                    unset($tableDifferences->removedIndexes[$removedIndexName]);
+                }
+            }
+        }
+    }
+
     /**
      * @param \Doctrine\DBAL\Schema\ForeignKeyConstraint $key1
      * @param \Doctrine\DBAL\Schema\ForeignKeyConstraint $key2

lib/Doctrine/DBAL/Schema/Table.php

@@ -489,13 +489,6 @@ class Table extends AbstractAsset
      */
     protected function _addIndex(Index $indexCandidate)
     {
-        // check for duplicates
-        foreach ($this->_indexes as $existingIndex) {
-            if ($indexCandidate->isFullfilledBy($existingIndex)) {
-                return $this;
-            }
-        }
-
         $indexName = $indexCandidate->getName();
         $indexName = $this->normalizeIdentifier($indexName);
 
@@ -503,13 +496,6 @@ class Table extends AbstractAsset
             throw SchemaException::indexAlreadyExists($indexName, $this->_name);
         }
 
-        // remove overruled indexes
-        foreach ($this->_indexes as $idxKey => $existingIndex) {
-            if ($indexCandidate->overrules($existingIndex)) {
-                unset($this->_indexes[$idxKey]);
-            }
-        }
-
         if ($indexCandidate->isPrimary()) {
             $this->_primaryKeyName = $indexName;
         }
@@ -538,9 +524,23 @@ class Table extends AbstractAsset
         $name = $this->normalizeIdentifier($name);
 
         $this->_fkConstraints[$name] = $constraint;
+
         // add an explicit index on the foreign key columns. If there is already an index that fulfils this requirements drop the request.
         // In the case of __construct calling this method during hydration from schema-details all the explicitly added indexes
         // lead to duplicates. This creates computation overhead in this case, however no duplicate indexes are ever added (based on columns).
+        $indexName = $this->_generateIdentifierName(
+            array_merge(array($this->getName()), $constraint->getColumns()),
+            "idx",
+            $this->_getMaxIdentifierLength()
+        );
+        $indexCandidate = $this->_createIndex($constraint->getColumns(), $indexName, false, false);
+
+        foreach ($this->_indexes as $existingIndex) {
+            if ($indexCandidate->isFullfilledBy($existingIndex)) {
+                return;
+            }
+        }
+
         $this->addIndex($constraint->getColumns());
     }
 

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

@@ -708,6 +708,59 @@ class ComparatorTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(0, count($tableDiff->renamedColumns), "no renamings should take place.");
     }
 
+    /**
+     * @group DBAL-1063
+     */
+    public function testDetectRenameIndex()
+    {
+        $table1 = new Table('foo');
+        $table1->addColumn('foo', 'integer');
+
+        $table2 = clone $table1;
+
+        $table1->addIndex(array('foo'), 'idx_foo');
+
+        $table2->addIndex(array('foo'), 'idx_bar');
+
+        $comparator = new Comparator();
+        $tableDiff = $comparator->diffTable($table1, $table2);
+
+        $this->assertCount(0, $tableDiff->addedIndexes);
+        $this->assertCount(0, $tableDiff->removedIndexes);
+        $this->assertArrayHasKey('idx_foo', $tableDiff->renamedIndexes);
+        $this->assertEquals('idx_bar', $tableDiff->renamedIndexes['idx_foo']->getName());
+    }
+
+    /**
+     * You can easily have ambiguities in the index renaming. If these
+     * are detected no renaming should take place, instead adding and dropping
+     * should be used exclusively.
+     *
+     * @group DBAL-1063
+     */
+    public function testDetectRenameIndexAmbiguous()
+    {
+        $table1 = new Table('foo');
+        $table1->addColumn('foo', 'integer');
+
+        $table2 = clone $table1;
+
+        $table1->addIndex(array('foo'), 'idx_foo');
+        $table1->addIndex(array('foo'), 'idx_bar');
+
+        $table2->addIndex(array('foo'), 'idx_baz');
+
+        $comparator = new Comparator();
+        $tableDiff = $comparator->diffTable($table1, $table2);
+
+        $this->assertCount(1, $tableDiff->addedIndexes);
+        $this->assertArrayHasKey('idx_baz', $tableDiff->addedIndexes);
+        $this->assertCount(2, $tableDiff->removedIndexes);
+        $this->assertArrayHasKey('idx_foo', $tableDiff->removedIndexes);
+        $this->assertArrayHasKey('idx_bar', $tableDiff->removedIndexes);
+        $this->assertCount(0, $tableDiff->renamedIndexes);
+    }
+
     public function testDetectChangeIdentifierType()
     {
         $this->markTestSkipped('DBAL-2 was reopened, this test cannot work anymore.');

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

@@ -351,19 +351,6 @@ class TableTest extends \Doctrine\Tests\DbalTestCase
         $this->assertEquals(1, count($table->getIndexes()));
     }
 
-    /**
-     * @group DBAL-50
-     */
-    public function testAddIndexTwice_IgnoreSecond()
-    {
-        $table = new Table("foo.bar");
-        $table->addColumn('baz', 'integer', array());
-        $table->addIndex(array('baz'));
-        $table->addIndex(array('baz'));
-
-        $this->assertEquals(1, count($table->getIndexes()));
-    }
-
     /**
      * @group DBAL-50
      */
@@ -385,10 +372,57 @@ class TableTest extends \Doctrine\Tests\DbalTestCase
         $this->assertEquals(array('id'), $index->getColumns());
     }
 
+    /**
+     * @group DBAL-1063
+     */
+    public function testAddForeignKeyDoesNotCreateDuplicateIndex()
+    {
+        $table = new Table('foo');
+        $table->addColumn('bar', 'integer');
+        $table->addIndex(array('bar'), 'bar_idx');
+
+        $foreignTable = new Table('bar');
+        $foreignTable->addColumn('foo', 'integer');
+
+        $table->addForeignKeyConstraint($foreignTable, array('bar'), array('foo'));
+
+        $this->assertCount(1, $table->getIndexes());
+        $this->assertTrue($table->hasIndex('bar_idx'));
+        $this->assertSame(array('bar'), $table->getIndex('bar_idx')->getColumns());
+    }
+
+    /**
+     * @group DBAL-1063
+     */
+    public function testAddForeignKeyAddsImplicitIndexIfIndexColumnsDoNotSpan()
+    {
+        $table = new Table('foo');
+        $table->addColumn('bar', 'integer');
+        $table->addColumn('baz', 'string');
+        $table->addColumn('bloo', 'string');
+        $table->addIndex(array('baz', 'bar'), 'composite_idx');
+        $table->addIndex(array('bar', 'baz', 'bloo'), 'full_idx');
+
+        $foreignTable = new Table('bar');
+        $foreignTable->addColumn('foo', 'integer');
+        $foreignTable->addColumn('baz', 'string');
+
+        $table->addForeignKeyConstraint($foreignTable, array('bar', 'baz'), array('foo', 'baz'));
+
+        $this->assertCount(3, $table->getIndexes());
+        $this->assertTrue($table->hasIndex('composite_idx'));
+        $this->assertTrue($table->hasIndex('full_idx'));
+        $this->assertTrue($table->hasIndex('idx_8c73652176ff8caa78240498'));
+        $this->assertSame(array('baz', 'bar'), $table->getIndex('composite_idx')->getColumns());
+        $this->assertSame(array('bar', 'baz', 'bloo'), $table->getIndex('full_idx')->getColumns());
+        $this->assertSame(array('bar', 'baz'), $table->getIndex('idx_8c73652176ff8caa78240498')->getColumns());
+    }
+
     /**
      * @group DBAL-50
+     * @group DBAL-1063
      */
-    public function testOverruleIndex()
+    public function testOverrulingIndexDoesNotDropOverruledIndex()
     {
         $table = new Table("bar");
         $table->addColumn('baz', 'integer', array());
@@ -399,23 +433,62 @@ class TableTest extends \Doctrine\Tests\DbalTestCase
         $index = current($indexes);
 
         $table->addUniqueIndex(array('baz'));
-        $this->assertEquals(1, count($table->getIndexes()));
-        $this->assertFalse($table->hasIndex($index->getName()));
+        $this->assertEquals(2, count($table->getIndexes()));
+        $this->assertTrue($table->hasIndex($index->getName()));
+    }
+
+    /**
+     * @group DBAL-1063
+     */
+    public function testAllowsAddingDuplicateIndexesBasedOnColumns()
+    {
+        $table = new Table('foo');
+        $table->addColumn('bar', 'integer');
+        $table->addIndex(array('bar'), 'bar_idx');
+        $table->addIndex(array('bar'), 'duplicate_idx');
+
+        $this->assertCount(2, $table->getIndexes());
+        $this->assertTrue($table->hasIndex('bar_idx'));
+        $this->assertTrue($table->hasIndex('duplicate_idx'));
+        $this->assertSame(array('bar'), $table->getIndex('bar_idx')->getColumns());
+        $this->assertSame(array('bar'), $table->getIndex('duplicate_idx')->getColumns());
     }
 
-    public function testPrimaryKeyOverrulesUniqueIndex()
+    /**
+     * @group DBAL-1063
+     */
+    public function testAllowsAddingFulfillingIndexesBasedOnColumns()
+    {
+        $table = new Table('foo');
+        $table->addColumn('bar', 'integer');
+        $table->addColumn('baz', 'string');
+        $table->addIndex(array('bar'), 'bar_idx');
+        $table->addIndex(array('bar', 'baz'), 'fulfilling_idx');
+
+        $this->assertCount(2, $table->getIndexes());
+        $this->assertTrue($table->hasIndex('bar_idx'));
+        $this->assertTrue($table->hasIndex('fulfilling_idx'));
+        $this->assertSame(array('bar'), $table->getIndex('bar_idx')->getColumns());
+        $this->assertSame(array('bar', 'baz'), $table->getIndex('fulfilling_idx')->getColumns());
+    }
+
+    /**
+     * @group DBAL-50
+     * @group DBAL-1063
+     */
+    public function testPrimaryKeyOverrulingUniqueIndexDoesNotDropUniqueIndex()
     {
         $table = new Table("bar");
         $table->addColumn('baz', 'integer', array());
-        $table->addUniqueIndex(array('baz'));
+        $table->addUniqueIndex(array('baz'), 'idx_unique');
 
         $table->setPrimaryKey(array('baz'));
 
         $indexes = $table->getIndexes();
-        $this->assertEquals(1, count($indexes), "Table should only contain the primary key table index, not the unique one anymore, because it was overruled.");
+        $this->assertEquals(2, count($indexes), "Table should only contain both the primary key table index and the unique one, even though it was overruled.");
 
-        $index = current($indexes);
-        $this->assertTrue($index->isPrimary());
+        $this->assertTrue($table->hasPrimaryKey());
+        $this->assertTrue($table->hasIndex('idx_unique'));
     }
 
     /**