Merge pull request #302 from deeky666/fix-asset-identfier-quotation

[DBAL-374] Fix asset identfier quotation

UPGRADE

+# Upgrade to 2.4
+
+## Doctrine\DBAL\Schema\Constraint
+
+If you have custom classes that implement the constraint interface, you have to implement
+an additional method ``getQuotedColumns`` now. This method is used to build proper constraint
+SQL for columns that need to be quoted, like keywords reserved by the specific platform used.
+The method has to return the same values as ``getColumns`` only that those column names that
+need quotation have to be returned quoted for the given platform.
+
 # Upgrade to 2.3
 
 ## Oracle Session Init now sets Numeric Character

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

@@ -1166,9 +1166,7 @@ abstract class AbstractPlatform
                 /* @var $index Index */
                 if ($index->isPrimary()) {
                     $platform = $this;
-                    $options['primary'] = array_map(function ($columnName) use ($table, $platform) {
-                        return $table->getColumn($columnName)->getQuotedName($platform);
-                    }, $index->getColumns());
+                    $options['primary']       = $index->getQuotedColumns($this);
                     $options['primary_index'] = $index;
                 } else {
                     $options['indexes'][$index->getName()] = $index;
@@ -1337,11 +1335,7 @@ abstract class AbstractPlatform
 
         $query = 'ALTER TABLE ' . $table . ' ADD CONSTRAINT ' . $constraint->getQuotedName($this);
 
-        $columns = array();
-        foreach ($constraint->getColumns() as $column) {
-            $columns[] = $column;
-        }
-        $columnList = '('. implode(', ', $columns) . ')';
+        $columnList = '('. implode(', ', $constraint->getQuotedColumns($this)) . ')';
 
         $referencesClause = '';
         if ($constraint instanceof Index) {
@@ -1357,13 +1351,8 @@ abstract class AbstractPlatform
         } else if ($constraint instanceof ForeignKeyConstraint) {
             $query .= ' FOREIGN KEY';
 
-            $foreignColumns = array();
-            foreach ($constraint->getForeignColumns() as $column) {
-                $foreignColumns[] = $column;
-            }
-
             $referencesClause = ' REFERENCES ' . $constraint->getQuotedForeignTableName($this) .
-                ' (' . implode(', ', $foreignColumns) . ')';
+                ' (' . implode(', ', $constraint->getQuotedForeignColumns($this)) . ')';
         }
         $query .= ' '.$columnList.$referencesClause;
 
@@ -1384,7 +1373,7 @@ abstract class AbstractPlatform
             $table = $table->getQuotedName($this);
         }
         $name = $index->getQuotedName($this);
-        $columns = $index->getColumns();
+        $columns = $index->getQuotedColumns($this);
 
         if (count($columns) == 0) {
             throw new \InvalidArgumentException("Incomplete definition. 'columns' required.");
@@ -1422,7 +1411,7 @@ abstract class AbstractPlatform
      */
     public function getCreatePrimaryKeySQL(Index $index, $table)
     {
-        return 'ALTER TABLE ' . $table . ' ADD PRIMARY KEY (' . $this->getIndexFieldDeclarationListSQL($index->getColumns()) . ')';
+        return 'ALTER TABLE ' . $table . ' ADD PRIMARY KEY (' . $this->getIndexFieldDeclarationListSQL($index->getQuotedColumns($this)) . ')';
     }
 
     /**
@@ -1874,12 +1863,14 @@ abstract class AbstractPlatform
      */
     public function getUniqueConstraintDeclarationSQL($name, Index $index)
     {
-        if (count($index->getColumns()) === 0) {
+        $columns = $index->getQuotedColumns($this);
+
+        if (count($columns) === 0) {
             throw new \InvalidArgumentException("Incomplete definition. 'columns' required.");
         }
 
         return 'CONSTRAINT ' . $name . ' UNIQUE ('
-             . $this->getIndexFieldDeclarationListSQL($index->getColumns())
+             . $this->getIndexFieldDeclarationListSQL($columns)
              . ')';
     }
 
@@ -1894,12 +1885,14 @@ abstract class AbstractPlatform
      */
     public function getIndexDeclarationSQL($name, Index $index)
     {
-        if (count($index->getColumns()) === 0) {
+        $columns = $index->getQuotedColumns($this);
+
+        if (count($columns) === 0) {
             throw new \InvalidArgumentException("Incomplete definition. 'columns' required.");
         }
 
         return $this->getCreateIndexSQLFlags($index) . 'INDEX ' . $name . ' ('
-             . $this->getIndexFieldDeclarationListSQL($index->getColumns())
+             . $this->getIndexFieldDeclarationListSQL($columns)
              . ')';
     }
 
@@ -2061,10 +2054,10 @@ abstract class AbstractPlatform
             throw new \InvalidArgumentException("Incomplete definition. 'foreignTable' required.");
         }
 
-        $sql .= implode(', ', $foreignKey->getLocalColumns())
+        $sql .= implode(', ', $foreignKey->getQuotedLocalColumns($this))
               . ') REFERENCES '
               . $foreignKey->getQuotedForeignTableName($this) . ' ('
-              . implode(', ', $foreignKey->getForeignColumns()) . ')';
+              . implode(', ', $foreignKey->getQuotedForeignColumns($this)) . ')';
 
         return $sql;
     }

lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

@@ -562,7 +562,6 @@ class MySqlPlatform extends AbstractPlatform
             foreach ($diff->addedIndexes as $addKey => $addIndex) {
                 if ($remIndex->getColumns() == $addIndex->getColumns()) {
 
-                    $columns = $addIndex->getColumns();
                     $type = '';
                     if ($addIndex->isUnique()) {
                         $type = 'UNIQUE ';
@@ -570,7 +569,7 @@ class MySqlPlatform extends AbstractPlatform
 
                     $query = 'ALTER TABLE ' . $table . ' DROP INDEX ' . $remIndex->getName() . ', ';
                     $query .= 'ADD ' . $type . 'INDEX ' . $addIndex->getName();
-                    $query .= ' (' . $this->getIndexFieldDeclarationListSQL($columns) . ')';
+                    $query .= ' (' . $this->getIndexFieldDeclarationListSQL($addIndex->getQuotedColumns($this)) . ')';
 
                     $sql[] = $query;
 

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

@@ -262,7 +262,7 @@ class SQLServerPlatform extends AbstractPlatform
         if ($index->hasFlag('nonclustered')) {
             $flags = ' NONCLUSTERED';
         }
-        return 'ALTER TABLE ' . $table . ' ADD PRIMARY KEY' . $flags . ' (' . $this->getIndexFieldDeclarationListSQL($index->getColumns()) . ')';
+        return 'ALTER TABLE ' . $table . ' ADD PRIMARY KEY' . $flags . ' (' . $this->getIndexFieldDeclarationListSQL($index->getQuotedColumns($this)) . ')';
     }
 
     /**
@@ -344,11 +344,8 @@ class SQLServerPlatform extends AbstractPlatform
     private function _appendUniqueConstraintDefinition($sql, Index $index)
     {
         $fields = array();
-        foreach ($index->getColumns() as $field => $definition) {
-            if (!is_array($definition)) {
-                $field = $definition;
-            }
 
+        foreach ($index->getQuotedColumns($this) as $field) {
             $fields[] = $field . ' IS NOT NULL';
         }
 

lib/Doctrine/DBAL/Schema/Constraint.php

@@ -39,4 +39,18 @@ interface Constraint
     public function getQuotedName(AbstractPlatform $platform);
 
     public function getColumns();
+
+    /**
+     * Returns the quoted representation of the column names
+     * the constraint is associated with.
+     *
+     * But only if they were defined with one or a column name
+     * is a keyword reserved by the platform.
+     * Otherwise the plain unquoted value as inserted is returned.
+     *
+     * @param \Doctrine\DBAL\Platforms\AbstractPlatform $platform The platform to use for quotation.
+     *
+     * @return array
+     */
+    public function getQuotedColumns(AbstractPlatform $platform);
 }

lib/Doctrine/DBAL/Schema/Identifier.php

+<?php
+/*
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+namespace Doctrine\DBAL\Schema;
+
+use Doctrine\DBAL\Schema\AbstractAsset;
+
+/**
+ * An abstraction class for an asset identifier.
+ *
+ * Wraps identifier names like column names in indexes / foreign keys
+ * in an abstract class for proper quotation capabilities.
+ *
+ * @author Steve Müller <st.mueller@dzh-online.de>
+ * @link   www.doctrine-project.org
+ * @since  2.4
+ */
+class Identifier extends AbstractAsset
+{
+    /**
+     * Constructor.
+     *
+     * @param string $identifier Identifier name to wrap.
+     */
+    public function __construct($identifier)
+    {
+        $this->_setName($identifier);
+    }
+}

lib/Doctrine/DBAL/Schema/Index.php

@@ -19,12 +19,15 @@
 
 namespace Doctrine\DBAL\Schema;
 
-use Doctrine\DBAL\Schema\Visitor\Visitor;
+use Doctrine\DBAL\Platforms\AbstractPlatform;
 
 class Index extends AbstractAsset implements Constraint
 {
     /**
-     * @var array
+     * Asset identifier instances of the column names the index is associated with.
+     * array($columnName => Identifier)
+     *
+     * @var Identifier[]
      */
     protected $_columns;
 
@@ -73,7 +76,7 @@ class Index extends AbstractAsset implements Constraint
     protected function _addColumn($column)
     {
         if(is_string($column)) {
-            $this->_columns[] = $column;
+            $this->_columns[$column] = new Identifier($column);
         } else {
             throw new \InvalidArgumentException("Expecting a string as Index Column");
         }
@@ -84,7 +87,21 @@ class Index extends AbstractAsset implements Constraint
      */
     public function getColumns()
     {
-        return $this->_columns;
+        return array_keys($this->_columns);
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function getQuotedColumns(AbstractPlatform $platform)
+    {
+        $columns = array();
+
+        foreach ($this->_columns as $column) {
+            $columns[] = $column->getQuotedName($platform);
+        }
+
+        return $columns;
     }
 
     /**
@@ -141,12 +158,16 @@ class Index extends AbstractAsset implements Constraint
      */
     public function spansColumns(array $columnNames)
     {
-        $sameColumns = true;
-        for ($i = 0; $i < count($this->_columns); $i++) {
-            if (!isset($columnNames[$i]) || $this->trimQuotes(strtolower($this->_columns[$i])) != $this->trimQuotes(strtolower($columnNames[$i]))) {
+        $columns         = $this->getColumns();
+        $numberOfColumns = count($columns);
+        $sameColumns     = true;
+
+        for ($i = 0; $i < $numberOfColumns; $i++) {
+            if ( ! isset($columnNames[$i]) || $this->trimQuotes(strtolower($columns[$i])) !== $this->trimQuotes(strtolower($columnNames[$i]))) {
                 $sameColumns = false;
             }
         }
+
         return $sameColumns;
     }
 

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

@@ -4,6 +4,7 @@ namespace Doctrine\Tests\DBAL\Platforms;
 
 use Doctrine\Common\EventManager;
 use Doctrine\DBAL\Events;
+use Doctrine\DBAL\Platforms\AbstractPlatform;
 use Doctrine\DBAL\Schema\ForeignKeyConstraint;
 use Doctrine\DBAL\Schema\Table;
 use Doctrine\DBAL\Schema\TableDiff;
@@ -432,8 +433,8 @@ abstract class AbstractPlatformTestCase extends \Doctrine\Tests\DbalTestCase
     public function testQuotedColumnInPrimaryKeyPropagation()
     {
         $table = new Table('`quoted`');
-        $table->addColumn('`key`', 'string');
-        $table->setPrimaryKey(array('key'));
+        $table->addColumn('create', 'string');
+        $table->setPrimaryKey(array('create'));
 
         $sql = $this->_platform->getCreateTableSQL($table);
         $this->assertEquals($this->getQuotedColumnInPrimaryKeySQL(), $sql);
@@ -441,19 +442,37 @@ abstract class AbstractPlatformTestCase extends \Doctrine\Tests\DbalTestCase
 
     abstract protected function getQuotedColumnInPrimaryKeySQL();
     abstract protected function getQuotedColumnInIndexSQL();
+    abstract protected function getQuotedColumnInForeignKeySQL();
 
     /**
      * @group DBAL-374
      */
     public function testQuotedColumnInIndexPropagation()
     {
-        $this->markTestSkipped('requires big refactoring of Platforms');
-
         $table = new Table('`quoted`');
-        $table->addColumn('`key`', 'string');
-        $table->addIndex(array('key'));
+        $table->addColumn('create', 'string');
+        $table->addIndex(array('create'));
 
         $sql = $this->_platform->getCreateTableSQL($table);
         $this->assertEquals($this->getQuotedColumnInIndexSQL(), $sql);
     }
+
+    /**
+     * @group DBAL-374
+     */
+    public function testQuotedColumnInForeignKeyPropagation()
+    {
+        $table = new Table('`quoted`');
+        $table->addColumn('create', 'string');
+        $table->addColumn('foo', 'string');
+
+        $foreignTable = new Table('foreign');
+        $foreignTable->addColumn('create', 'string');
+        $foreignTable->addColumn('bar', 'string');
+
+        $table->addForeignKeyConstraint($foreignTable, array('create', 'foo'), array('create', 'bar'));
+
+        $sql = $this->_platform->getCreateTableSQL($table, AbstractPlatform::CREATE_FOREIGNKEYS);
+        $this->assertEquals($this->getQuotedColumnInForeignKeySQL(), $sql);
+    }
 }

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

@@ -237,14 +237,22 @@ class MySqlPlatformTest extends AbstractPlatformTestCase
     protected function getQuotedColumnInPrimaryKeySQL()
     {
         return array(
-            'CREATE TABLE `quoted` (`key` VARCHAR(255) NOT NULL, PRIMARY KEY(`key`)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB'
+            'CREATE TABLE `quoted` (`create` VARCHAR(255) NOT NULL, PRIMARY KEY(`create`)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB'
         );
     }
 
     protected function getQuotedColumnInIndexSQL()
     {
         return array(
-            'CREATE TABLE `quoted` (`key` VARCHAR(255) NOT NULL, INDEX IDX_22660D028A90ABA9 (`key`)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB'
+            'CREATE TABLE `quoted` (`create` VARCHAR(255) NOT NULL, INDEX IDX_22660D028FD6E0FB (`create`)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB'
+        );
+    }
+
+    protected function getQuotedColumnInForeignKeySQL()
+    {
+        return array(
+            'CREATE TABLE `quoted` (`create` VARCHAR(255) NOT NULL, foo VARCHAR(255) NOT NULL) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB',
+            'ALTER TABLE `quoted` ADD CONSTRAINT FK_22660D028FD6E0FB8C736521 FOREIGN KEY (`create`, foo) REFERENCES `foreign` (`create`, bar)',
         );
     }
 

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

@@ -288,14 +288,22 @@ class OraclePlatformTest extends AbstractPlatformTestCase
 
     protected function getQuotedColumnInPrimaryKeySQL()
     {
-        return array('CREATE TABLE "quoted" ("key" VARCHAR2(255) NOT NULL, PRIMARY KEY("key"))');
+        return array('CREATE TABLE "quoted" ("create" VARCHAR2(255) NOT NULL, PRIMARY KEY("create"))');
     }
 
     protected function getQuotedColumnInIndexSQL()
     {
         return array(
-            'CREATE TABLE "quoted" ("key" VARCHAR2(255) NOT NULL)',
-            'CREATE INDEX IDX_22660D028A90ABA9 ON "quoted" ("key")',
+            'CREATE TABLE "quoted" ("create" VARCHAR2(255) NOT NULL)',
+            'CREATE INDEX IDX_22660D028FD6E0FB ON "quoted" ("create")',
+        );
+    }
+
+    protected function getQuotedColumnInForeignKeySQL()
+    {
+        return array(
+            'CREATE TABLE "quoted" ("create" VARCHAR2(255) NOT NULL, foo VARCHAR2(255) NOT NULL)',
+            'ALTER TABLE "quoted" ADD CONSTRAINT FK_22660D028FD6E0FB8C736521 FOREIGN KEY ("create", foo) REFERENCES foreign ("create", bar)',
         );
     }
 }

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

@@ -270,15 +270,23 @@ class PostgreSqlPlatformTest extends AbstractPlatformTestCase
     protected function getQuotedColumnInPrimaryKeySQL()
     {
         return array(
-            'CREATE TABLE "quoted" ("key" VARCHAR(255) NOT NULL, PRIMARY KEY("key"))',
+            'CREATE TABLE "quoted" ("create" VARCHAR(255) NOT NULL, PRIMARY KEY("create"))',
         );
     }
 
     protected function getQuotedColumnInIndexSQL()
     {
         return array(
-            'CREATE TABLE "quoted" ("key" VARCHAR(255) NOT NULL)',
-            'CREATE INDEX IDX_22660D028A90ABA9 ON "quoted" ("key")',
+            'CREATE TABLE "quoted" ("create" VARCHAR(255) NOT NULL)',
+            'CREATE INDEX IDX_22660D028FD6E0FB ON "quoted" ("create")',
+        );
+    }
+
+    protected function getQuotedColumnInForeignKeySQL()
+    {
+        return array(
+            'CREATE TABLE "quoted" ("create" VARCHAR(255) NOT NULL, foo VARCHAR(255) NOT NULL)',
+            'ALTER TABLE "quoted" ADD CONSTRAINT FK_22660D028FD6E0FB8C736521 FOREIGN KEY ("create", foo) REFERENCES "foreign" ("create", bar) NOT DEFERRABLE INITIALLY IMMEDIATE',
         );
     }
 

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

@@ -296,15 +296,23 @@ class SQLServerPlatformTest extends AbstractPlatformTestCase
     protected function getQuotedColumnInPrimaryKeySQL()
     {
         return array(
-            'CREATE TABLE [quoted] ([key] NVARCHAR(255) NOT NULL, PRIMARY KEY ([key]))',
+            'CREATE TABLE [quoted] ([create] NVARCHAR(255) NOT NULL, PRIMARY KEY ([create]))',
         );
     }
 
     protected function getQuotedColumnInIndexSQL()
     {
         return array(
-            'CREATE TABLE [quoted] ([key] NVARCHAR(255) NOT NULL)',
-            'CREATE INDEX IDX_22660D028A90ABA9 ON [quoted] ([key])',
+            'CREATE TABLE [quoted] ([create] NVARCHAR(255) NOT NULL)',
+            'CREATE INDEX IDX_22660D028FD6E0FB ON [quoted] ([create])',
+        );
+    }
+
+    protected function getQuotedColumnInForeignKeySQL()
+    {
+        return array(
+            'CREATE TABLE [quoted] ([create] NVARCHAR(255) NOT NULL, foo NVARCHAR(255) NOT NULL)',
+            'ALTER TABLE [quoted] ADD CONSTRAINT FK_22660D028FD6E0FB8C736521 FOREIGN KEY ([create], foo) REFERENCES [foreign] ([create], bar)',
         );
     }
 }

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

@@ -280,15 +280,22 @@ class SqlitePlatformTest extends AbstractPlatformTestCase
     protected function getQuotedColumnInPrimaryKeySQL()
     {
         return array(
-            'CREATE TABLE "quoted" ("key" VARCHAR(255) NOT NULL, PRIMARY KEY("key"))',
+            'CREATE TABLE "quoted" ("create" VARCHAR(255) NOT NULL, PRIMARY KEY("create"))',
         );
     }
 
     protected function getQuotedColumnInIndexSQL()
     {
         return array(
-            'CREATE TABLE "quoted" ("key" VARCHAR(255) NOT NULL)',
-            'CREATE INDEX IDX_22660D028A90ABA9 ON "quoted" ("key")',
+            'CREATE TABLE "quoted" ("create" VARCHAR(255) NOT NULL)',
+            'CREATE INDEX IDX_22660D028FD6E0FB ON "quoted" ("create")',
+        );
+    }
+
+    protected function getQuotedColumnInForeignKeySQL()
+    {
+        return array(
+            'CREATE TABLE "quoted" ("create" VARCHAR(255) NOT NULL, foo VARCHAR(255) NOT NULL, CONSTRAINT FK_22660D028FD6E0FB8C736521 FOREIGN KEY ("create", foo) REFERENCES "foreign" ("create", bar) NOT DEFERRABLE INITIALLY IMMEDIATE)',
         );
     }
 }