From 561236bd5626d0ca99d81a12d9c80b66e54d96a1 Mon Sep 17 00:00:00 2001
From: "Roman S. Borschel" <roman.borschel@googlemail.com>
Date: Sat, 8 May 2010 14:08:18 +0200
Subject: [PATCH] [DDC-576] Fixed.

---
 lib/Doctrine/DBAL/Driver/Connection.php       |  2 -
 .../DBAL/Driver/PDOMsSql/Connection.php       | 29 +++++++-----
 lib/Doctrine/ORM/Id/IdentityGenerator.php     | 31 ++++++++----
 .../ORM/Id/SequenceIdentityGenerator.php      | 46 ------------------
 .../ORM/Mapping/ClassMetadataFactory.php      | 24 ++++++----
 .../ORM/Mapping/ClassMetadataInfo.php         |  4 +-
 .../PostgreSQLIdentityStrategyTest.php        | 47 +++++++++++++++++++
 7 files changed, 105 insertions(+), 78 deletions(-)
 delete mode 100644 lib/Doctrine/ORM/Id/SequenceIdentityGenerator.php
 create mode 100644 tests/Doctrine/Tests/ORM/Functional/PostgreSQLIdentityStrategyTest.php

diff --git a/lib/Doctrine/DBAL/Driver/Connection.php b/lib/Doctrine/DBAL/Driver/Connection.php
index cee11f31a..4cc5776a6 100644
--- a/lib/Doctrine/DBAL/Driver/Connection.php
+++ b/lib/Doctrine/DBAL/Driver/Connection.php
@@ -1,7 +1,5 @@
 <?php
 /*
- *  $Id$
- *
  * 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
diff --git a/lib/Doctrine/DBAL/Driver/PDOMsSql/Connection.php b/lib/Doctrine/DBAL/Driver/PDOMsSql/Connection.php
index 1b5fb4efc..b9ff38899 100644
--- a/lib/Doctrine/DBAL/Driver/PDOMsSql/Connection.php
+++ b/lib/Doctrine/DBAL/Driver/PDOMsSql/Connection.php
@@ -1,7 +1,5 @@
 <?php
 /*
- *  $Id$
- *
  * 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
@@ -21,17 +19,17 @@
 
 namespace Doctrine\DBAL\Driver\PDOMsSql;
 
+use PDO, Doctrine\DBAL\Driver\Connection as DriverConnection;
+
 /**
  * MsSql Connection implementation.
  *
  * @since 2.0
  */
-class Connection extends \PDO implements \Doctrine\DBAL\Driver\Connection
+class Connection extends PDO implements DriverConnection
 {
     /**
-     * Performs the rollback.
-     * 
-     * @override
+     * {@inheritdoc}
      */
     public function rollback()
     {
@@ -39,9 +37,7 @@ class Connection extends \PDO implements \Doctrine\DBAL\Driver\Connection
     }
 
     /**
-     * Performs the commit.
-     * 
-     * @override
+     * {@inheritdoc}
      */
     public function commit()
     {
@@ -49,12 +45,21 @@ class Connection extends \PDO implements \Doctrine\DBAL\Driver\Connection
     }
 
     /**
-     * Begins a database transaction.
-     * 
-     * @override
+     * {@inheritdoc}
      */
     public function beginTransaction()
     {
         $this->exec('BEGIN TRANSACTION');
     }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function lastInsertId($name = null)
+    {
+        $stmt = $this->query('SELECT SCOPE_IDENTITY()');
+        $id = $stmt->fetchColumn();
+        $stmt->closeCursor();
+        return $id;
+    }
 }
\ No newline at end of file
diff --git a/lib/Doctrine/ORM/Id/IdentityGenerator.php b/lib/Doctrine/ORM/Id/IdentityGenerator.php
index 96ad08add..75da2733d 100644
--- a/lib/Doctrine/ORM/Id/IdentityGenerator.php
+++ b/lib/Doctrine/ORM/Id/IdentityGenerator.php
@@ -21,23 +21,36 @@ namespace Doctrine\ORM\Id;
 
 use Doctrine\ORM\EntityManager;
 
+/**
+ * Id generator that obtains IDs from special "identity" columns. These are columns
+ * that automatically get a database-generated, auto-incremented identifier on INSERT.
+ * This generator obtains the last insert id after such an insert.
+ */
 class IdentityGenerator extends AbstractIdGenerator
 {
+    /** @var string The name of the sequence to pass to lastInsertId(), if any. */
+    private $_seqName;
+
     /**
-     * Generates an ID for the given entity.
-     *
-     * @param object $entity
-     * @return integer|float
-     * @override
+     * @param string $seqName The name of the sequence to pass to lastInsertId()
+     *                        to obtain the last generated identifier within the current
+     *                        database session/connection, if any.
      */
-    public function generate(EntityManager $em, $entity)
+    public function __construct($seqName = null)
     {
-        return $em->getConnection()->lastInsertId();
+        $this->_seqName = $seqName;
     }
 
     /**
-     * @return boolean
-     * @override
+     * {@inheritdoc}
+     */
+    public function generate(EntityManager $em, $entity)
+    {
+        return $em->getConnection()->lastInsertId($this->_seqName);
+    }
+
+    /**
+     * {@inheritdoc}
      */
     public function isPostInsertGenerator()
     {
diff --git a/lib/Doctrine/ORM/Id/SequenceIdentityGenerator.php b/lib/Doctrine/ORM/Id/SequenceIdentityGenerator.php
deleted file mode 100644
index c7158bbed..000000000
--- a/lib/Doctrine/ORM/Id/SequenceIdentityGenerator.php
+++ /dev/null
@@ -1,46 +0,0 @@
-<?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 LGPL. For more information, see
- * <http://www.doctrine-project.org>.
- */
-
-namespace Doctrine\ORM\Id;
-
-use Doctrine\ORM\EntityManager;
-
-class SequenceIdentityGenerator extends IdentityGenerator
-{
-    private $_sequenceName;
-    
-    public function __construct($sequenceName)
-    {
-        $this->_sequenceName = $sequenceName;
-    }
-
-    public function generate(EntityManager $em, $entity)
-    {
-        return $em->getConnection()->lastInsertId($this->_sequenceName);
-    }
-    
-    /**
-     * @return boolean
-     * @override
-     */
-    public function isPostInsertGenerator()
-    {
-        return true;
-    }
-}
\ No newline at end of file
diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
index 1dc812cd7..fb3a09906 100644
--- a/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
+++ b/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php
@@ -19,8 +19,10 @@
 
 namespace Doctrine\ORM\Mapping;
 
-use Doctrine\ORM\ORMException,
-    Doctrine\DBAL\Platforms\AbstractPlatform,
+use ReflectionException,
+    Doctrine\ORM\ORMException,
+    Doctrine\ORM\EntityManager,
+    Doctrine\DBAL\Platforms,
     Doctrine\ORM\Events;
 
 /**
@@ -53,7 +55,7 @@ class ClassMetadataFactory
      *
      * @param $driver  The metadata driver to use.
      */
-    public function __construct(\Doctrine\ORM\EntityManager $em)
+    public function __construct(EntityManager $em)
     {
         $this->_em = $em;
     }
@@ -94,15 +96,15 @@ class ClassMetadataFactory
         if ( ! $this->_initialized) {
             $this->_initialize();
         }
-        
+
         $metadata = array();
         foreach ($this->_driver->getAllClassNames() as $className) {
             $metadata[] = $this->getMetadataFor($className);
         }
-        
+
         return $metadata;
     }
-    
+
     /**
      * Lazy initialization of this stuff, especially the metadata driver,
      * since these are not needed at all when a metadata cache is active.
@@ -252,7 +254,7 @@ class ClassMetadataFactory
             // Invoke driver
             try {
                 $this->_driver->loadMetadataForClass($className, $class);
-            } catch(\ReflectionException $e) { 
+            } catch(ReflectionException $e) { 
                 throw MappingException::reflectionFailure($className, $e);
             }
 
@@ -376,7 +378,13 @@ class ClassMetadataFactory
         // Create & assign an appropriate ID generator instance
         switch ($class->generatorType) {
             case ClassMetadata::GENERATOR_TYPE_IDENTITY:
-                $class->setIdGenerator(new \Doctrine\ORM\Id\IdentityGenerator());
+                // For PostgreSQL IDENTITY (SERIAL) we need a sequence name. It defaults to
+                // <table>_<column>_seq in PostgreSQL for SERIAL columns.
+                // Not pretty but necessary and the simplest solution that currently works.
+                $seqName = $this->_targetPlatform instanceof Platforms\PostgreSQLPlatform ?
+                        $class->table['name'] . '_' . $class->columnNames[$class->identifier[0]] . '_seq' :
+                        null;
+                $class->setIdGenerator(new \Doctrine\ORM\Id\IdentityGenerator($seqName));
                 break;
             case ClassMetadata::GENERATOR_TYPE_SEQUENCE:
                 // If there is no sequence definition yet, create a default definition
diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
index a6100856c..b173e395d 100644
--- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
+++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
@@ -317,7 +317,7 @@ class ClassMetadataInfo
      * READ-ONLY: The ID generator used for generating IDs for this class.
      *
      * @var AbstractIdGenerator
-     * @todo Remove
+     * @todo Remove!
      */
     public $idGenerator;
 
@@ -335,6 +335,7 @@ class ClassMetadataInfo
      * </code>
      *
      * @var array
+     * @todo Merge with tableGeneratorDefinition into generic generatorDefinition
      */
     public $sequenceGeneratorDefinition;
 
@@ -343,6 +344,7 @@ class ClassMetadataInfo
      * TABLE generation strategy.
      *
      * @var array
+     * @todo Merge with tableGeneratorDefinition into generic generatorDefinition
      */
     public $tableGeneratorDefinition;
 
diff --git a/tests/Doctrine/Tests/ORM/Functional/PostgreSQLIdentityStrategyTest.php b/tests/Doctrine/Tests/ORM/Functional/PostgreSQLIdentityStrategyTest.php
new file mode 100644
index 000000000..8b4a49ccf
--- /dev/null
+++ b/tests/Doctrine/Tests/ORM/Functional/PostgreSQLIdentityStrategyTest.php
@@ -0,0 +1,47 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\ORM\Event\PreUpdateEventArgs;
+
+require_once __DIR__ . '/../../TestInit.php';
+
+class PostgreSQLIdentityStrategyTest extends \Doctrine\Tests\OrmFunctionalTestCase
+{
+    protected function setUp() {
+        parent::setUp();
+        if ($this->_em->getConnection()->getDatabasePlatform()->getName() != 'postgresql') {
+            $this->markTestSkipped('This test is special to the PostgreSQL IDENTITY key generation strategy.');
+        } else {
+            try {
+                $this->_schemaTool->createSchema(array(
+                        $this->_em->getClassMetadata('Doctrine\Tests\ORM\Functional\PostgreSQLIdentityEntity'),
+                ));
+            } catch (\Exception $e) {
+                // Swallow all exceptions. We do not test the schema tool here.
+            }
+        }
+    }
+    
+    public function testPreSavePostSaveCallbacksAreInvoked()
+    {        
+        $entity = new PostgreSQLIdentityEntity();
+        $entity->setValue('hello');
+        $this->_em->persist($entity);
+        $this->_em->flush();
+        $this->assertTrue(is_numeric($entity->getId()));
+        $this->assertTrue($entity->getId() > 0);
+        $this->assertTrue($this->_em->contains($entity));
+    }
+}
+
+/** @Entity */
+class PostgreSQLIdentityEntity {
+    /** @Id @Column(type="integer") @GeneratedValue(strategy="IDENTITY") */
+    private $id;
+    /** @Column(type="string") */
+    private $value;
+    public function getId() {return $this->id;}
+    public function getValue() {return $this->value;}
+    public function setValue($value) {$this->value = $value;}
+}