From d186b4a926400a9a619c75195effa215bc560d2f 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 | 39 +++++++++++++++---- .../InvalidAllOrNothingConfiguration.php | 16 ++++++++ .../Console/Command/MigrateCommandTest.php | 26 +++++++++++-- 5 files changed, 85 insertions(+), 15 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..71eb4b793 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 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 ------------------------ 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..a9be446af 100644 --- a/src/Tools/Console/ConsoleInputMigratorConfigurationFactory.php +++ b/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, }; } } 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 @@ +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, 2: bool, 3?: bool}> */ + /** @psalm-return Generator, 2: bool, 3?: bool}> */ public static function allOrNothing(): Generator { yield [false, ['--all-or-nothing' => false], false]; @@ -381,7 +384,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]; @@ -389,6 +393,20 @@ public static function allOrNothing(): Generator 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