From 47329a5da4adb1ca080bd8f2b45230ccbcf0923e Mon Sep 17 00:00:00 2001
From: Woody Gilk <woody.gilk@gmail.com>
Date: Tue, 15 Jun 2021 11:01:42 -0500
Subject: [PATCH] Check for all-or-nothing conflict in migrations

Fixes https://github.com/doctrine/migrations/issues/1166
---
 lib/Doctrine/Migrations/DbalMigrator.php      | 11 ++++++++
 .../MigrationConfigurationConflict.php        | 27 +++++++++++++++++++
 .../Migrations/Tests/MigratorTest.php         | 20 ++++++++++++++
 .../MigrationNonTransactional.php             | 24 +++++++++++++++++
 4 files changed, 82 insertions(+)
 create mode 100644 lib/Doctrine/Migrations/Exception/MigrationConfigurationConflict.php
 create mode 100644 tests/Doctrine/Migrations/Tests/Stub/NonTransactional/MigrationNonTransactional.php

diff --git a/lib/Doctrine/Migrations/DbalMigrator.php b/lib/Doctrine/Migrations/DbalMigrator.php
index e41ed28ca2..8271d8bb0f 100644
--- a/lib/Doctrine/Migrations/DbalMigrator.php
+++ b/lib/Doctrine/Migrations/DbalMigrator.php
@@ -5,6 +5,7 @@
 namespace Doctrine\Migrations;
 
 use Doctrine\DBAL\Connection;
+use Doctrine\Migrations\Exception\MigrationConfigurationConflict;
 use Doctrine\Migrations\Metadata\MigrationPlanList;
 use Doctrine\Migrations\Query\Query;
 use Doctrine\Migrations\Tools\BytesFormatter;
@@ -65,6 +66,7 @@ private function executeMigrations(
         $allOrNothing = $migratorConfiguration->isAllOrNothing();
 
         if ($allOrNothing) {
+            $this->assertAllMigrationsAreTransactional($migrationsPlan);
             $this->connection->beginTransaction();
         }
 
@@ -89,6 +91,15 @@ private function executeMigrations(
         return $sql;
     }
 
+    private function assertAllMigrationsAreTransactional(MigrationPlanList $migrationsPlan): void
+    {
+        foreach ($migrationsPlan->getItems() as $plan) {
+            if (! $plan->getMigration()->isTransactional()) {
+                throw MigrationConfigurationConflict::migrationIsNotTransactional($plan->getMigration());
+            }
+        }
+    }
+
     /**
      * @return array<string, Query[]>
      */
diff --git a/lib/Doctrine/Migrations/Exception/MigrationConfigurationConflict.php b/lib/Doctrine/Migrations/Exception/MigrationConfigurationConflict.php
new file mode 100644
index 0000000000..2458160937
--- /dev/null
+++ b/lib/Doctrine/Migrations/Exception/MigrationConfigurationConflict.php
@@ -0,0 +1,27 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Migrations\Exception;
+
+use Doctrine\Migrations\AbstractMigration;
+use UnexpectedValueException;
+
+use function get_class;
+use function sprintf;
+
+final class MigrationConfigurationConflict extends UnexpectedValueException implements MigrationException
+{
+    public static function migrationIsNotTransactional(AbstractMigration $migration): self
+    {
+        return new self(sprintf(
+            <<<'EXCEPTION'
+Context: attempting to execute migrations with all-or-nothing enabled
+Problem: migration %s is marked as non-transactional
+Solution: disable all-or-nothing in configuration or by command-line option, or enable transactions for all migrations
+EXCEPTION
+                ,
+            get_class($migration)
+        ));
+    }
+}
diff --git a/tests/Doctrine/Migrations/Tests/MigratorTest.php b/tests/Doctrine/Migrations/Tests/MigratorTest.php
index 4ab1277729..ce8a9f6c56 100644
--- a/tests/Doctrine/Migrations/Tests/MigratorTest.php
+++ b/tests/Doctrine/Migrations/Tests/MigratorTest.php
@@ -9,6 +9,7 @@
 use Doctrine\Migrations\Configuration\Configuration;
 use Doctrine\Migrations\DbalMigrator;
 use Doctrine\Migrations\EventDispatcher;
+use Doctrine\Migrations\Exception\MigrationConfigurationConflict;
 use Doctrine\Migrations\Metadata\MigrationPlan;
 use Doctrine\Migrations\Metadata\MigrationPlanList;
 use Doctrine\Migrations\Metadata\Storage\MetadataStorage;
@@ -17,6 +18,7 @@
 use Doctrine\Migrations\Provider\SchemaDiffProvider;
 use Doctrine\Migrations\Tests\Stub\Functional\MigrateNotTouchingTheSchema;
 use Doctrine\Migrations\Tests\Stub\Functional\MigrationThrowsError;
+use Doctrine\Migrations\Tests\Stub\NonTransactional\MigrationNonTransactional;
 use Doctrine\Migrations\Version\DbalExecutor;
 use Doctrine\Migrations\Version\Direction;
 use Doctrine\Migrations\Version\Executor;
@@ -170,4 +172,22 @@ public function testMigrateAllOrNothingRollback(): void
 
         $migrator->migrate($planList, $this->migratorConfiguration);
     }
+
+    public function testMigrateAllOrNothingNonTransactionalMigration(): void
+    {
+        $this->config->addMigrationsDirectory('DoctrineMigrations\\', __DIR__ . '/Stub/NonTransactional');
+
+        $migrator = $this->createTestMigrator();
+
+        $this->migratorConfiguration->setAllOrNothing(true);
+
+        $migration = new MigrationNonTransactional($this->conn, $this->logger);
+        $plan      = new MigrationPlan(new Version(MigrationNonTransactional::class), $migration, Direction::UP);
+        $planList  = new MigrationPlanList([$plan], Direction::UP);
+
+        self::expectException(MigrationConfigurationConflict::class);
+        self::expectExceptionMessage(MigrationNonTransactional::class);
+
+        $migrator->migrate($planList, $this->migratorConfiguration);
+    }
 }
diff --git a/tests/Doctrine/Migrations/Tests/Stub/NonTransactional/MigrationNonTransactional.php b/tests/Doctrine/Migrations/Tests/Stub/NonTransactional/MigrationNonTransactional.php
new file mode 100644
index 0000000000..163cadf409
--- /dev/null
+++ b/tests/Doctrine/Migrations/Tests/Stub/NonTransactional/MigrationNonTransactional.php
@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Migrations\Tests\Stub\NonTransactional;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+class MigrationNonTransactional extends AbstractMigration
+{
+    public function up(Schema $schema): void
+    {
+    }
+
+    public function down(Schema $schema): void
+    {
+    }
+
+    public function isTransactional(): bool
+    {
+        return false;
+    }
+}