Fix(migrations) Handle transaction within migrations

This fixes #98

Author: thePanz

lib/Doctrine/Export.php

@@ -1221,7 +1221,7 @@ public function exportClasses(array $classes)
                  }
              }
 
-             $connection->commit();
+             Doctrine_TransactionHelper::commitIfInTransaction($connection);
          }
      }
 

lib/Doctrine/Migration.php

@@ -187,7 +187,7 @@ public function loadMigrationClass($name, $path = null)
         }
 
         if ($class === false) {
-            return false;
+            return;
         }
 
         if (empty($this->_migrationClasses)) {
@@ -307,15 +307,13 @@ public function getNextMigrationClassVersion()
      *
      * @param  integer $to       Version to migrate to
      * @param  boolean $dryRun   Whether or not to run the migrate process as a dry run
-     * @return integer $to       Version number migrated to
+     * @return bool              True if the migration succeeded, false otherwise
      * @throws Doctrine_Exception
      */
     public function migrate($to = null, $dryRun = false)
     {
         $this->clearErrors();
-
         $this->_createMigrationTable();
-
         $this->_connection->beginTransaction();
 
         try {
@@ -335,24 +333,22 @@ public function migrate($to = null, $dryRun = false)
 
             if ($dryRun) {
                 return false;
-            } else {
-                $this->_throwErrorsException();
-            }
-        } else {
-            if ($dryRun) {
-                $this->_connection->rollback();
-                if ($this->hasErrors()) {
-                    return false;
-                } else {
-                    return $to;
-                }
-            } else {
-                $this->_connection->commit();
-                $this->setCurrentVersion($to);
-                return $to;
             }
+
+            $this->_throwErrorsException();
         }
-        return false;
+
+
+        if ($dryRun) {
+            $this->_connection->rollback();
+
+            return !$this->hasErrors();
+        }
+
+        Doctrine_TransactionHelper::commitIfInTransaction($this->_connection);
+        $this->setCurrentVersion($to);
+
+        return true;
     }
 
     /**
@@ -437,8 +433,9 @@ public function getMigrationClass($num)
     /**
      * Throw an exception with all the errors trigged during the migration
      *
-     * @return void
-     * @throws Doctrine_Migration_Exception $e
+     * @throws Doctrine_Migration_Exception
+     *
+     * @return never
      */
     protected function _throwErrorsException()
     {
@@ -559,4 +556,4 @@ protected function _createMigrationTable()
             return false;
         }
     }
-}
+}

lib/Doctrine/TransactionHelper.php

@@ -0,0 +1,24 @@
+<?php
+
+/**
+ * Helper for transactions handling.
+ *
+ * @author Kyle McGrogan <[email protected]>
+ */
+final class Doctrine_TransactionHelper
+{
+    /**
+     * Execute a commit on the given connection, only if a transaction already started.
+     */
+    public static function commitIfInTransaction(Doctrine_Connection $connection): void
+    {
+        $handler = $connection->getDbh();
+
+        // Attempt to commit while no transaction is running results in exception since PHP 8 + pdo_mysql combination
+        if ($handler instanceof PDO && !$handler->inTransaction()) {
+            return;
+        }
+
+        $connection->commit();
+    }
+}

tests/DoctrineTest/Doctrine_UnitTestCase.php

@@ -216,6 +216,7 @@ public function init()
             }
         }
     }
+
     public function prepareTables() {
         foreach($this->tables as $name) {
             $name = ucwords($name);
@@ -230,6 +231,7 @@ public function prepareTables() {
         $this->conn->export->exportClasses($this->tables);
         $this->objTable = $this->connection->getTable('User');
     }
+
     public function prepareData()
     {
         $groups = new Doctrine_Collection($this->connection->getTable('Group'));
@@ -307,6 +309,17 @@ public function getDeclaration($type)
         return $this->dataDict->getPortableDeclaration(array('type' => $type, 'name' => 'colname', 'length' => 1, 'fixed' => true));
     }
 
+    /**
+     * @param string $pdoDsn
+     * @return Doctrine_Connection
+     */
+    protected function openAdditionalPdoConnection(string $pdoDsn)
+    {
+        $pdoFromDsn = new PDO($pdoDsn);
+
+        return $this->openAdditionalConnection($pdoFromDsn);
+    }
+
     protected function openAdditionalConnection($adapter = null, $name = null)
     {
         $connection = $this->manager->openConnection($adapter, $name);

tests/MigrationMysqlTestCase.php

@@ -0,0 +1,68 @@
+<?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
+ * 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>.
+ */
+
+/**
+ * Doctrine_Migration_TestCase
+ *
+ * @package     Doctrine
+ * @author      Konsta Vesterinen <[email protected]>
+ * @license     http://www.opensource.org/licenses/lgpl-license.php LGPL
+ * @category    Object Relational Mapping
+ * @link        www.doctrine-project.org
+ * @since       1.0
+ * @version     $Revision$
+ */
+class Doctrine_MigrationMysql_TestCase extends Doctrine_UnitTestCase
+{
+    public function setUp()
+    {
+        parent::setUp();
+        $dsn = getenv('MYSQL_TEST_DSN') ?? '';
+
+        $this->connection = $this->openAdditionalPdoConnection($dsn);
+        $this->connection->setOption('dsn', $dsn);
+        $this->connection->dropDatabase();
+        $this->connection->createDatabase();
+    }
+
+    public function testAfterSuccessfullMigrationItWillSetMigratedVersionAsCurrentVersionInMysqlDB()
+    {
+        $migration = new Doctrine_Migration(__DIR__.'/migration_classes', $this->connection);
+        $this->assertFalse($migration->hasMigrated());
+        $migration->setCurrentVersion(0);
+        $this->assertFalse($this->connection->import->tableExists('migration_phonenumber'));
+        $this->assertFalse($this->connection->import->tableExists('migration_user'));
+        $this->assertFalse($this->connection->import->tableExists('migration_profile'));
+
+        $migration->migrate(3);
+        $this->assertTrue($migration->hasMigrated());
+        $this->assertEqual($migration->getCurrentVersion(), 3);
+        $this->assertTrue($this->connection->import->tableExists('migration_phonenumber'));
+        $this->assertTrue($this->connection->import->tableExists('migration_user'));
+        $this->assertTrue($this->connection->import->tableExists('migration_profile'));
+
+        $migration->migrate(4);
+        $this->assertEqual($migration->getCurrentVersion(), 4);
+        $this->assertTrue($this->connection->import->tableExists('migration_phonenumber'));
+        $this->assertTrue($this->connection->import->tableExists('migration_user'));
+        $this->assertFalse($this->connection->import->tableExists('migration_profile'));
+    }
+}

tests/run.php

@@ -279,6 +279,7 @@
 // Migration Tests
 $migration = new GroupTest('Migration Tests', 'migration');
 $migration->addTestCase(new Doctrine_Migration_TestCase());
+$migration->addTestCase(new Doctrine_MigrationMysql_TestCase());
 $migration->addTestCase(new Doctrine_Migration_Base_TestCase());
 $migration->addTestCase(new Doctrine_Migration_Diff_TestCase());
 $test->addTestCase($migration);