GH-1443: fix --all-or-nothing default behavior

As result of some past changes to improve the way the option was inferred, several attempts to build a mechanism to handle the absence of value of this option were built via the following commits:

4da00c5
5335951
5038099
799f16d
f726a5f

This commit leverages all the iterative improvements introduced along the way, and setting the correct value when the option is correctly specified.

This should bring the behavior inline with what is currently documented, while additionally adding a documentation deprecation informing users that passing a value to that option won't be allowed in 4.x

As an extra addition, this commit also introduces the negated option `--no-all-or-nothing`. The intent is to leverage the native Symfony >= 5.3 [Negatable command options](https://symfony.com/blog/new-in-symfony-5-3-negatable-command-options) in the future once the optional value from `--all-or-nothing` is completely removed.

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/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