From a5d0657d9b7a7d975082d2d65a20918c1ca5b108 Mon Sep 17 00:00:00 2001
From: marc tobias <mtmail@gmx.net>
Date: Sun, 26 Apr 2020 03:33:15 +0200
Subject: [PATCH] lonvia PR feedback

---
 .travis.yml                   |  2 +-
 lib/DB.php                    | 35 +++++++++++------------------------
 test/README.md                |  4 ++--
 test/php/Nominatim/DBTest.php | 14 +++++++++++---
 4 files changed, 25 insertions(+), 30 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index dd973aec..f5344742 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -25,7 +25,7 @@ script:
   - cd $TRAVIS_BUILD_DIR/
   - if [[ $TEST_SUITE == "tests" ]]; then phpcs --report-width=120 . ; fi
   - cd $TRAVIS_BUILD_DIR/test/php
-  - if [[ $TEST_SUITE == "tests" ]]; then UNIT_TEST_DSN='pgsql:dbname=nominatim_unit_tests' /usr/bin/phpunit ./ ; fi
+  - if [[ $TEST_SUITE == "tests" ]]; then /usr/bin/phpunit ./ ; fi
   - cd $TRAVIS_BUILD_DIR/test/bdd
   - # behave --format=progress3 api
   - if [[ $TEST_SUITE == "tests" ]]; then behave -DREMOVE_TEMPLATE=1 --format=progress3 db ; fi
diff --git a/lib/DB.php b/lib/DB.php
index 5915b2e8..38b3e27e 100644
--- a/lib/DB.php
+++ b/lib/DB.php
@@ -251,7 +251,7 @@ class DB
     }
 
     /**
-     * Deletes a table. Returns true on success. Returns true if the table didn't exist.
+     * Deletes a table. Returns true if deleted or didn't exist.
      *
      * @param string  $sTableName
      *
@@ -405,29 +405,16 @@ END;
      */
     public static function generateDSN($aInfo)
     {
-        $sDSN = 'pgsql:';
-        if (isset($aInfo['host'])) {
-            $sDSN .= 'host=' . $aInfo['host'] . ';';
-        } elseif (isset($aInfo['hostspec'])) {
-            $sDSN .= 'host=' . $aInfo['hostspec'] . ';';
-        }
-        if (isset($aInfo['port'])) {
-            $sDSN .= 'port=' . $aInfo['port'] . ';';
-        }
-        if (isset($aInfo['dbname'])) {
-            $sDSN .= 'dbname=' . $aInfo['dbname'] . ';';
-        } elseif (isset($aInfo['database'])) {
-            $sDSN .= 'dbname=' . $aInfo['database'] . ';';
-        }
-        if (isset($aInfo['user'])) {
-            $sDSN .= 'user=' . $aInfo['user'] . ';';
-        } elseif (isset($aInfo['username'])) {
-            $sDSN .= 'user=' . $aInfo['username'] . ';';
-        }
-        if (isset($aInfo['password'])) {
-            $sDSN .= 'password=' . $aInfo['password'] . ';';
-        }
-        $sDSN = preg_replace('/;$/', '', $sDSN);
+        $sDSN = sprintf(
+            'pgsql:host=%s;port=%s;dbname=%s;user=%s;password=%s;',
+            $aInfo['host'] ?? $aInfo['hostspec'] ?? '',
+            $aInfo['port'] ?? '',
+            $aInfo['dbname'] ?? $aInfo['database'] ?? '',
+            $aInfo['user'] ?? '',
+            $aInfo['password'] ?? ''
+        );
+        $sDSN = preg_replace('/\b\w+=;/', '', $sDSN);
+        $sDSN = preg_replace('/;\Z/', '', $sDSN);
 
         return $sDSN;
     }
diff --git a/test/README.md b/test/README.md
index 5de08759..1f7a33ca 100644
--- a/test/README.md
+++ b/test/README.md
@@ -51,8 +51,8 @@ To execute the test suite run
 It will read phpunit.xml which points to the library, test path, bootstrap
 strip and set other parameters.
 
-The database set by `UNIT_TEST_DSN` will be deleted and recreated. Not setting
-it will skip some tests as pending, but not fail the tests.
+It will use (and destroy) a local database 'nominatim_unit_tests'. You can set
+a different connection string with e.g. UNIT_TEST_DSN='pgsql:dbname=foo_unit_tests'.
 
 BDD Functional Tests
 ====================
diff --git a/test/php/Nominatim/DBTest.php b/test/php/Nominatim/DBTest.php
index e4791975..1991f6f1 100644
--- a/test/php/Nominatim/DBTest.php
+++ b/test/php/Nominatim/DBTest.php
@@ -128,11 +128,19 @@ class DBTest extends \PHPUnit\Framework\TestCase
 
     public function testAgainstDatabase()
     {
-        if (getenv('UNIT_TEST_DSN') == false) $this->markTestSkipped('UNIT_TEST_DSN not set');
+        $unit_test_dsn = getenv('UNIT_TEST_DSN') != false ?
+                            getenv('UNIT_TEST_DSN') :
+                            'pgsql:dbname=nominatim_unit_tests';
+
+        $this->assertRegExp(
+            '/unit_test/',
+            $unit_test_dsn,
+            'Test database will get destroyed, thus should have a name like unit_test to be safe'
+        );
 
         ## Create the database.
         {
-            $aDSNParsed = \Nominatim\DB::parseDSN(getenv('UNIT_TEST_DSN'));
+            $aDSNParsed = \Nominatim\DB::parseDSN($unit_test_dsn);
             $sDbname = $aDSNParsed['database'];
             $aDSNParsed['database'] = 'postgres';
 
@@ -142,7 +150,7 @@ class DBTest extends \PHPUnit\Framework\TestCase
             $oDB->exec('CREATE DATABASE ' . $sDbname);
         }
 
-        $oDB = new \Nominatim\DB(getenv('UNIT_TEST_DSN'));
+        $oDB = new \Nominatim\DB($unit_test_dsn);
         $oDB->connect();
 
         $this->assertTrue(
-- 
2.39.5