From ce0b654d6ddfbb7c7e64317e213ab537d3d9d2a3 Mon Sep 17 00:00:00 2001 From: Agustin Gomes Date: Fri, 23 Aug 2024 22:02:57 +0200 Subject: [PATCH 1/2] Ad-Hoc: Add extra tests for configuration defaults These extra tests may help in the future to catch unintended behavior changes for the default of these 2 specific settings. --- tests/Configuration/ConfigurationTest.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/Configuration/ConfigurationTest.php b/tests/Configuration/ConfigurationTest.php index e1f8156e52..9b4a38dbff 100644 --- a/tests/Configuration/ConfigurationTest.php +++ b/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()); + } } From ddddf6f45715c28e6afc77ce59986279e842373f Mon Sep 17 00:00:00 2001 From: Agustin Gomes Date: Sun, 25 Aug 2024 20:13:43 +0200 Subject: [PATCH 2/2] 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 7b92c7520e..71eb4b793b 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 4394ac026d..956a2fb4a6 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 fffc450ba2..a9be446af7 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 0000000000..742b6e4d18 --- /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