Merge pull request #1444 from agustingomes/3.7.x

GH-1443: Fix default option when `--all-or-nothing` option is used as intended

docs/en/reference/configuration.rst

@@ -243,18 +243,23 @@ Setting ``transactional`` to ``false`` will disable that.
 From the Command Line
 ~~~~~~~~~~~~~~~~~~~~~
 
-You can also set this option from the command line with the ``migrate`` command and the ``--all-or-nothing`` option:
+To override the configuration and explicitly enable All or Nothing Transaction from the command line,
+use the ``--all-or-nothing`` option:
 
 .. code-block:: sh
 
     $ ./vendor/bin/doctrine-migrations migrate --all-or-nothing
 
-If you have it enabled at the configuration level and want to change it for an individual migration you can
-pass a value of ``0`` or ``1`` to ``--all-or-nothing``.
+.. note::
+
+    Passing options to --all-or-nothing is deprecated from 3.7.x, and will not be allowed in 4.x
+
+To override the configuration and explicitly disable All or Nothing Transaction from the command line,
+use the ``--no-all-or-nothing`` option:
 
 .. code-block:: sh
 
-    $ ./vendor/bin/doctrine-migrations migrate --all-or-nothing=0
+    $ ./vendor/bin/doctrine-migrations migrate --no-all-or-nothing
 
 Connection Configuration
 ------------------------

src/Tools/Console/Command/MigrateCommand.php

@@ -81,6 +81,12 @@ protected function configure(): void
                 'Wrap the entire migration in a transaction.',
                 ConsoleInputMigratorConfigurationFactory::ABSENT_CONFIG_VALUE,
             )
+            ->addOption(
+                'no-all-or-nothing',
+                null,
+                InputOption::VALUE_NONE,
+                'Disable wrapping the entire migration in a transaction.',
+            )
             ->setHelp(<<<'EOT'
 The <info>%command.name%</info> command executes a migration to a specified version or the latest available version:
 

src/Tools/Console/ConsoleInputMigratorConfigurationFactory.php

@@ -31,30 +31,55 @@ public function getMigratorConfiguration(InputInterface $input): MigratorConfigu
 
     private function determineAllOrNothingValueFrom(InputInterface $input): bool|null
     {
-        $allOrNothingOption        = null;
+        $enableAllOrNothingOption  = self::ABSENT_CONFIG_VALUE;
+        $disableAllOrNothingOption = null;
+
+        if ($input->hasOption('no-all-or-nothing')) {
+            $disableAllOrNothingOption = $input->getOption('no-all-or-nothing');
+        }
+
         $wasOptionExplicitlyPassed = $input->hasOption('all-or-nothing');
 
         if ($wasOptionExplicitlyPassed) {
-            $allOrNothingOption = $input->getOption('all-or-nothing');
+            /**
+             * Due to this option being able to receive optional values, its behavior is tricky:
+             * - when `--all-or-nothing` option is not provided, the default is set to self::ABSENT_CONFIG_VALUE
+             * - when `--all-or-nothing` option is provided without values, this will be `null`
+             * - when `--all-or-nothing` option is provided with a value, we get the provided value
+             */
+            $enableAllOrNothingOption = $input->getOption('all-or-nothing');
+        }
+
+        $enableAllOrNothingDeprecation = match ($enableAllOrNothingOption) {
+            self::ABSENT_CONFIG_VALUE, null => false,
+            default => true,
+        };
+
+        if ($enableAllOrNothingOption !== self::ABSENT_CONFIG_VALUE && $disableAllOrNothingOption === true) {
+            throw InvalidAllOrNothingConfiguration::new();
+        }
+
+        if ($disableAllOrNothingOption === true) {
+            return false;
         }
 
-        if ($wasOptionExplicitlyPassed && ($allOrNothingOption !== null && $allOrNothingOption !== self::ABSENT_CONFIG_VALUE)) {
+        if ($enableAllOrNothingDeprecation) {
             Deprecation::trigger(
                 'doctrine/migrations',
                 'https://github.com/doctrine/migrations/issues/1304',
                 <<<'DEPRECATION'
                     Context: Passing values to option `--all-or-nothing`
                     Problem: Passing values is deprecated
-                    Solution: If you need to disable the behavior, omit the option,
+                    Solution: If you need to disable the behavior, use --no-all-or-nothing,
                     otherwise, pass the option without a value
                     DEPRECATION,
             );
         }
 
-        return match ($allOrNothingOption) {
+        return match ($enableAllOrNothingOption) {
             self::ABSENT_CONFIG_VALUE => null,
-            null => false,
-            default => (bool) $allOrNothingOption,
+            null => true,
+            default => (bool) $enableAllOrNothingOption,
         };
     }
 }

src/Tools/Console/InvalidAllOrNothingConfiguration.php

@@ -0,0 +1,16 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Migrations\Tools\Console;
+
+use Doctrine\Migrations\Configuration\Exception\ConfigurationException;
+use LogicException;
+
+final class InvalidAllOrNothingConfiguration extends LogicException implements ConfigurationException
+{
+    public static function new(): self
+    {
+        return new self('Providing --all-or-nothing and --no-all-or-nothing simultaneously is forbidden.');
+    }
+}

tests/Configuration/ConfigurationTest.php

@@ -93,4 +93,18 @@ public function testMigrationOrganizationCanBeReset(): void
         self::assertFalse($config->areMigrationsOrganizedByYearAndMonth());
         self::assertFalse($config->areMigrationsOrganizedByYear());
     }
+
+    public function testTransactionalConfigDefaultOption(): void
+    {
+        $config = new Configuration();
+
+        self::assertTrue($config->isTransactional());
+    }
+
+    public function testAllOrNothingConfigDefaultOption(): void
+    {
+        $config = new Configuration();
+
+        self::assertFalse($config->isAllOrNothing());
+    }
 }

tests/Tools/Console/Command/MigrateCommandTest.php

@@ -26,6 +26,7 @@
 use Doctrine\Migrations\Tests\Helper;
 use Doctrine\Migrations\Tests\MigrationTestCase;
 use Doctrine\Migrations\Tools\Console\Command\MigrateCommand;
+use Doctrine\Migrations\Tools\Console\InvalidAllOrNothingConfiguration;
 use Doctrine\Migrations\Version\AlphabeticalComparator;
 use Doctrine\Migrations\Version\ExecutionResult;
 use Doctrine\Migrations\Version\MigrationFactory;
@@ -343,11 +344,13 @@ public function testExecuteMigrateDown(): void
      *
      * @dataProvider allOrNothing
      */
-    public function testExecuteMigrateAllOrNothing(bool $default, array $input, bool $expected, bool $expectDeprecation = true): void
+    public function testExecuteMigrateAllOrNothing(bool|null $default, array $input, bool $expected, bool $expectDeprecation = true): void
     {
         $migrator = $this->createMock(DbalMigrator::class);
         $this->dependencyFactory->setService(Migrator::class, $migrator);
-        $this->configuration->setAllOrNothing($default);
+        if ($default !== null) {
+            $this->configuration->setAllOrNothing($default);
+        }
 
         $migrator->expects(self::once())
             ->method('migrate')
@@ -371,7 +374,7 @@ public function testExecuteMigrateAllOrNothing(bool $default, array $input, bool
         self::assertSame(0, $this->migrateCommandTester->getStatusCode());
     }
 
-    /** @psalm-return Generator<array{0: bool, 1: array<string, bool|int|string|null>, 2: bool, 3?: bool}> */
+    /** @psalm-return Generator<array{0: bool|null, 1: array<string, bool|int|string|null>, 2: bool, 3?: bool}> */
     public static function allOrNothing(): Generator
     {
         yield [false, ['--all-or-nothing' => false], false];
@@ -381,14 +384,29 @@ public static function allOrNothing(): Generator
         yield [false, ['--all-or-nothing' => true], true];
         yield [false, ['--all-or-nothing' => 1], true];
         yield [false, ['--all-or-nothing' => '1'], true];
-        yield [false, ['--all-or-nothing' => null], false, false];
+        yield [false, ['--all-or-nothing' => null], true, false];
+        yield [true, ['--no-all-or-nothing' => null], false, false];
 
         yield [true, ['--all-or-nothing' => false], false];
         yield [true, ['--all-or-nothing' => 0], false];
         yield [true, ['--all-or-nothing' => '0'], false];
 
         yield [true, [], true, false];
         yield [false, [], false, false];
+        yield [null, [], false, false];
+    }
+
+    public function testThrowsOnContradictoryAllOrNothingOptions(): void
+    {
+        $this->expectException(InvalidAllOrNothingConfiguration::class);
+
+        $this->migrateCommandTester->execute(
+            [
+                '--all-or-nothing' => null,
+                '--no-all-or-nothing' => null,
+            ],
+            ['interactive' => false],
+        );
     }
 
     public function testExecuteMigrateCancelExecutedUnavailableMigrations(): void