Don't Prompt the User for Confirmation on the migrations:migrate Co…

…mmand When There is Nothing to Do (#480) * Improve the Tests for the Migrate Command This brings them inline with some of the other, newer command tests that use Symfony's command tester. There's more coverage as well. Done so some changes can be made to `MigrateCommand`. * Bail out of MigrationCommand::execute Early on --write-sql Removes a level of nesting from the rest of the method so some changes can be a bit nicer later on. * Move the "No Migrations..." Message into Migrations::migrate Provides the same output to the user as before. * Add a Confirmation Callback to Migration::migrate Provides a way to hook into the migration and ask the user for confirmation. Right now that happens further up stream in a console command. * Convert MigrateCommand to use the Confirmation Callback Means there's no longer a big warning or confirmation when no migrations are to be executed. * Re-Add Some Newlines Between @param, @return, and @throws * Don't Return `false` from Migrations::migrate on Cancel Because it's not really an error. This also reworks the console command to keep the same interface: returns a 1 status code on cancel and outputs "migrations cancelled"
doctrine · Nov 18, 2016 · 073195c · 073195c
1 parent dbbcc24
commit 073195c
Show file tree

Hide file tree

Showing 6 changed files with 348 additions and 114 deletions.
diff --git a/lib/Doctrine/DBAL/Migrations/Migration.php b/lib/Doctrine/DBAL/Migrations/Migration.php
@@ -118,12 +118,13 @@ public function setNoMigrationException($noMigrationException = false)
      * @param string  $to             The version to migrate to.
      * @param boolean $dryRun         Whether or not to make this a dry run and not execute anything.
      * @param boolean $timeAllQueries Measuring or not the execution time of each SQL query.
+     * @param callable|null $confirm A callback to confirm whether the migrations should be executed.
      *
-     * @return array $sql     The array of migration sql statements
+     * @return array An array of migration sql statements. This will be empty if the the $confirm callback declines to execute the migration
      *
      * @throws MigrationException
      */
-    public function migrate($to = null, $dryRun = false, $timeAllQueries = false)
+    public function migrate($to = null, $dryRun = false, $timeAllQueries = false, callable $confirm = null)
     {
         /**
          * If no version to migrate to is given we default to the last available one.
@@ -156,6 +157,10 @@ public function migrate($to = null, $dryRun = false, $timeAllQueries = false)
          * to signify that there is nothing left to do.
          */
         if ($from === $to && empty($migrationsToExecute) && !empty($migrations)) {
+            return $this->noMigrations();
+        }
+
+        if (!$dryRun && false === $this->migrationsCanExecute($confirm)) {
             return [];
         }
 
@@ -168,6 +173,8 @@ public function migrate($to = null, $dryRun = false, $timeAllQueries = false)
          */
         if (empty($migrationsToExecute) && !$this->noMigrationException) {
             throw MigrationException::noMigrationsToExecute();
+        } elseif (empty($migrationsToExecute)) {
+            return $this->noMigrations();
         }
 
         $sql = [];
@@ -185,4 +192,15 @@ public function migrate($to = null, $dryRun = false, $timeAllQueries = false)
 
         return $sql;
     }
+
+    private function noMigrations()
+    {
+        $this->outputWriter->write('<comment>No migrations to execute.</comment>');
+        return [];
+    }
+
+    private function migrationsCanExecute(callable $confirm=null)
+    {
+        return null === $confirm ? true : $confirm();
+    }
 }
diff --git a/lib/Doctrine/DBAL/Migrations/Tools/Console/Command/MigrateCommand.php b/lib/Doctrine/DBAL/Migrations/Tools/Console/Command/MigrateCommand.php
@@ -83,7 +83,7 @@ protected function configure()
     public function execute(InputInterface $input, OutputInterface $output)
     {
         $configuration = $this->getMigrationConfiguration($input, $output);
-        $migration = new Migration($configuration);
+        $migration = $this->createMigration($configuration);
 
         $this->outputHeader($configuration, $output);
 
@@ -124,30 +124,40 @@ public function execute(InputInterface $input, OutputInterface $output)
         if ($path = $input->getOption('write-sql')) {
             $path = is_bool($path) ? getcwd() : $path;
             $migration->writeSqlFile($path, $version);
-        } else {
-            $dryRun = (boolean) $input->getOption('dry-run');
-
-            // warn the user if no dry run and interaction is on
-            if (! $dryRun) {
-                $question = 'WARNING! You are about to execute a database migration'
-                    . ' that could result in schema changes and data lost.'
-                    . ' Are you sure you wish to continue? (y/n)';
-                if (! $this->canExecute($question, $input, $output)) {
-                    $output->writeln('<error>Migration cancelled!</error>');
-
-                    return 1;
-                }
-            }
+            return 0;
+        }
 
-            $migration->setNoMigrationException($input->getOption('allow-no-migration'));
-            $sql = $migration->migrate($version, $dryRun, $timeAllqueries);
+        $dryRun = (boolean) $input->getOption('dry-run');
 
-            if (empty($sql)) {
-                $output->writeln('<comment>No migrations to execute.</comment>');
-            }
+        $cancelled = false;
+        $migration->setNoMigrationException($input->getOption('allow-no-migration'));
+        $result = $migration->migrate($version, $dryRun, $timeAllqueries, function () use ($input, $output, &$cancelled) {
+            $question = 'WARNING! You are about to execute a database migration'
+                . ' that could result in schema changes and data lost.'
+                . ' Are you sure you wish to continue? (y/n)';
+            $canContinue = $this->canExecute($question, $input, $output);
+            $cancelled = !$canContinue;
+
+            return $canContinue;
+        });
+
+        if ($cancelled) {
+            $output->writeln('<error>Migration cancelled!</error>');
+            return 1;
         }
     }
 
+    /**
+     * Create a new migration instance to execute the migrations.
+     *
+     * @param $configuration The configuration with which the migrations will be executed
+     * @return Migration a new migration instance
+     */
+    protected function createMigration(Configuration $configuration)
+    {
+        return new Migration($configuration);
+    }
+
     /**
      * @param string $question
      * @param InputInterface $input

diff --git a/tests/Doctrine/DBAL/Migrations/Tests/MigrationTest.php b/tests/Doctrine/DBAL/Migrations/Tests/MigrationTest.php
@@ -44,6 +44,7 @@ function realpath($path)
 use Doctrine\DBAL\Migrations\Migration;
 use Doctrine\DBAL\Migrations\MigrationException;
 use Doctrine\DBAL\Migrations\OutputWriter;
+use Doctrine\DBAL\Migrations\Tests\Stub\Functional\MigrateNotTouchingTheSchema;
 use \Mockery as m;
 use org\bovigo\vfs\vfsStream;
 use org\bovigo\vfs\vfsStreamFile;
@@ -54,14 +55,16 @@ function realpath($path)
  */
 class MigrationTest extends MigrationTestCase
 {
+    private $conn;
     /** @var Configuration */
     private $config;
 
     protected $output;
 
     protected function setUp()
     {
-        $this->config = new Configuration($this->getSqliteConnection());
+        $this->conn = $this->getSqliteConnection();
+        $this->config = new Configuration($this->conn);
         $this->config->setMigrationsDirectory(__DIR__ . DIRECTORY_SEPARATOR . 'Stub/migration-empty-folder');
         $this->config->setMigrationsNamespace('DoctrineMigrations\\');
     }
@@ -90,10 +93,18 @@ public function testMigrateWithNoMigrationsThrowsException()
 
     public function testMigrateWithNoMigrationsDontThrowsExceptionIfContiniousIntegrationOption()
     {
+        $messages = [];
+        $output = new OutputWriter(function ($msg) use (&$messages) {
+            $messages[] = $msg;
+        });
+        $this->config->setOutputWriter($output);
         $migration = new Migration($this->config);
 
         $migration->setNoMigrationException(true);
         $migration->migrate();
+
+        $this->assertCount(2, $messages, 'should output header and no migrations message');
+        $this->assertContains('No migrations', $messages[1]);
     }
 
     /**
@@ -186,4 +197,63 @@ public function testWriteSqlFileShouldUseStandardCommentMarkerInSql()
         $this->assertRegExp('/^\s*-- Version 1/m', $sqlMigrationFile->getContent());
         $this->assertNotRegExp('/^\s*#/m', $sqlMigrationFile->getContent());
     }
+
+    public function testMigrateWithMigrationsAndAddTheCurrentVersionOutputsANoMigrationsMessage()
+    {
+        $messages = [];
+        $output = new OutputWriter(function ($msg) use (&$messages) {
+            $messages[] = $msg;
+        });
+        $this->config->setOutputWriter($output);
+        $this->config->setMigrationsDirectory(__DIR__.'/Stub/migrations-empty-folder');
+        $this->config->setMigrationsNamespace('DoctrineMigrations\\');
+        $this->config->registerMigration('20160707000000', MigrateNotTouchingTheSchema::class);
+        $this->config->createMigrationTable();
+        $this->conn->insert($this->config->getMigrationsTableName(), [
+            'version' => '20160707000000',
+        ]);
+
+        $migration = new Migration($this->config);
+
+        $migration->migrate();
+
+        $this->assertCount(1, $messages, 'should output the no migrations message');
+        $this->assertContains('No migrations', $messages[0]);
+    }
+
+    public function testMigrateReturnsFalseWhenTheConfirmationIsDeclined()
+    {
+        $this->config->setMigrationsDirectory(__DIR__.'/Stub/migrations-empty-folder');
+        $this->config->setMigrationsNamespace('DoctrineMigrations\\');
+        $this->config->registerMigration('20160707000000', MigrateNotTouchingTheSchema::class);
+        $this->config->createMigrationTable();
+        $called = false;
+        $migration = new Migration($this->config);
+
+        $result = $migration->migrate(null, false, false, function () use (&$called) {
+            $called = true;
+            return false;
+        });
+
+        $this->assertSame([], $result);
+        $this->assertTrue($called, 'should have called the confirmation callback');
+    }
+
+    public function testMigrateWithDryRunDoesNotCallTheConfirmationCallback()
+    {
+        $this->config->setMigrationsDirectory(__DIR__.'/Stub/migrations-empty-folder');
+        $this->config->setMigrationsNamespace('DoctrineMigrations\\');
+        $this->config->registerMigration('20160707000000', MigrateNotTouchingTheSchema::class);
+        $this->config->createMigrationTable();
+        $called = false;
+        $migration = new Migration($this->config);
+
+        $result = $migration->migrate(null, true, false, function () use (&$called) {
+            $called = true;
+            return false;
+        });
+
+        $this->assertFalse($called);
+        $this->assertEquals(['20160707000000' => ['SELECT 1']], $result);
+    }
 }
diff --git a/tests/Doctrine/DBAL/Migrations/Tests/Tools/Console/Command/DialogSupport.php b/tests/Doctrine/DBAL/Migrations/Tests/Tools/Console/Command/DialogSupport.php
@@ -0,0 +1,31 @@
+<?php
+
+namespace Doctrine\DBAL\Migrations\Tests\Tools\Console\Command;
+
+use Symfony\Component\Console\Application;
+use Symfony\Component\Console\Helper\QuestionHelper;
+use Symfony\Component\Console\Helper\DialogHelper;
+
+trait DialogSupport
+{
+    protected $questions, $isDialogHelper;
+
+    protected function configureDialogs(Application $app)
+    {
+        if (class_exists(QuestionHelper::class)) {
+            $this->isDialogHelper = false;
+            $this->questions = $this->getMock(QuestionHelper::class);
+        } else {
+            $this->isDialogHelper = true;
+            $this->questions = $this->getMock(DialogHelper::class);
+        }
+        $app->getHelperSet()->set($this->questions, $this->isDialogHelper ? 'dialog' : 'question');
+    }
+
+    protected function willAskConfirmationAndReturn($bool)
+    {
+        $this->questions->expects($this->once())
+            ->method($this->isDialogHelper ? 'askConfirmation' : 'ask')
+            ->willReturn($bool);
+    }
+}
diff --git a/tests/Doctrine/DBAL/Migrations/Tests/Tools/Console/Command/ExecuteCommandTest.php b/tests/Doctrine/DBAL/Migrations/Tests/Tools/Console/Command/ExecuteCommandTest.php
@@ -10,9 +10,11 @@
 
 class ExecuteCommandTest extends CommandTestCase
 {
+    use DialogSupport;
+
     const VERSION = '20160705000000';
 
-    private $version, $questions, $isDialogHelper;
+    private $version;
 
     public function testWriteSqlCommandOutputsSqlFileToTheCurrentWorkingDirectory()
     {
@@ -94,14 +96,7 @@ protected function setUp()
             ->with(self::VERSION)
             ->willReturn($this->version);
 
-        if (class_exists(QuestionHelper::class)) {
-            $this->isDialogHelper = false;
-            $this->questions = $this->getMock(QuestionHelper::class);
-        } else {
-            $this->isDialogHelper = true;
-            $this->questions = $this->getMock(DialogHelper::class);
-        }
-        $this->app->getHelperSet()->set($this->questions, $this->isDialogHelper ? 'dialog' : 'question');
+        $this->configureDialogs($this->app);
     }
 
     protected function createCommand()
@@ -121,11 +116,4 @@ private function mockWithoutConstructor($cls)
             ->disableOriginalConstructor()
             ->getMock();
     }
-
-    private function willAskConfirmationAndReturn($bool)
-    {
-        $this->questions->expects($this->once())
-            ->method($this->isDialogHelper ? 'askConfirmation' : 'ask')
-            ->willReturn($bool);
-    }
 }