From 2fc5665ee37ad48b446e998b749f0b94ade941ed Mon Sep 17 00:00:00 2001 From: Agustin Gomes Date: Sun, 25 Aug 2024 20:13:43 +0200 Subject: [PATCH] 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: 4da00c540672d687b8efa7b5f0c2b50107845379 533595134247c0c9f2e16c9a17fd1776a30303a4 5038099da818718703586804cc5bf890aec9086f 799f16d458a93aaadaec29012d8b8b425131fa1d f726a5fdfeb9f61db594b436b62dbeeb5e925308 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 | 13 ++++--- src/Tools/Console/Command/MigrateCommand.php | 6 ++++ ...nsoleInputMigratorConfigurationFactory.php | 34 +++++++++++++++---- .../InvalidAllOrNothingConfiguration.php | 16 +++++++++ .../Console/Command/ExecuteCommandTest.php | 4 +-- .../Console/Command/MigrateCommandTest.php | 17 +++++++++- 6 files changed, 76 insertions(+), 14 deletions(-) create mode 100644 src/Tools/Console/InvalidAllOrNothingConfiguration.php diff --git a/docs/en/reference/configuration.rst b/docs/en/reference/configuration.rst index 7b92c7520..322d77ca0 100644 --- a/docs/en/reference/configuration.rst +++ b/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 enable all the migrations to complete together, you can also set this option from the command line with +the ``migrate`` command and 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 disable all the migrations to complete together, you can also set this option from the command line with +the ``migrate`` command and 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 ------------------------ diff --git a/src/Tools/Console/Command/MigrateCommand.php b/src/Tools/Console/Command/MigrateCommand.php index 4394ac026..956a2fb4a 100644 --- a/src/Tools/Console/Command/MigrateCommand.php +++ b/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 %command.name% command executes a migration to a specified version or the latest available version: diff --git a/src/Tools/Console/ConsoleInputMigratorConfigurationFactory.php b/src/Tools/Console/ConsoleInputMigratorConfigurationFactory.php index fffc450ba..f899bbbf8 100644 --- a/src/Tools/Console/ConsoleInputMigratorConfigurationFactory.php +++ b/src/Tools/Console/ConsoleInputMigratorConfigurationFactory.php @@ -31,30 +31,50 @@ public function getMigratorConfiguration(InputInterface $input): MigratorConfigu private function determineAllOrNothingValueFrom(InputInterface $input): bool|null { - $allOrNothingOption = null; + $enableAllOrNothingOption = null; + $disableAllOrNothingOption = null; + + /** + * 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 + */ + 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'); + $enableAllOrNothingOption = $input->getOption('all-or-nothing'); + } + + 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 ($wasOptionExplicitlyPassed && ($enableAllOrNothingOption !== null && $enableAllOrNothingOption !== self::ABSENT_CONFIG_VALUE)) { 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, }; } } diff --git a/src/Tools/Console/InvalidAllOrNothingConfiguration.php b/src/Tools/Console/InvalidAllOrNothingConfiguration.php new file mode 100644 index 000000000..742b6e4d1 --- /dev/null +++ b/src/Tools/Console/InvalidAllOrNothingConfiguration.php @@ -0,0 +1,16 @@ +executeCommandTester->getStatusCode()); } - public function testExecuteAllOrNothingDefaultsToFalse(): void + public function testExecuteAllOrNothingDefaultsToTrue(): void { $this->executeCommandTester->setInputs(['yes']); @@ -180,7 +180,7 @@ public function testExecuteAllOrNothingDefaultsToFalse(): void ->expects(self::once()) ->method('migrate') ->willReturnCallback(static function (MigrationPlanList $planList, MigratorConfiguration $configuration): array { - self::assertFalse($configuration->isAllOrNothing()); + self::assertTrue($configuration->isAllOrNothing()); return ['A']; }); diff --git a/tests/Tools/Console/Command/MigrateCommandTest.php b/tests/Tools/Console/Command/MigrateCommandTest.php index dd3876f71..94d394cb9 100644 --- a/tests/Tools/Console/Command/MigrateCommandTest.php +++ b/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; @@ -381,7 +382,8 @@ 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]; @@ -391,6 +393,19 @@ public static function allOrNothing(): Generator yield [false, [], 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 { $result = new ExecutionResult(new Version('345'));