Handle absence of value for --all-or-nothing properly

[agustingomes]

Expected behavior:

Upon specification of the option --all-or-nothing wraps multiple migrations into a transaction

Observed behavior

Specifying --all-or-nothing by itself does not cause the desired effect, as the result of the statement $input->getOption('all-or-nothing') is null, and therefore, in the absence of config, this value is ultimately evaluated to false, therefore not wrapping multiple migrations into a single transaction.

Q	A
Type	bug
BC Break	no
Fixed issues	#1279

Summary

[greg0ire]

As stated in #1279 today, this looks to me more like an unintended bug introduced in #1161. Can you take a look and let me know what you think?

[agustingomes]

Hi @greg0ire

I will try to add a failing test case if possible. I'll keep you posted.

[agustingomes]

@greg0ire I added an extra permutation for the data provider of the allOrNothing test, which confirms that the behavior is not as expected.

What would you advice as next step?

[greg0ire]

• revert the doc change
• fix the bug: $allOrNothing = $input->hasOption('all-or-nothing') ? $input->getOption('all-or-nothing') ?? true : null; should IMO do the trick.

[agustingomes]

@greg0ire unfortunately your suggestion alone does not seem to do the trick.

I've checked the hasOption method in Symfony and it seems that method is not a reliable way to know if a user passed the command option or not.

In any case, I'll do a small refactor there to stop using hasOption.

[greg0ire]

Indeed! I think by using a default value different than null (like false?) for all-or-nothing, you might be able to tell whether the option was used or not with getOption

[agustingomes]

I'm not sure @greg0ire

I'll give it some thought over the week, since it seems like relying on the default value of the option may not work, as it can override the Doctrine project configuration.

[greg0ire]

I guess if you set the default to false, you would then have to translate null to true (option provided), and false to null (default value, option not provided), so that the line $allOrNothing = (bool) ($allOrNothing ?? $this->configuration->isAllOrNothing()); continues to work as intended. Bit convoluted, maybe you'll find something more elegant than this. And maybe we should use VALUE_NONE in 4.0, to simplify all this.

[agustingomes]

And maybe we should use VALUE_NONE in 4.0, to simplify all this.

That sounds like a good idea.

[greg0ire]

You can use vendor/bin/phpcbf to fix the cs issues.

[agustingomes]

@greg0ire how do you want to proceed regarding the PHPStan failures? THey seem to be originating from deprecations, and the BC check error I never seen it before.

[greg0ire]

Nice commit messages. I've seen the test failing, so you can squash both commits now, so that all commits pass the build.

[greg0ire]

@greg0ire how do you want to proceed regarding the PHPStan failures? THey seem to be originating from deprecations, and the BC check error I never seen it before.

Don't worry about those, I'll take care of it.

Regarding the BC-check, it seems to have been broken for a while: Nyholm/roave-bc-check-docker#32
Regarding the static analysis issues, I created #1297

[greg0ire]

If the command does not have the option, the value will default to true, but I guess it does not matter since all-or-nothing is not used when the option is not defined.

[e-jevdokimovs]

This is a problem, the default value of MigrationConfiguration property allOrNothing is false, these changes always set the value to true if the option isn't passed at all AND the command doesn't have an all-or-nothing option, for example ExecuteCommand.

[greg0ire]

You mean the option is used although it's not defined in the command? Where?

[e-jevdokimovs]

The option itself isn't used, but ConsoleInputMigratorConfigurationFactory->getMigratorConfiguration() is being called in ExecuteCommand->execute() which in turn creates a MigratorConfiguration with allOrNothing set to true because ExecuteCommand doesn't have an all-or-nothing option which results in the value not defaulting to notprovided.

[e-jevdokimovs]

Basically this method should look like this:

        if (!$input->hasOption('all-or-nothing')) {
            return null;
        }

        $allOrNothingOption = $input->getOption('all-or-nothing');

        if ($allOrNothingOption === 'notprovided') {
            return null;
        }

        return (bool) ($allOrNothingOption ?? true);

[e-jevdokimovs]

Created a PR with the fix: #1308

[greg0ire]

Thanks @agustingomes !

[greg0ire]

And maybe we should use VALUE_NONE in 4.0, to simplify all this.

That sounds like a good idea.

@agustingomes if you want to work on this, you can open a PR on 3.6.x to deprecate passing a value :)

Here is an example:

migrations/lib/Doctrine/Migrations/Tools/TransactionHelper.php

Lines 22 to 32 in 01f89a1

	Deprecation::trigger(
	'doctrine/migrations',
	'https://github.com/doctrine/migrations/issues/1169',
	<<<'DEPRECATION'
	Context: trying to commit a transaction
	Problem: the transaction is already committed, relying on silencing is deprecated.
	Solution: override `AbstractMigration::isTransactional()` so that it returns false.
	Automate that by setting `transactional` to false in the configuration.
	More details at https://www.doctrine-project.org/projects/doctrine-migrations/en/3.2/explanation/implicit-commits.html
	DEPRECATION
	);

[agustingomes]

@greg0ire I'll look into it the next days 👍🏼