From 78084b9a66edb85a8c86e0320d1d3fe76b161c30 Mon Sep 17 00:00:00 2001 From: thePanz Date: Thu, 27 Jun 2024 15:11:52 +0200 Subject: [PATCH] Fix(migrations) Handle transaction within migrations This fixes #98 --- .github/workflows/continuous-integration.yml | 11 ++++ lib/Doctrine/Export.php | 2 +- lib/Doctrine/Migration.php | 42 ++++++------ lib/Doctrine/TransactionHelper.php | 24 +++++++ tests/DoctrineTest/Doctrine_UnitTestCase.php | 13 ++++ tests/MigrationMysqlTestCase.php | 68 ++++++++++++++++++++ tests/run.php | 1 + 7 files changed, 137 insertions(+), 24 deletions(-) create mode 100644 lib/Doctrine/TransactionHelper.php create mode 100644 tests/MigrationMysqlTestCase.php diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 26b4f3b23..2b95e6caf 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -35,6 +35,15 @@ jobs: - "8.1" - "8.2" - "8.3" + services: + mysql: + image: mysql:5.7-debian + env: + MYSQL_DATABASE: 'doctrine1_test' + MYSQL_USER: 'doctrine1' + MYSQL_PASSWORD: 'd0ctr1n3' + ports: + - 3306:3306 steps: - name: Checkout @@ -62,4 +71,6 @@ jobs: run: composer install --prefer-dist - name: Run Tests + env: + MYSQL_TEST_DSN: 'mysql:host=127.0.0.1;dbname=doctrine1_test;user=doctrine1;password=d0ctr1n3' run: cd tests && php run.php diff --git a/lib/Doctrine/Export.php b/lib/Doctrine/Export.php index ab56d6620..f09610451 100644 --- a/lib/Doctrine/Export.php +++ b/lib/Doctrine/Export.php @@ -1221,7 +1221,7 @@ public function exportClasses(array $classes) } } - $connection->commit(); + Doctrine_TransactionHelper::commitIfInTransaction($connection); } } diff --git a/lib/Doctrine/Migration.php b/lib/Doctrine/Migration.php index 0208ce540..f14c16c61 100644 --- a/lib/Doctrine/Migration.php +++ b/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,21 @@ 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 +432,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 +555,4 @@ protected function _createMigrationTable() return false; } } -} \ No newline at end of file +} diff --git a/lib/Doctrine/TransactionHelper.php b/lib/Doctrine/TransactionHelper.php new file mode 100644 index 000000000..c594a91ff --- /dev/null +++ b/lib/Doctrine/TransactionHelper.php @@ -0,0 +1,24 @@ + + */ +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(); + } +} diff --git a/tests/DoctrineTest/Doctrine_UnitTestCase.php b/tests/DoctrineTest/Doctrine_UnitTestCase.php index 3fa7bcbcd..6f7406a70 100644 --- a/tests/DoctrineTest/Doctrine_UnitTestCase.php +++ b/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); diff --git a/tests/MigrationMysqlTestCase.php b/tests/MigrationMysqlTestCase.php new file mode 100644 index 000000000..5e753ac01 --- /dev/null +++ b/tests/MigrationMysqlTestCase.php @@ -0,0 +1,68 @@ +. + */ + +/** + * Doctrine_Migration_TestCase + * + * @package Doctrine + * @author Konsta Vesterinen + * @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')); + } +} diff --git a/tests/run.php b/tests/run.php index d1dd2c7ee..ddb4619ba 100644 --- a/tests/run.php +++ b/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);