Merge pull request #550 from acelaya-forks/feature/fix-db-silent-errors

Feature/fix db silent errors

CHANGELOG.md

@@ -26,6 +26,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 
 * [#512](https://github.com/shlinkio/shlink/issues/512) Fixed query params not being properly forwarded from short URL to long one.
 * [#540](https://github.com/shlinkio/shlink/issues/540) Fixed errors thrown when creating short URLs if the original URL has an internationalized domain name and URL validation is enabled.
+* [#528](https://github.com/shlinkio/shlink/issues/528) Ensured `db:create` and `db:migrate` commands do not silently fail when run as part of `install` or `update`.
 
 
 ## 1.20.0 - 2019-11-02

composer.json

@@ -102,11 +102,9 @@
             "@test:ci",
             "@infect:ci"
         ],
-
         "cs": "phpcs",
         "cs:fix": "phpcbf",
         "stan": "phpstan analyse module/*/src/ module/*/config config docker/config --level=5 -c phpstan.neon",
-
         "test": [
             "@test:unit",
             "@test:db",
@@ -135,20 +133,19 @@
         "test:db:maria": "DB_DRIVER=maria composer test:db:sqlite",
         "test:db:postgres": "DB_DRIVER=postgres composer test:db:sqlite",
         "test:api": "bin/test/run-api-tests.sh",
-
         "test:pretty": [
             "@test",
             "phpdbg -qrr vendor/bin/phpcov merge build --html build/html"
         ],
         "test:unit:pretty": "phpdbg -qrr vendor/bin/phpunit --order-by=random --colors=always --coverage-html build/coverage",
-
         "infect": "infection --threads=4 --min-msi=75 --log-verbosity=default --only-covered",
         "infect:ci": "infection --threads=4 --min-msi=75 --log-verbosity=default --only-covered --coverage=build",
         "infect:show": "infection --threads=4 --min-msi=75 --log-verbosity=default --only-covered --show-mutations",
         "infect:test": [
             "@test:unit:ci",
             "@infect:ci"
-        ]
+        ],
+        "clean:dev": "rm -f data/database.sqlite && rm -f config/params/generated_config.php"
     },
     "scripts-descriptions": {
         "ci": "<fg=blue;options=bold>Alias for \"cs\", \"stan\", \"test:ci\" and \"infect:ci\"</>",
@@ -171,7 +168,8 @@
         "infect": "<fg=blue;options=bold>Checks unit tests quality applying mutation testing</>",
         "infect:ci": "<fg=blue;options=bold>Checks unit tests quality applying mutation testing with existing reports and logs</>",
         "infect:show": "<fg=blue;options=bold>Checks unit tests quality applying mutation testing and shows applied mutators</>",
-        "infect:test": "<fg=blue;options=bold>Checks unit tests quality applying mutation testing</>"
+        "infect:test": "<fg=blue;options=bold>Checks unit tests quality applying mutation testing</>",
+        "clean:dev": "<fg=blue;options=bold>Deletes artifacts which are gitignored and could affect dev env</>"
     },
     "config": {
         "sort-packages": true

module/CLI/src/Command/Db/AbstractDatabaseCommand.php

@@ -5,6 +5,7 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\CLI\Command\Db;
 
 use Shlinkio\Shlink\CLI\Command\Util\AbstractLockedCommand;
+use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig;
 use Symfony\Component\Console\Helper\ProcessHelper;
 use Symfony\Component\Console\Output\OutputInterface;
 use Symfony\Component\Lock\Factory as Locker;
@@ -29,6 +30,11 @@ abstract class AbstractDatabaseCommand extends AbstractLockedCommand
     protected function runPhpCommand(OutputInterface $output, array $command): void
     {
         array_unshift($command, $this->phpBinary);
-        $this->processHelper->run($output, $command, null, null, $output->getVerbosity());
+        $this->processHelper->mustRun($output, $command);
+    }
+
+    protected function getLockConfig(): LockedCommandConfig
+    {
+        return new LockedCommandConfig($this->getName(), true);
     }
 }

module/CLI/src/Command/Db/CreateDatabaseCommand.php

@@ -5,7 +5,6 @@ declare(strict_types=1);
 namespace Shlinkio\Shlink\CLI\Command\Db;
 
 use Doctrine\DBAL\Connection;
-use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig;
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
 use Symfony\Component\Console\Helper\ProcessHelper;
 use Symfony\Component\Console\Input\InputInterface;
@@ -19,8 +18,8 @@ use function Functional\contains;
 class CreateDatabaseCommand extends AbstractDatabaseCommand
 {
     public const NAME = 'db:create';
-    public const DOCTRINE_HELPER_SCRIPT = 'vendor/doctrine/orm/bin/doctrine.php';
+    public const DOCTRINE_SCRIPT = 'vendor/doctrine/orm/bin/doctrine.php';
-    public const DOCTRINE_HELPER_COMMAND = 'orm:schema-tool:create';
+    public const DOCTRINE_CREATE_SCHEMA_COMMAND = 'orm:schema-tool:create';
 
     /** @var Connection */
     private $regularConn;
@@ -61,7 +60,7 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
 
         // Create database
         $io->writeln('<fg=blue>Creating database tables...</>');
-        $this->runPhpCommand($output, [self::DOCTRINE_HELPER_SCRIPT, self::DOCTRINE_HELPER_COMMAND]);
+        $this->runPhpCommand($output, [self::DOCTRINE_SCRIPT, self::DOCTRINE_CREATE_SCHEMA_COMMAND]);
         $io->success('Database properly created!');
 
         return ExitCodes::EXIT_SUCCESS;
@@ -87,13 +86,8 @@ class CreateDatabaseCommand extends AbstractDatabaseCommand
     private function schemaExists(): bool
     {
         // If at least one of the shlink tables exist, we will consider the database exists somehow.
-        // Any inconsistency will be taken care by the migrations
+        // Any inconsistency should be taken care by the migrations
         $schemaManager = $this->regularConn->getSchemaManager();
         return ! empty($schemaManager->listTableNames());
     }
-
-    protected function getLockConfig(): LockedCommandConfig
-    {
-        return new LockedCommandConfig($this->getName(), true);
-    }
 }

module/CLI/src/Command/Db/MigrateDatabaseCommand.php

@@ -4,7 +4,6 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\CLI\Command\Db;
 
-use Shlinkio\Shlink\CLI\Command\Util\LockedCommandConfig;
 use Shlinkio\Shlink\CLI\Util\ExitCodes;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
@@ -13,8 +12,8 @@ use Symfony\Component\Console\Style\SymfonyStyle;
 class MigrateDatabaseCommand extends AbstractDatabaseCommand
 {
     public const NAME = 'db:migrate';
-    public const DOCTRINE_HELPER_SCRIPT = 'vendor/doctrine/migrations/bin/doctrine-migrations.php';
+    public const DOCTRINE_MIGRATIONS_SCRIPT = 'vendor/doctrine/migrations/bin/doctrine-migrations.php';
-    public const DOCTRINE_HELPER_COMMAND = 'migrations:migrate';
+    public const DOCTRINE_MIGRATE_COMMAND = 'migrations:migrate';
 
     protected function configure(): void
     {
@@ -28,14 +27,9 @@ class MigrateDatabaseCommand extends AbstractDatabaseCommand
         $io = new SymfonyStyle($input, $output);
 
         $io->writeln('<fg=blue>Migrating database...</>');
-        $this->runPhpCommand($output, [self::DOCTRINE_HELPER_SCRIPT, self::DOCTRINE_HELPER_COMMAND]);
+        $this->runPhpCommand($output, [self::DOCTRINE_MIGRATIONS_SCRIPT, self::DOCTRINE_MIGRATE_COMMAND]);
         $io->success('Database properly migrated!');
 
         return ExitCodes::EXIT_SUCCESS;
     }
-
-    protected function getLockConfig(): LockedCommandConfig
-    {
-        return new LockedCommandConfig($this->getName(), true);
-    }
 }

module/CLI/test/Command/Db/CreateDatabaseCommandTest.php

@@ -116,10 +116,10 @@ class CreateDatabaseCommandTest extends TestCase
         $createDatabase = $this->schemaManager->createDatabase($shlinkDatabase)->will(function () {
         });
         $listTables = $this->schemaManager->listTableNames()->willReturn([]);
-        $runCommand = $this->processHelper->run(Argument::type(OutputInterface::class), [
+        $runCommand = $this->processHelper->mustRun(Argument::type(OutputInterface::class), [
             '/usr/local/bin/php',
-            CreateDatabaseCommand::DOCTRINE_HELPER_SCRIPT,
+            CreateDatabaseCommand::DOCTRINE_SCRIPT,
-            CreateDatabaseCommand::DOCTRINE_HELPER_COMMAND,
+            CreateDatabaseCommand::DOCTRINE_CREATE_SCHEMA_COMMAND,
         ], Argument::cetera());
 
         $this->commandTester->execute([]);

module/CLI/test/Command/Db/MigrateDatabaseCommandTest.php

@@ -48,36 +48,20 @@ class MigrateDatabaseCommandTest extends TestCase
         $this->commandTester = new CommandTester($command);
     }
 
-    /**
+    /** @test */
-     * @test
+    public function migrationsCommandIsRunWithProperVerbosity(): void
-     * @dataProvider provideVerbosities
-     */
-    public function migrationsCommandIsRunWithProperVerbosity(int $verbosity): void
     {
-        $runCommand = $this->processHelper->run(Argument::type(OutputInterface::class), [
+        $runCommand = $this->processHelper->mustRun(Argument::type(OutputInterface::class), [
             '/usr/local/bin/php',
-            MigrateDatabaseCommand::DOCTRINE_HELPER_SCRIPT,
+            MigrateDatabaseCommand::DOCTRINE_MIGRATIONS_SCRIPT,
-            MigrateDatabaseCommand::DOCTRINE_HELPER_COMMAND,
+            MigrateDatabaseCommand::DOCTRINE_MIGRATE_COMMAND,
-        ], null, null, $verbosity);
+        ], Argument::cetera());
 
-        $this->commandTester->execute([], [
+        $this->commandTester->execute([]);
-            'verbosity' => $verbosity,
-        ]);
         $output = $this->commandTester->getDisplay();
 
-        if ($verbosity >= OutputInterface::VERBOSITY_VERBOSE) {
+        $this->assertStringContainsString('Migrating database...', $output);
-            $this->assertStringContainsString('Migrating database...', $output);
+        $this->assertStringContainsString('Database properly migrated!', $output);
-            $this->assertStringContainsString('Database properly migrated!', $output);
-        }
         $runCommand->shouldHaveBeenCalledOnce();
     }
-
-    public function provideVerbosities(): iterable
-    {
-        yield 'debug' => [OutputInterface::VERBOSITY_DEBUG];
-        yield 'normal' => [OutputInterface::VERBOSITY_NORMAL];
-        yield 'quiet' => [OutputInterface::VERBOSITY_QUIET];
-        yield 'verbose' => [OutputInterface::VERBOSITY_VERBOSE];
-        yield 'very verbose' => [OutputInterface::VERBOSITY_VERY_VERBOSE];
-    }
 }

phpstan.neon

@@ -4,3 +4,4 @@ parameters:
 		- '#ObjectManager::flush()#'
 		- '#Undefined variable: \$metadata#'
 		- '#AbstractQuery::setParameters()#'
+		- '#mustRun()#'