Merge pull request #979 from goetas/write-path

Allow to use --write-sql to save the migration execution log to file
doctrine · May 23, 2020 · 42f7c90 · 42f7c90
2 parents af6a764 + f3c7942
commit 42f7c90
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 62 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -10,7 +10,9 @@ please refer to the [Code BC breaks](#code-bc-breaks) section.
   Console output is not covered by the BC promise, so please try not to rely on specific a output.
   Different levels of verbosity are available now (`-v`, `-vv` and `-vvv` ).
 - The `--show-versions` option from `migrations:status` command has been removed, 
-  use `migrations:list` instead. 
+  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.  
 
 ## Migrations table
 

diff --git a/lib/Doctrine/Migrations/Tools/Console/Command/ExecuteCommand.php b/lib/Doctrine/Migrations/Tools/Console/Command/ExecuteCommand.php
@@ -15,6 +15,7 @@
 use function implode;
 use function is_string;
 use function is_writable;
+use function sprintf;
 
 /**
  * The ExecuteCommand class is responsible for executing migration versions up or down manually.
@@ -114,42 +115,39 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
         $this->getDependencyFactory()->getMetadataStorage()->ensureInitialized();
 
         $versions  = $input->getArgument('versions');
-        $path      = $input->getOption('write-sql');
         $direction = $input->getOption('down') !== false
             ? Direction::DOWN
             : Direction::UP;
 
-        $migrator       = $this->getDependencyFactory()->getMigrator();
+        $path = $input->getOption('write-sql') ?? getcwd();
+        if (is_string($path) && ! is_writable($path)) {
+            $this->io->error(sprintf('The path "%s" not writeable!', $path));
+
+            return 1;
+        }
+
         $planCalculator = $this->getDependencyFactory()->getMigrationPlanCalculator();
         $plan           = $planCalculator->getPlanForVersions(array_map(static function (string $version) : Version {
             return new Version($version);
         }, $versions), $direction);
 
-        if ($migratorConfiguration->isDryRun()) {
-            $sql = $migrator->migrate($plan, $migratorConfiguration);
-
-            $path = is_string($path) ? $path : getcwd();
-
-            if (! is_string($path) || ! is_writable($path)) {
-                $this->io->error('Path not writeable!');
-
-                return 1;
-            }
-
-            $writer = $this->getDependencyFactory()->getQueryWriter();
-            $writer->write($path, $direction, $sql);
-
-            return 0;
-        }
-
         $this->getDependencyFactory()->getLogger()->notice(
             'Executing' . ($migratorConfiguration->isDryRun() ? ' (dry-run)' : '') . ' {versions} {direction}',
             [
                 'direction' => $plan->getDirection(),
                 'versions' => implode(', ', $versions),
             ]
         );
-        $migrator->migrate($plan, $migratorConfiguration);
+
+        $migrator = $this->getDependencyFactory()->getMigrator();
+        $sql      = $migrator->migrate($plan, $migratorConfiguration);
+
+        if (is_string($path)) {
+            $writer = $this->getDependencyFactory()->getQueryWriter();
+            $writer->write($path, $direction, $sql);
+        }
+
+        $this->io->newLine();
 
         return 0;
     }

diff --git a/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php b/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php
@@ -132,7 +132,13 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
 
         $allowNoMigration = $input->getOption('allow-no-migration');
         $versionAlias     = $input->getArgument('version');
-        $path             = $input->getOption('write-sql');
+
+        $path = $input->getOption('write-sql') ?? getcwd();
+        if (is_string($path) && ! is_writable($path)) {
+            $this->io->error(sprintf('The path "%s" not writeable!', $path));
+
+            return 1;
+        }
 
         try {
             $version = $this->getDependencyFactory()->getVersionAliasResolver()->resolveVersionAlias($versionAlias);
@@ -154,24 +160,6 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
             return $this->errorForAlias($versionAlias, $allowNoMigration);
         }
 
-        $migrator = $this->getDependencyFactory()->getMigrator();
-        if ($migratorConfiguration->isDryRun()) {
-            $sql = $migrator->migrate($plan, $migratorConfiguration);
-
-            $path = is_string($path) ? $path : getcwd();
-
-            if (! is_string($path) || ! is_writable($path)) {
-                $this->io->error('Path not writeable!');
-
-                return 1;
-            }
-
-            $writer = $this->getDependencyFactory()->getQueryWriter();
-            $writer->write($path, $plan->getDirection(), $sql);
-
-            return 0;
-        }
-
         $this->getDependencyFactory()->getLogger()->notice(
             'Migrating' . ($migratorConfiguration->isDryRun() ? ' (dry-run)' : '') . ' {direction} to {to}',
             [
@@ -180,7 +168,13 @@ protected function execute(InputInterface $input, OutputInterface $output) : int
             ]
         );
 
-        $migrator->migrate($plan, $migratorConfiguration);
+        $migrator = $this->getDependencyFactory()->getMigrator();
+        $sql      = $migrator->migrate($plan, $migratorConfiguration);
+
+        if (is_string($path)) {
+            $writer = $this->getDependencyFactory()->getQueryWriter();
+            $writer->write($path, $plan->getDirection(), $sql);
+        }
 
         $this->io->newLine();
 

diff --git a/lib/Doctrine/Migrations/Tools/Console/ConsoleInputMigratorConfigurationFactory.php b/lib/Doctrine/Migrations/Tools/Console/ConsoleInputMigratorConfigurationFactory.php
@@ -22,11 +22,10 @@ public function getMigratorConfiguration(InputInterface $input) : MigratorConfig
     {
         $timeAllQueries = $input->hasOption('query-time') ? (bool) $input->getOption('query-time') : false;
         $dryRun         = $input->hasOption('dry-run') ? (bool) $input->getOption('dry-run') : false;
-        $writeSql       = $input->hasOption('write-sql') ? $input->getOption('write-sql') : false;
         $allOrNothing   = $input->hasOption('all-or-nothing') ? (bool) $input->getOption('all-or-nothing') : $this->configuration->isAllOrNothing();
 
         return (new MigratorConfiguration())
-            ->setDryRun($dryRun || $writeSql !== false)
+            ->setDryRun($dryRun)
             ->setTimeAllQueries($timeAllQueries)
             ->setAllOrNothing($allOrNothing);
     }

diff --git a/tests/Doctrine/Migrations/Tests/Tools/Console/Command/ExecuteCommandTest.php b/tests/Doctrine/Migrations/Tests/Tools/Console/Command/ExecuteCommandTest.php
@@ -47,30 +47,38 @@ class ExecuteCommandTest extends MigrationTestCase
     private $planCalculator;
 
     /**
-     * @param mixed $arg
+     * @param bool|string|null $arg
      *
      * @dataProvider getWriteSqlValues
      */
-    public function testWriteSql($arg, string $path) : void
+    public function testWriteSql(bool $dryRun, $arg, ?string $path) : void
     {
         $this->migrator
             ->expects(self::once())
             ->method('migrate')
-            ->willReturnCallback(static function (MigrationPlanList $planList, MigratorConfiguration $configuration) : array {
-                self::assertTrue($configuration->isDryRun());
+            ->willReturnCallback(static function (MigrationPlanList $planList, MigratorConfiguration $configuration) use ($dryRun) : array {
+                self::assertSame($dryRun, $configuration->isDryRun());
 
                 return ['A'];
             });
 
-        $this->queryWriter->expects(self::once())
-            ->method('write')
-            ->with($path, 'down', ['A']);
+        if ($arg === false) {
+            $this->queryWriter
+                ->expects(self::never())
+                ->method('write');
+        } else {
+            $this->queryWriter
+                ->expects(self::once())
+                ->method('write')
+                ->with($path, 'down', ['A']);
+        }
 
         $this->executeCommandTester->execute([
             'versions' => ['1'],
             '--down' => true,
             '--write-sql' => $arg,
-        ]);
+            '--dry-run' => $dryRun,
+        ], ['interactive' => false]);
 
         self::assertSame(0, $this->executeCommandTester->getStatusCode());
     }
@@ -81,8 +89,14 @@ public function testWriteSql($arg, string $path) : void
     public function getWriteSqlValues() : array
     {
         return [
-            [true, getcwd()],
-            [ __DIR__ . '/_files', __DIR__ . '/_files'],
+            // dry-run, write-path, path
+            [true, false, null],
+            [true, null, getcwd()],
+            [true,  __DIR__ . '/_files', __DIR__ . '/_files'],
+
+            [false, false, null],
+            [false, null, getcwd()],
+            [false,  __DIR__ . '/_files', __DIR__ . '/_files'],
         ];
     }
 

diff --git a/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php b/tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrateCommandTest.php
@@ -168,28 +168,40 @@ public function testExecutedUnavailableMigrationsCancel() : void
     }
 
     /**
-     * @param mixed $arg
+     * @param bool|string|null $arg
      *
      * @dataProvider getWriteSqlValues
      */
-    public function testExecuteWriteSql($arg, string $path) : void
+    public function testExecuteWriteSql(bool $dryRun, $arg, ?string $path) : void
     {
         $migrator = $this->createMock(DbalMigrator::class);
 
         $this->dependencyFactory->setService(Migrator::class, $migrator);
 
         $migrator->expects(self::once())
             ->method('migrate')
-            ->willReturnCallback(static function (MigrationPlanList $planList, MigratorConfiguration $configuration) : array {
+            ->willReturnCallback(static function (MigrationPlanList $planList, MigratorConfiguration $configuration) use ($dryRun) : array {
+                self::assertSame($dryRun, $configuration->isDryRun());
+
                 return ['A'];
             });
 
-        $this->queryWriter->expects(self::once())
-            ->method('write')
-            ->with($path, 'up', ['A']);
+        if ($arg === false) {
+            $this->queryWriter
+                ->expects(self::never())
+                ->method('write');
+        } else {
+            $this->queryWriter
+                ->expects(self::once())
+                ->method('write')
+                ->with($path, 'up', ['A']);
+        }
 
         $this->migrateCommandTester->execute(
-            ['--write-sql' => $arg],
+            [
+                '--write-sql' => $arg,
+                '--dry-run' => $dryRun,
+            ],
             ['interactive' => false]
         );
         self::assertSame(0, $this->migrateCommandTester->getStatusCode());
@@ -201,8 +213,14 @@ public function testExecuteWriteSql($arg, string $path) : void
     public function getWriteSqlValues() : array
     {
         return [
-            [true, getcwd()],
-            [ __DIR__ . '/_files', __DIR__ . '/_files'],
+            // dry-run, write-path, path
+            [true, false, null],
+            [true, null, getcwd()],
+            [true,  __DIR__ . '/_files', __DIR__ . '/_files'],
+
+            [false, false, null],
+            [false, null, getcwd()],
+            [false,  __DIR__ . '/_files', __DIR__ . '/_files'],
         ];
     }