From 89455048c30c36131bb9011b5d8e7dd3708e5480 Mon Sep 17 00:00:00 2001 From: Asmir Mustafic Date: Thu, 18 Jun 2020 21:23:39 +0200 Subject: [PATCH 1/3] Merge pull request #988 from oat-sa/fix-latest-version Re-enable alpha1 meaningful message when "latest" already reached. --- .../Tools/Console/Command/MigrateCommand.php | 19 ++++++++++++++----- .../Console/Command/MigrateCommandTest.php | 15 +++++++++++++-- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php b/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php index ce148f2123..6b68c4e710 100644 --- a/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php +++ b/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php @@ -218,11 +218,20 @@ private function errorForAlias(string $versionAlias, bool $allowNoMigration) : i if (in_array($versionAlias, ['first', 'next', 'latest'], true) || strpos($versionAlias, 'current') === 0) { $version = $this->getDependencyFactory()->getVersionAliasResolver()->resolveVersionAlias('current'); - $message = sprintf( - 'The version "%s" couldn\'t be reached, you are at version "%s"', - $versionAlias, - (string) $version - ); + // Allow meaningful message when latest version already reached. + if ($versionAlias === 'next' || $versionAlias === 'latest') { + $message = sprintf( + 'Already at "%s" version ("%s")', + $versionAlias, + (string) $version + ); + } else { + $message = sprintf( + 'The version "%s" couldn\'t be reached, you are at version "%s"', + $versionAlias, + (string) $version + ); + } if ($allowNoMigration) { $this->io->warning($message); diff --git a/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php b/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php index 27caeef11e..bc53dab60a 100644 --- a/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php +++ b/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php @@ -36,6 +36,7 @@ use Symfony\Component\Console\Helper\QuestionHelper; use Symfony\Component\Console\Tester\CommandTester; use function getcwd; +use function in_array; use function sprintf; use function strpos; use function trim; @@ -121,15 +122,25 @@ public function testExecuteAtVersion(string $targetAlias, bool $allowNoMigration ['interactive' => false] ); + $display = trim($this->migrateCommandTester->getDisplay(true)); + $aliases = ['next', 'latest']; + + if (in_array($targetAlias, $aliases, true)) { + $message = '[%s] Already at "%s" version ("%s")'; + } else { + $message = '[%s] The version "%s" couldn\'t be reached, you are at version "%s"'; + } + self::assertStringContainsString( - trim($this->migrateCommandTester->getDisplay(true)), + $display, sprintf( - '[%s] The version "%s" couldn\'t be reached, you are at version "%s"', + $message, ($allowNoMigration ? 'WARNING' : 'ERROR'), $targetAlias, ($executedMigration ?? '0') ) ); + self::assertSame($allowNoMigration ? 0 : 1, $this->migrateCommandTester->getStatusCode()); } From 116647f588ed932829958af565ca25a1dd380599 Mon Sep 17 00:00:00 2001 From: Asmir Mustafic Date: Sat, 20 Jun 2020 20:37:56 +0200 Subject: [PATCH 2/3] Migrate exits with 0 for empty plans as in 2.x In doctrine 2.x the migration command throws an exception if there are not migrations to execute (unless the --allow-no-migrations parameter is passed). In 3.0.0 this was misunderstood and the exception was thrown even if there are registered migrations but there are no migrations to execute for the current migration plan. With this commit we are back to the 2.x strategy. This commit also improves the error messages when the various aliases can not be reached or are already reached. --- UPGRADE.md | 3 - .../Tools/Console/Command/MigrateCommand.php | 85 ++++++++------ .../Console/Command/MigrateCommandTest.php | 110 +++++++++++++----- 3 files changed, 131 insertions(+), 67 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index d84d8ba9c0..52098be5c3 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -13,9 +13,6 @@ please refer to the [Code BC breaks](#code-bc-breaks) section. use `migrations:list` instead. - The `--write-sql` option for `migrations:migrate` and `migrations:execute` does not imply dry-run anymore, use the `--dry-run` parameter instead. -- The `migrations:migrate` command will return a non-zero exit code if there are no migrations to execute, -use the `--allow-no-migration` parameter to have a zero exit code. - ## Migrations table diff --git a/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php b/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php index 6b68c4e710..bcbe45c8da 100644 --- a/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php +++ b/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php @@ -140,10 +140,35 @@ protected function execute(InputInterface $input, OutputInterface $output) : int return 1; } + $migrationRepository = $this->getDependencyFactory()->getMigrationRepository(); + if (count($migrationRepository->getMigrations()) === 0) { + $message = sprintf( + 'The version "%s" couldn\'t be reached, there are no registered migrations.', + $versionAlias + ); + + if ($allowNoMigration) { + $this->io->warning($message); + + return 0; + } + + $this->io->error($message); + + return 1; + } + try { $version = $this->getDependencyFactory()->getVersionAliasResolver()->resolveVersionAlias($versionAlias); - } catch (UnknownMigrationVersion|NoMigrationsToExecute|NoMigrationsFoundWithCriteria $e) { - return $this->errorForAlias($versionAlias, $allowNoMigration); + } catch (UnknownMigrationVersion $e) { + $this->io->error(sprintf( + 'Unknown version: %s', + OutputFormatter::escape($versionAlias) + )); + + return 1; + } catch (NoMigrationsToExecute|NoMigrationsFoundWithCriteria $e) { + return $this->exitForAlias($versionAlias); } $planCalculator = $this->getDependencyFactory()->getMigrationPlanCalculator(); @@ -157,7 +182,7 @@ protected function execute(InputInterface $input, OutputInterface $output) : int $plan = $planCalculator->getPlanUntilVersion($version); if (count($plan) === 0) { - return $this->errorForAlias($versionAlias, $allowNoMigration); + return $this->exitForAlias($versionAlias); } $this->getDependencyFactory()->getLogger()->notice( @@ -213,42 +238,36 @@ private function checkExecutedUnavailableMigrations( return true; } - private function errorForAlias(string $versionAlias, bool $allowNoMigration) : int + private function exitForAlias(string $versionAlias) : int { - if (in_array($versionAlias, ['first', 'next', 'latest'], true) || strpos($versionAlias, 'current') === 0) { - $version = $this->getDependencyFactory()->getVersionAliasResolver()->resolveVersionAlias('current'); - - // Allow meaningful message when latest version already reached. - if ($versionAlias === 'next' || $versionAlias === 'latest') { - $message = sprintf( - 'Already at "%s" version ("%s")', - $versionAlias, - (string) $version - ); - } else { - $message = sprintf( - 'The version "%s" couldn\'t be reached, you are at version "%s"', - $versionAlias, - (string) $version - ); - } - - if ($allowNoMigration) { - $this->io->warning($message); + $version = $this->getDependencyFactory()->getVersionAliasResolver()->resolveVersionAlias('current'); + + // Allow meaningful message when latest version already reached. + if (in_array($versionAlias, ['current', 'latest', 'first'], true)) { + $message = sprintf( + 'Already at the %s version ("%s")', + $versionAlias, + (string) $version + ); - return 0; - } + $this->io->success($message); + } elseif (in_array($versionAlias, ['next', 'prev'], true) || strpos($versionAlias, 'current') === 0) { + $message = sprintf( + 'The version "%s" couldn\'t be reached, you are at version "%s"', + $versionAlias, + (string) $version + ); $this->io->error($message); + } else { + $message = sprintf( + 'You are already at version "%s"', + (string) $version + ); - return 1; + $this->io->success($message); } - $this->io->error(sprintf( - 'Unknown version: %s', - OutputFormatter::escape($versionAlias) - )); - - return 1; + return 0; } } diff --git a/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php b/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php index bc53dab60a..cf800e14e1 100644 --- a/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php +++ b/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php @@ -75,39 +75,83 @@ class MigrateCommandTest extends MigrationTestCase public function testTargetUnknownVersion() : void { - $result = new ExecutionResult(new Version('A')); - $this->storage->complete($result); - $this->migrateCommandTester->execute( - ['version' => 'A'], + ['version' => 'B'], ['interactive' => false] ); - self::assertStringContainsString('[ERROR] Unknown version: A', $this->migrateCommandTester->getDisplay(true)); + self::assertStringContainsString('[ERROR] Unknown version: B', $this->migrateCommandTester->getDisplay(true)); self::assertSame(1, $this->migrateCommandTester->getStatusCode()); } + /** + * @return bool[][] + */ + public function getMigrateWithMigrationsOrWithout() : array + { + return [ + // migrations available, allow-no-migrations + [false, false], + [true, false], + [false, true], + [true, true], + ]; + } + + /** + * @dataProvider getMigrateWithMigrationsOrWithout + */ + public function testMigrateWhenNoMigrationsAvailable(bool $hasMigrations, bool $allowNoMigration) : void + { + $finder = $this->createMock(Finder::class); + $factory = $this->createMock(MigrationFactory::class); + $this->migrationRepository = new FilesystemMigrationsRepository([], [], $finder, $factory); + $this->dependencyFactory->setService(MigrationsRepository::class, $this->migrationRepository); + + if ($hasMigrations) { + $migration = $this->createMock(AbstractMigration::class); + Helper::registerMigrationInstance($this->migrationRepository, new Version('A'), $migration); + } + + $this->migrateCommandTester->execute( + ['--allow-no-migration' => $allowNoMigration], + ['interactive' => false] + ); + + if (! $hasMigrations) { + $display = trim($this->migrateCommandTester->getDisplay(true)); + self::assertStringContainsString( + sprintf( + '[%s] The version "latest" couldn\'t be reached, there are no registered migrations.', + $allowNoMigration ? 'WARNING' : 'ERROR' + ), + $display + ); + } + + self::assertSame($hasMigrations || $allowNoMigration ? 0 : 1, $this->migrateCommandTester->getStatusCode()); + } + /** * @return array> */ public function getTargetAliases() : array { return [ - ['latest', true, 'A'], - ['latest', false, 'A'], - ['first', true, null], - ['first', false, null], - ['next', true, 'A'], - ['next', false, 'A'], - ['current+1', false, 'A'], - ['current+1', true, 'A'], + ['A', 'OK', 'A'], // already at A + ['latest', 'OK', 'A'], // already at latest + ['first', 'OK', null], // already at first + ['next', 'ERROR', 'A'], // already at latest, no next available + ['prev', 'ERROR', null], // no prev, already at first + ['current', 'OK', 'A'], // already at latest, always + ['current+1', 'ERROR', 'A'], // no current+1 ]; } /** * @dataProvider getTargetAliases */ - public function testExecuteAtVersion(string $targetAlias, bool $allowNoMigration, ?string $executedMigration) : void + public function testExecuteAtVersion(string $targetAlias, string $level, ?string $executedMigration) : void { if ($executedMigration !== null) { $result = new ExecutionResult(new Version($executedMigration)); @@ -115,33 +159,37 @@ public function testExecuteAtVersion(string $targetAlias, bool $allowNoMigration } $this->migrateCommandTester->execute( - [ - 'version' => $targetAlias, - '--allow-no-migration' => $allowNoMigration, - ], + ['version' => $targetAlias], ['interactive' => false] ); $display = trim($this->migrateCommandTester->getDisplay(true)); - $aliases = ['next', 'latest']; + $aliases = ['current', 'latest', 'first']; if (in_array($targetAlias, $aliases, true)) { - $message = '[%s] Already at "%s" version ("%s")'; + $message = sprintf( + '[%s] Already at the %s version ("%s")', + $level, + $targetAlias, + ($executedMigration ?? '0') + ); + } elseif ($targetAlias === 'A') { + $message = sprintf( + '[%s] You are already at version "%s"', + $level, + $targetAlias + ); } else { - $message = '[%s] The version "%s" couldn\'t be reached, you are at version "%s"'; - } - - self::assertStringContainsString( - $display, - sprintf( - $message, - ($allowNoMigration ? 'WARNING' : 'ERROR'), + $message = sprintf( + '[%s] The version "%s" couldn\'t be reached, you are at version "%s"', + $level, $targetAlias, ($executedMigration ?? '0') - ) - ); + ); + } - self::assertSame($allowNoMigration ? 0 : 1, $this->migrateCommandTester->getStatusCode()); + self::assertStringContainsString($message, $display); + self::assertSame(0, $this->migrateCommandTester->getStatusCode()); } public function testExecuteUnknownVersion() : void From bd985d370937c3257cdf549be58530b644d49189 Mon Sep 17 00:00:00 2001 From: Asmir Mustafic Date: Sun, 21 Jun 2020 10:44:23 +0200 Subject: [PATCH 3/3] add exit codes to test data-provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Grégoire Paris --- .../Tools/Console/Command/MigrateCommandTest.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php b/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php index cf800e14e1..7730500fb5 100644 --- a/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php +++ b/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php @@ -85,23 +85,23 @@ public function testTargetUnknownVersion() : void } /** - * @return bool[][] + * @return array> */ public function getMigrateWithMigrationsOrWithout() : array { return [ - // migrations available, allow-no-migrations - [false, false], - [true, false], - [false, true], - [true, true], + // migrations available, allow-no-migrations, expected exit code + [false, false, 1], + [true, false, 0], + [false, true, 0], + [true, true, 0], ]; } /** * @dataProvider getMigrateWithMigrationsOrWithout */ - public function testMigrateWhenNoMigrationsAvailable(bool $hasMigrations, bool $allowNoMigration) : void + public function testMigrateWhenNoMigrationsAvailable(bool $hasMigrations, bool $allowNoMigration, int $expectedExitCode) : void { $finder = $this->createMock(Finder::class); $factory = $this->createMock(MigrationFactory::class); @@ -129,7 +129,7 @@ public function testMigrateWhenNoMigrationsAvailable(bool $hasMigrations, bool $ ); } - self::assertSame($hasMigrations || $allowNoMigration ? 0 : 1, $this->migrateCommandTester->getStatusCode()); + self::assertSame($expectedExitCode, $this->migrateCommandTester->getStatusCode()); } /**