From 001a7bad66fd1ce2b208abfe89177e72b5e1baa5 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sat, 3 Sep 2016 09:24:34 +0200 Subject: [PATCH 1/3] Add a check for the database connection Checking for the driver isn't enough. We are now checking if we can etablish a connection to the database before trying to do anything. By displaying the error from the Exception (in case of error) we hope to reduce issues overload about people getting error with the database --- .../CoreBundle/Command/InstallCommand.php | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/Wallabag/CoreBundle/Command/InstallCommand.php b/src/Wallabag/CoreBundle/Command/InstallCommand.php index 813ac720d..50d0dfff4 100644 --- a/src/Wallabag/CoreBundle/Command/InstallCommand.php +++ b/src/Wallabag/CoreBundle/Command/InstallCommand.php @@ -72,8 +72,10 @@ class InstallCommand extends ContainerAwareCommand { $this->defaultOutput->writeln('Step 1 of 5. Checking system requirements.'); - $fulfilled = true; + $rows = []; + // testing if database driver exists + $fulfilled = true; $label = 'PDO Driver'; $status = 'OK!'; $help = ''; @@ -84,7 +86,21 @@ class InstallCommand extends ContainerAwareCommand $help = 'Database driver "'.$this->getContainer()->getParameter('database_driver').'" is not installed.'; } - $rows = []; + $rows[] = [$label, $status, $help]; + + // testing if connection to the database can be etablished + $label = 'Database connection'; + $status = 'OK!'; + $help = ''; + + try { + $this->getContainer()->get('doctrine')->getManager()->getConnection()->connect(); + } catch (\Exception $e) { + $fulfilled = false; + $status = 'ERROR!'; + $help = 'Can\'t connect to the database: '.$e->getMessage(); + } + $rows[] = [$label, $status, $help]; foreach ($this->functionExists as $functionRequired) { From f62c3faf88fbc1c2a484854019d17ff377836717 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sat, 3 Sep 2016 10:34:27 +0200 Subject: [PATCH 2/3] Update test If the database isn't found when checking for the connection it means, we can connect to the server. The InstallCommand will create the database later. Also, when checking for the SQLite connection, Doctrine creates the file (so the database). That's why the test is skipped for SQLite. --- src/Wallabag/CoreBundle/Command/InstallCommand.php | 10 ++++++---- .../Wallabag/CoreBundle/Command/InstallCommandTest.php | 6 ++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Wallabag/CoreBundle/Command/InstallCommand.php b/src/Wallabag/CoreBundle/Command/InstallCommand.php index 50d0dfff4..cd8161490 100644 --- a/src/Wallabag/CoreBundle/Command/InstallCommand.php +++ b/src/Wallabag/CoreBundle/Command/InstallCommand.php @@ -96,9 +96,11 @@ class InstallCommand extends ContainerAwareCommand try { $this->getContainer()->get('doctrine')->getManager()->getConnection()->connect(); } catch (\Exception $e) { - $fulfilled = false; - $status = 'ERROR!'; - $help = 'Can\'t connect to the database: '.$e->getMessage(); + if (false === strpos($e->getMessage(), "Unknown database")) { + $fulfilled = false; + $status = 'ERROR!'; + $help = 'Can\'t connect to the database: '.$e->getMessage(); + } } $rows[] = [$label, $status, $help]; @@ -472,7 +474,7 @@ class InstallCommand extends ContainerAwareCommand } // custom verification for sqlite, since `getListDatabasesSQL` doesn't work for sqlite - if ('sqlite' == $schemaManager->getDatabasePlatform()->getName()) { + if ('sqlite' === $schemaManager->getDatabasePlatform()->getName()) { $params = $this->getContainer()->get('doctrine.dbal.default_connection')->getParams(); if (isset($params['path']) && file_exists($params['path'])) { diff --git a/tests/Wallabag/CoreBundle/Command/InstallCommandTest.php b/tests/Wallabag/CoreBundle/Command/InstallCommandTest.php index 089a1c5fe..83f5bf246 100644 --- a/tests/Wallabag/CoreBundle/Command/InstallCommandTest.php +++ b/tests/Wallabag/CoreBundle/Command/InstallCommandTest.php @@ -127,6 +127,12 @@ class InstallCommandTest extends WallabagCoreTestCase public function testRunInstallCommandWithDatabaseRemoved() { + // skipped SQLite check when database is removed because while testing for the connection, + // the driver will create the file (so the database) before testing if database exist + if ($this->getClient()->getContainer()->get('doctrine')->getConnection()->getDriver() instanceof \Doctrine\DBAL\Driver\PDOSqlite\Driver) { + $this->markTestSkipped('SQLite spotted: can\'t test with database removed.'); + } + $application = new Application($this->getClient()->getKernel()); $application->add(new DropDatabaseDoctrineCommand()); From 5070644a12ad264444a812b783a04386d6581cb0 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sat, 3 Sep 2016 11:45:59 +0200 Subject: [PATCH 3/3] CS --- src/Wallabag/CoreBundle/Command/InstallCommand.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Wallabag/CoreBundle/Command/InstallCommand.php b/src/Wallabag/CoreBundle/Command/InstallCommand.php index cd8161490..035eb8655 100644 --- a/src/Wallabag/CoreBundle/Command/InstallCommand.php +++ b/src/Wallabag/CoreBundle/Command/InstallCommand.php @@ -96,7 +96,7 @@ class InstallCommand extends ContainerAwareCommand try { $this->getContainer()->get('doctrine')->getManager()->getConnection()->connect(); } catch (\Exception $e) { - if (false === strpos($e->getMessage(), "Unknown database")) { + if (false === strpos($e->getMessage(), 'Unknown database')) { $fulfilled = false; $status = 'ERROR!'; $help = 'Can\'t connect to the database: '.$e->getMessage();