Skip to content

Commit

Permalink
GH-1443: fix --all-or-nothing default behavior
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
agustingomes committed Aug 25, 2024
1 parent 0b09289 commit 2fc5665
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 14 deletions.
13 changes: 9 additions & 4 deletions docs/en/reference/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
------------------------
Expand Down
6 changes: 6 additions & 0 deletions src/Tools/Console/Command/MigrateCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
34 changes: 27 additions & 7 deletions src/Tools/Console/ConsoleInputMigratorConfigurationFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
}
16 changes: 16 additions & 0 deletions src/Tools/Console/InvalidAllOrNothingConfiguration.php
Original file line number Diff line number Diff line change
@@ -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.');
}
}
4 changes: 2 additions & 2 deletions tests/Tools/Console/Command/ExecuteCommandTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -172,15 +172,15 @@ public function testExecuteCancel(): void
self::assertSame(1, $this->executeCommandTester->getStatusCode());
}

public function testExecuteAllOrNothingDefaultsToFalse(): void
public function testExecuteAllOrNothingDefaultsToTrue(): void
{
$this->executeCommandTester->setInputs(['yes']);

$this->migrator
->expects(self::once())
->method('migrate')
->willReturnCallback(static function (MigrationPlanList $planList, MigratorConfiguration $configuration): array {
self::assertFalse($configuration->isAllOrNothing());
self::assertTrue($configuration->isAllOrNothing());

return ['A'];
});
Expand Down
17 changes: 16 additions & 1 deletion tests/Tools/Console/Command/MigrateCommandTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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];
Expand All @@ -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'));
Expand Down

0 comments on commit 2fc5665

Please sign in to comment.