Merge pull request #7196 from yguedidi/improve-install-command

Improve install command

app/config/config_test.yml

@@ -5,6 +5,7 @@ imports:
 
 parameters:
     fosuser_registration: true
+    wallabag_dbname_suffix: '_test'
 
 framework:
     test: ~
@@ -23,7 +24,7 @@ web_profiler:
 
 doctrine:
     dbal:
-        dbname_suffix: '_test' # for MySQL and PostgreSQL
+        dbname_suffix: '%wallabag_dbname_suffix%' # for MySQL and PostgreSQL
         use_savepoints: true
 
     orm:

app/config/parameters_test.yml

@@ -1,2 +1,2 @@
 parameters:
-    database_path: "%kernel.project_dir%/data/db/wallabag_test.sqlite"
+    database_path: "%kernel.project_dir%/data/db/wallabag%wallabag_dbname_suffix%.sqlite"

app/config/services.yml

@@ -109,6 +109,9 @@ services:
     Doctrine\ORM\EntityManagerInterface:
         alias: doctrine.orm.entity_manager
 
+    Doctrine\Migrations\Metadata\Storage\TableMetadataStorageConfiguration:
+        alias: doctrine.migrations.storage.table_storage
+
     Doctrine\Persistence\ManagerRegistry:
         alias: doctrine
 
@@ -260,7 +263,6 @@ services:
     Wallabag\Command\InstallCommand:
         arguments:
             $databaseDriver: '%database_driver%'
-            $databaseName: '%database_name%'
             $defaultSettings: '%wallabag.default_internal_settings%'
             $defaultIgnoreOriginInstanceRules: '%wallabag.default_ignore_origin_instance_rules%'
 

app/config/tests/parameters_test.sqlite.yml

@@ -1,2 +1,2 @@
 parameters:
-    env(DATABASE_URL): sqlite:///%kernel.project_dir%/data/db/wallabag_test.sqlite?charset=utf8
+    env(DATABASE_URL): sqlite:///%kernel.project_dir%/data/db/wallabag%wallabag_dbname_suffix%.sqlite?charset=utf8

src/Command/InstallCommand.php

@@ -7,6 +7,7 @@ use Doctrine\DBAL\Exception\DriverException;
 use Doctrine\DBAL\Platforms\MySQLPlatform;
 use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
 use Doctrine\DBAL\Platforms\SqlitePlatform;
+use Doctrine\Migrations\Metadata\Storage\TableMetadataStorageConfiguration;
 use Doctrine\ORM\EntityManagerInterface;
 use FOS\UserBundle\Event\UserEvent;
 use FOS\UserBundle\FOSUserEvents;
@@ -35,34 +36,28 @@ class InstallCommand extends Command
         'curl_exec',
         'curl_multi_init',
     ];
-    private bool $runOtherCommands = true;
 
     private EntityManagerInterface $entityManager;
     private EventDispatcherInterface $dispatcher;
     private UserManagerInterface $userManager;
+    private TableMetadataStorageConfiguration $tableMetadataStorageConfiguration;
     private string $databaseDriver;
-    private string $databaseName;
     private array $defaultSettings;
     private array $defaultIgnoreOriginInstanceRules;
 
-    public function __construct(EntityManagerInterface $entityManager, EventDispatcherInterface $dispatcher, UserManagerInterface $userManager, string $databaseDriver, string $databaseName, array $defaultSettings, array $defaultIgnoreOriginInstanceRules)
+    public function __construct(EntityManagerInterface $entityManager, EventDispatcherInterface $dispatcher, UserManagerInterface $userManager, TableMetadataStorageConfiguration $tableMetadataStorageConfiguration, string $databaseDriver, array $defaultSettings, array $defaultIgnoreOriginInstanceRules)
     {
         $this->entityManager = $entityManager;
         $this->dispatcher = $dispatcher;
         $this->userManager = $userManager;
+        $this->tableMetadataStorageConfiguration = $tableMetadataStorageConfiguration;
         $this->databaseDriver = $databaseDriver;
-        $this->databaseName = $databaseName;
         $this->defaultSettings = $defaultSettings;
         $this->defaultIgnoreOriginInstanceRules = $defaultIgnoreOriginInstanceRules;
 
         parent::__construct();
     }
 
-    public function disableRunOtherCommands(): void
-    {
-        $this->runOtherCommands = false;
-    }
-
     protected function configure()
     {
         $this
@@ -127,7 +122,7 @@ class InstallCommand extends Command
             $conn->connect();
         } catch (\Exception $e) {
             if (!str_contains($e->getMessage(), 'Unknown database')
-                && !str_contains($e->getMessage(), 'database "' . $this->databaseName . '" does not exist')) {
+                && !str_contains($e->getMessage(), 'database "' . $conn->getDatabase() . '" does not exist')) {
                 $fulfilled = false;
                 $status = '<error>ERROR!</error>';
                 $help = 'Can\'t connect to the database: ' . $e->getMessage();
@@ -198,13 +193,21 @@ class InstallCommand extends Command
     {
         $this->io->section('Step 2 of 4: Setting up database.');
 
+        $conn = $this->entityManager->getConnection();
+        $databasePlatform = $conn->isConnected() ? $conn->getDatabasePlatform() : null;
+
         // user want to reset everything? Don't care about what is already here
         if (true === $this->defaultInput->getOption('reset')) {
             $this->io->text('Dropping database, creating database and schema, clearing the cache');
 
+            $this->runCommand('doctrine:schema:drop', ['--force' => true, '--full-database' => true]);
+
+            if (!$databasePlatform instanceof PostgreSQLPlatform) {
+                $this->runCommand('doctrine:database:drop', ['--force' => true]);
+                $this->runCommand('doctrine:database:create');
+            }
+
             $this
-                ->runCommand('doctrine:database:drop', ['--force' => true])
-                ->runCommand('doctrine:database:create')
                 ->runCommand('doctrine:migrations:migrate', ['--no-interaction' => true])
                 ->runCommand('cache:clear')
             ;
@@ -231,19 +234,20 @@ class InstallCommand extends Command
         if ($this->io->confirm('It appears that your database already exists. Would you like to reset it?', false)) {
             $this->io->text('Dropping database, creating database and schema...');
 
-            $this
-                ->runCommand('doctrine:database:drop', ['--force' => true])
-                ->runCommand('doctrine:database:create')
-                ->runCommand('doctrine:migrations:migrate', ['--no-interaction' => true])
-            ;
+            $this->runCommand('doctrine:schema:drop', ['--force' => true, '--full-database' => true]);
+
+            if (!$databasePlatform instanceof PostgreSQLPlatform) {
+                $this->runCommand('doctrine:database:drop', ['--force' => true]);
+                $this->runCommand('doctrine:database:create');
+            }
+
+            $this->runCommand('doctrine:migrations:migrate', ['--no-interaction' => true]);
         } elseif ($this->isSchemaPresent()) {
             if ($this->io->confirm('Seems like your database contains schema. Do you want to reset it?', false)) {
                 $this->io->text('Dropping schema and creating schema...');
 
-                $this
-                    ->runCommand('doctrine:schema:drop', ['--force' => true])
-                    ->runCommand('doctrine:migrations:migrate', ['--no-interaction' => true])
-                ;
+                $this->dropWallabagSchemaOnly();
+                $this->runCommand('doctrine:migrations:migrate', ['--no-interaction' => true]);
             }
         } else {
             $this->io->text('Creating schema...');
@@ -333,10 +337,6 @@ class InstallCommand extends Command
      */
     private function runCommand($command, $parameters = [])
     {
-        if (!$this->runOtherCommands) {
-            return $this;
-        }
-
         $parameters = array_merge(
             ['command' => $command],
             $parameters,
@@ -376,7 +376,13 @@ class InstallCommand extends Command
     private function isDatabasePresent()
     {
         $connection = $this->entityManager->getConnection();
-        $databaseName = $connection->getDatabase();
+        $params = $connection->getParams();
+
+        if ($connection->getDatabasePlatform() instanceof SqlitePlatform) {
+            $databaseName = $params['path'];
+        } else {
+            $databaseName = $params['dbname'];
+        }
 
         try {
             $schemaManager = $connection->createSchemaManager();
@@ -396,8 +402,6 @@ class InstallCommand extends Command
 
         // custom verification for sqlite, since `getListDatabasesSQL` doesn't work for sqlite
         if ($connection->getDatabasePlatform() instanceof SqlitePlatform) {
-            $params = $connection->getParams();
-
             if (isset($params['path']) && file_exists($params['path'])) {
                 return true;
             }
@@ -416,14 +420,21 @@ class InstallCommand extends Command
 
     /**
      * Check if the schema is already created.
-     * If we found at least one table, it means the schema exists.
-     *
-     * @return bool
+     * We use the Doctrine Migrations table for the check.
      */
-    private function isSchemaPresent()
+    private function isSchemaPresent(): bool
     {
         $schemaManager = $this->entityManager->getConnection()->createSchemaManager();
 
-        return \count($schemaManager->listTableNames()) > 0 ? true : false;
+        return $schemaManager->tablesExist([$this->tableMetadataStorageConfiguration->getTableName()]);
+    }
+
+    private function dropWallabagSchemaOnly(): void
+    {
+        $this->runCommand('doctrine:schema:drop', ['--force' => true]);
+
+        $connection = $this->entityManager->getConnection();
+        $databasePlatform = $connection->getDatabasePlatform();
+        $connection->executeQuery('DROP TABLE ' . $databasePlatform->quoteIdentifier($this->tableMetadataStorageConfiguration->getTableName()) . ';');
     }
 }

tests/Command/InstallCommandTest.php

@@ -8,6 +8,7 @@ use Doctrine\DBAL\Platforms\MySQLPlatform;
 use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
 use Doctrine\DBAL\Platforms\SqlitePlatform;
 use Doctrine\Persistence\ManagerRegistry;
+use GuzzleHttp\Psr7\Uri;
 use Symfony\Bundle\FrameworkBundle\Console\Application;
 use Symfony\Component\Console\Command\LazyCommand;
 use Symfony\Component\Console\Input\ArrayInput;
@@ -36,50 +37,57 @@ class InstallCommandTest extends WallabagTestCase
 
         /** @var Connection $connection */
         $connection = $this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection();
-        if ($connection->getDatabasePlatform() instanceof PostgreSQLPlatform) {
-            /*
-             * LOG:  statement: CREATE DATABASE "wallabag"
-             * ERROR:  source database "template1" is being accessed by other users
-             * DETAIL:  There is 1 other session using the database.
-             * STATEMENT:  CREATE DATABASE "wallabag"
-             * FATAL:  database "wallabag" does not exist
-             *
-             * http://stackoverflow.com/a/14374832/569101
-             */
-            $this->markTestSkipped('PostgreSQL spotted: can\'t find a good way to drop current database, skipping.');
-        }
+
+        $originalDatabaseUrl = $this->getTestClient()->getContainer()->getParameter('env(DATABASE_URL)');
+        $dbnameSuffix = $this->getTestClient()->getContainer()->getParameter('wallabag_dbname_suffix');
+        $tmpDatabaseName = 'wallabag_' . bin2hex(random_bytes(5));
 
         if ($connection->getDatabasePlatform() instanceof SqlitePlatform) {
-            // Environnement variable useful only for sqlite to avoid the error "attempt to write a readonly database"
-            // We can't define always this environnement variable because pdo_mysql seems to use it
-            // and we have the error:
-            // SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax;
-            // check the manual that corresponds to your MariaDB server version for the right syntax to use
-            // near '/tmp/wallabag_testTYj1kp' at line 1
-            $databasePath = tempnam(sys_get_temp_dir(), 'wallabag_test');
-            putenv("DATABASE_URL=sqlite:///$databasePath?charset=utf8");
-
-            // The environnement has been changed, recreate the client in order to update connection
-            parent::setUp();
+            $tmpDatabaseUrl = str_replace('wallabag' . $dbnameSuffix . '.sqlite', $tmpDatabaseName . $dbnameSuffix . '.sqlite', $originalDatabaseUrl);
+        } else {
+            $tmpDatabaseUrl = (string) (new Uri($originalDatabaseUrl))->withPath($tmpDatabaseName);
         }
 
-        $this->resetDatabase($this->getTestClient());
+        putenv("DATABASE_URL=$tmpDatabaseUrl");
+
+        if ($connection->getDatabasePlatform() instanceof PostgreSQLPlatform) {
+            // PostgreSQL requires that the database exists before connecting to it
+            $tmpTestDatabaseName = $tmpDatabaseName . $dbnameSuffix;
+            $connection->executeQuery('CREATE DATABASE ' . $tmpTestDatabaseName);
+        }
+
+        // The environnement has been changed, recreate the client in order to update connection
+        $this->getNewClient();
     }
 
     protected function tearDown(): void
     {
         $databaseUrl = getenv('DATABASE_URL');
-        $databasePath = parse_url($databaseUrl, \PHP_URL_PATH);
-        // Remove the real environnement variable
-        putenv('DATABASE_URL');
 
-        if ($databasePath && file_exists($databasePath)) {
-            unlink($databasePath);
+        /** @var Connection $connection */
+        $connection = $this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection();
+
+        if ($connection->getDatabasePlatform() instanceof SqlitePlatform) {
+            // Remove the real environnement variable
+            putenv('DATABASE_URL');
+
+            $databasePath = parse_url($databaseUrl, \PHP_URL_PATH);
+
+            if (file_exists($databasePath)) {
+                unlink($databasePath);
+            }
         } else {
+            $testDatabaseName = $connection->getDatabase();
+            $connection->close();
+
+            // Remove the real environnement variable
+            putenv('DATABASE_URL');
+
             // Create a new client to avoid the error:
             // Transaction commit failed because the transaction has been marked for rollback only.
-            $client = $this->getNewClient();
-            $this->resetDatabase($client);
+            $this->getNewClient();
+
+            $this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->executeQuery('DROP DATABASE ' . $testDatabaseName);
         }
 
         parent::tearDown();
@@ -87,12 +95,9 @@ class InstallCommandTest extends WallabagTestCase
 
     public function testRunInstallCommand()
     {
-        $command = $this->getCommand();
+        $this->setupDatabase();
 
-        // enable calling other commands for MySQL only because rollback isn't supported
-        if (!$this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->getDatabasePlatform() instanceof MySQLPlatform) {
-            $command->disableRunOtherCommands();
-        }
+        $command = $this->getCommand();
 
         $tester = new CommandTester($command);
         $tester->setInputs([
@@ -112,12 +117,9 @@ class InstallCommandTest extends WallabagTestCase
 
     public function testRunInstallCommandWithReset()
     {
-        if ($this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->getDatabasePlatform() instanceof MySQLPlatform) {
-            $this->markTestSkipped('Rollback are not properly handled for MySQL, skipping.');
-        }
+        $this->setupDatabase();
 
         $command = $this->getCommand();
-        $command->disableRunOtherCommands();
 
         $tester = new CommandTester($command);
         $tester->setInputs([
@@ -140,10 +142,10 @@ class InstallCommandTest extends WallabagTestCase
         $this->assertStringContainsString('Dropping database, creating database and schema, clearing the cache', $tester->getDisplay());
     }
 
-    public function testRunInstallCommandWithDatabaseRemoved()
+    public function testRunInstallCommandWithNonExistingDatabase()
     {
-        if ($this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->getDatabasePlatform() instanceof MySQLPlatform) {
-            $this->markTestSkipped('Rollback are not properly handled for MySQL, skipping.');
+        if ($this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->getDatabasePlatform() instanceof PostgreSQLPlatform) {
+            $this->markTestSkipped('PostgreSQL spotted: PostgreSQL requires that the database exists before connecting to it, skipping.');
         }
 
         // skipped SQLite check when database is removed because while testing for the connection,
@@ -154,15 +156,6 @@ class InstallCommandTest extends WallabagTestCase
 
         $application = new Application($this->getTestClient()->getKernel());
 
-        // drop database first, so the install command won't ask to reset things
-        $command = $application->find('doctrine:database:drop');
-        $command->run(new ArrayInput([
-            '--force' => true,
-        ]), new NullOutput());
-
-        // start a new application to avoid lagging connexion to pgsql
-        $this->getNewClient();
-
         $command = $this->getCommand();
 
         $tester = new CommandTester($command);
@@ -185,12 +178,9 @@ class InstallCommandTest extends WallabagTestCase
 
     public function testRunInstallCommandChooseResetSchema()
     {
-        if ($this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->getDatabasePlatform() instanceof MySQLPlatform) {
-            $this->markTestSkipped('Rollback are not properly handled for MySQL, skipping.');
-        }
+        $this->setupDatabase();
 
         $command = $this->getCommand();
-        $command->disableRunOtherCommands();
 
         $tester = new CommandTester($command);
         $tester->setInputs([
@@ -210,29 +200,8 @@ class InstallCommandTest extends WallabagTestCase
 
     public function testRunInstallCommandChooseNothing()
     {
-        /*
-         *  [PHPUnit\Framework\Error\Warning (2)]
-         *  filemtime(): stat failed for /home/runner/work/wallabag/wallabag/var/cache/tes_/ContainerNVNxA24/appAppKernelTestDebugContainer.php
-         *
-         * I don't know from where the "/tes_/" come from, it should be "/test/" instead ...
-         */
-        if ($this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->getDatabasePlatform() instanceof MySQLPlatform) {
-            $this->markTestSkipped('That test is failing when using MySQL when clearing the cache (see code comment)');
-        }
-
         $application = new Application($this->getTestClient()->getKernel());
 
-        // drop database first, so the install command won't ask to reset things
-        $command = $application->find('doctrine:database:drop');
-        $command->run(new ArrayInput([
-            '--force' => true,
-        ]), new NullOutput());
-
-        $this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->close();
-
-        $command = $application->find('doctrine:database:create');
-        $command->run(new ArrayInput([]), new NullOutput());
-
         $command = $this->getCommand();
 
         $tester = new CommandTester($command);
@@ -247,17 +216,24 @@ class InstallCommandTest extends WallabagTestCase
         $this->assertStringContainsString('Administration setup.', $tester->getDisplay());
         $this->assertStringContainsString('Config setup.', $tester->getDisplay());
 
-        $this->assertStringContainsString('Creating schema', $tester->getDisplay());
+        $databasePlatform = $this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->getDatabasePlatform();
+
+        if ($databasePlatform instanceof SqlitePlatform || $databasePlatform instanceof PostgreSQLPlatform) {
+            // SQLite and PostgreSQL always have the database created, so we create the schema only
+            $this->assertStringContainsString('Creating schema', $tester->getDisplay());
+        }
+
+        if ($databasePlatform instanceof MySQLPlatform) {
+            // MySQL can start with a non-existing database, so we create both the database and the schema
+            $this->assertStringContainsString('Creating database and schema', $tester->getDisplay());
+        }
     }
 
     public function testRunInstallCommandNoInteraction()
     {
-        if ($this->getTestClient()->getContainer()->get(ManagerRegistry::class)->getConnection()->getDatabasePlatform() instanceof MySQLPlatform) {
-            $this->markTestSkipped('Rollback are not properly handled for MySQL, skipping.');
-        }
+        $this->setupDatabase();
 
         $command = $this->getCommand();
-        $command->disableRunOtherCommands();
 
         $tester = new CommandTester($command);
         $tester->execute([], [
@@ -284,4 +260,36 @@ class InstallCommandTest extends WallabagTestCase
 
         return $command;
     }
+
+    private function setupDatabase()
+    {
+        $application = new Application($this->getTestClient()->getKernel());
+        $application->setAutoExit(false);
+
+        $application->run(new ArrayInput([
+            'command' => 'doctrine:database:create',
+            '--no-interaction' => true,
+            '--env' => 'test',
+        ]), new NullOutput());
+
+        $application->run(new ArrayInput([
+            'command' => 'doctrine:migrations:migrate',
+            '--no-interaction' => true,
+            '--env' => 'test',
+        ]), new NullOutput());
+
+        $application->run(new ArrayInput([
+            'command' => 'doctrine:fixtures:load',
+            '--no-interaction' => true,
+            '--env' => 'test',
+        ]), new NullOutput());
+
+        /*
+         * Recreate client to avoid error:
+         *
+         * [Doctrine\DBAL\ConnectionException]
+         * Transaction commit failed because the transaction has been marked for rollback only.
+         */
+        $this->getNewClient();
+    }
 }

tests/WallabagTestCase.php

@@ -3,11 +3,8 @@
 namespace Tests\Wallabag;
 
 use Doctrine\ORM\EntityManagerInterface;
-use Symfony\Bundle\FrameworkBundle\Console\Application;
 use Symfony\Bundle\FrameworkBundle\KernelBrowser;
 use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
-use Symfony\Component\Console\Input\ArrayInput;
-use Symfony\Component\Console\Output\NullOutput;
 use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
 use Wallabag\Entity\User;
 
@@ -39,40 +36,6 @@ abstract class WallabagTestCase extends WebTestCase
         return $this->client;
     }
 
-    public function resetDatabase(KernelBrowser $client)
-    {
-        $application = new Application($client->getKernel());
-        $application->setAutoExit(false);
-
-        $application->run(new ArrayInput([
-            'command' => 'doctrine:schema:drop',
-            '--no-interaction' => true,
-            '--force' => true,
-            '--full-database' => true,
-            '--env' => 'test',
-        ]), new NullOutput());
-
-        $application->run(new ArrayInput([
-            'command' => 'doctrine:migrations:migrate',
-            '--no-interaction' => true,
-            '--env' => 'test',
-        ]), new NullOutput());
-
-        $application->run(new ArrayInput([
-            'command' => 'doctrine:fixtures:load',
-            '--no-interaction' => true,
-            '--env' => 'test',
-        ]), new NullOutput());
-
-        /*
-         * Recreate client to avoid error:
-         *
-         * [Doctrine\DBAL\ConnectionException]
-         * Transaction commit failed because the transaction has been marked for rollback only.
-         */
-        $this->client = $this->getNewClient();
-    }
-
     public function getEntityManager()
     {
         return $this->client->getContainer()->get(EntityManagerInterface::class);