From edec5c5a2e3d62b4edb972066ef47cdf4207e030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Tue, 27 Aug 2024 18:03:38 +0200 Subject: [PATCH 1/4] Assert there is no failure The pattern is not dynamic, and we know it works. This addresses an issue reported by PHPStan. --- src/Generator/Generator.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Generator/Generator.php b/src/Generator/Generator.php index fdd19854b..1945879b2 100644 --- a/src/Generator/Generator.php +++ b/src/Generator/Generator.php @@ -9,6 +9,7 @@ use Doctrine\Migrations\Tools\Console\Helper\MigrationDirectoryHelper; use InvalidArgumentException; +use function assert; use function explode; use function file_get_contents; use function file_put_contents; @@ -74,11 +75,14 @@ public function generateMigration( string|null $up = null, string|null $down = null, ): string { - $mch = []; - if (preg_match('~(.*)\\\\([^\\\\]+)~', $fqcn, $mch) === 0) { + $mch = []; + $matchResult = preg_match('~(.*)\\\\([^\\\\]+)~', $fqcn, $mch); + if ($matchResult === 0) { throw new InvalidArgumentException(sprintf('Invalid FQCN')); } + assert($matchResult !== false); + [$fqcn, $namespace, $className] = $mch; $dirs = $this->configuration->getMigrationDirectories(); From b137e61c9699ac519d4933cbab160691a5e5114c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Wed, 28 Aug 2024 11:06:20 +0200 Subject: [PATCH 2/4] Be consistent with how we deal with preg_match's return type I'm a bit undecided on what's the best way of doing this. Let's at least use the same strategy everywhere we can. --- src/Generator/Generator.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Generator/Generator.php b/src/Generator/Generator.php index 1945879b2..7eb5edcd8 100644 --- a/src/Generator/Generator.php +++ b/src/Generator/Generator.php @@ -9,7 +9,6 @@ use Doctrine\Migrations\Tools\Console\Helper\MigrationDirectoryHelper; use InvalidArgumentException; -use function assert; use function explode; use function file_get_contents; use function file_put_contents; @@ -75,14 +74,11 @@ public function generateMigration( string|null $up = null, string|null $down = null, ): string { - $mch = []; - $matchResult = preg_match('~(.*)\\\\([^\\\\]+)~', $fqcn, $mch); - if ($matchResult === 0) { + $mch = []; + if (preg_match('~(.*)\\\\([^\\\\]+)~', $fqcn, $mch) !== 1) { throw new InvalidArgumentException(sprintf('Invalid FQCN')); } - assert($matchResult !== false); - [$fqcn, $namespace, $className] = $mch; $dirs = $this->configuration->getMigrationDirectories(); From ce0b654d6ddfbb7c7e64317e213ab537d3d9d2a3 Mon Sep 17 00:00:00 2001 From: Agustin Gomes Date: Fri, 23 Aug 2024 22:02:57 +0200 Subject: [PATCH 3/4] 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 e1f8156e5..9b4a38dbf 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 4/4] 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