[Core] Remove Database::singleton function (#8427)

There are currently 3 methods of getting a reference to a database in LORIS, in order of preference: 1. LorisInstance->getDatabaseConnection() 2. NDB_Factory::singleton()->database(); 3. Database::singleton() The only reference to 3 left is in the implementation of option 2. This removes the Database::singleton by inlineing the reference into the factory and removes all remaining references to Database::singleton (mostly from documentation and a couple unit tests.)
aces · Mar 8, 2023 · 6b80e4f · 6b80e4f
1 parent 2f21693
commit 6b80e4f
Show file tree

Hide file tree

Showing 10 changed files with 69 additions and 126 deletions.
diff --git a/docs/CodingStandards.md b/docs/CodingStandards.md
@@ -32,16 +32,16 @@ Prepared statements MUST be used for any database interactions that use user inp
 
 LORIS has many classes that use a Singleton design pattern. To facilitate with
 unit testing, it is best to use these singletons via the NDB_Factory class.
-For example, you should use the Database class like this:
+For example, you should use the Project class like this:
 
 ```php
-$database = \NDB_Factory::singleton()->database();
+$project = \NDB_Factory::singleton()->project("controlproject");
 ```
 
 instead of 
 
 ```php
-$database = \Database::singleton();
+$project = \Project::singleton("controlproject");
 ```
 
 # HTML

diff --git a/docs/instruments/NDB_BVL_Instrument_UPLOADER_TEMPLATE.class.inc b/docs/instruments/NDB_BVL_Instrument_UPLOADER_TEMPLATE.class.inc
@@ -189,7 +189,7 @@ class NDB_BVL_Instrument_TEST_NAME extends NDB_BVL_Instrument
    function importFile(&$file, $args) {
         $fp=fopen($file->fileInfo['tmp_name'], "r");
 
-        $db=& Database::singleton();
+        $db= $this->loris->getDatabaseConnection();
         ///Setting trackchanges to false because getting error messages
         $db->_trackChanges = false;
         ////////////////////////////////////////////////////////////////

diff --git a/docs/wiki/99_Developers/UnitTestGuide.md b/docs/wiki/99_Developers/UnitTestGuide.md
@@ -395,7 +395,6 @@ This can be used for almost any class within LORIS. In the section right below,
 
 
 ```
-    $DB      = \Database::singleton();
     $config  = \NDB_Config::singleton();
     $user    = \User::singleton();
 ```
@@ -406,7 +405,6 @@ Please update the code to use this LORIS standard declaration:
 
 ```
     $factory = NDB_Factory::singleton();
-    $DB      = $factory->database();
     $config  = $factory->config();
     $user    = $factory->user();
 ```
@@ -526,20 +524,11 @@ protected function setUp(): void \
     $this->_factory->setTesting(false);
     
     $this->_configMock = $this->_factory->Config(CONFIG_XML);
-    $database          = $this->_configMock->getSetting('database');
-	
-    $this->_dbMock     = \Database::singleton(
-        $database['database'],
-        $database['username'],
-        $database['password'],
-        $database['host'],
-        true
-    );
-
+    $this->_dbMock     = $this->_factory->database();
 } 
 
 ```
-As you can see, the `$this->_dbMock` object is now declared as a `\Database::singleton()` rather than as a mock object. This is why it is not a pure unit test and why we cannot use the same “expects” method as in the previous section. 
+As you can see, the `$this->_dbMock` object is now declared as a Database object rather than as a mock object. This is why it is not a pure unit test and why we cannot use the same “expects” method as in the previous section. 
 
 
 The **tearDown** function is the same as before:

diff --git a/php/libraries/Database.class.inc b/php/libraries/Database.class.inc
@@ -18,9 +18,12 @@ class Database implements LoggerAwareInterface
     use \Psr\Log\LoggerAwareTrait;
 
     /**
-     * The database handle, created by the connect() method
+     * The database handle, created by the connect() method.
+     * This *MUST* be private to ensure close deletes the
+     * last (only) reference to it, which is how connections
+     * are closed in PDO
      *
-     * @var    PDO
+     * @var    ?PDO
      * @access private
      */
     var $_PDO = null;
@@ -56,7 +59,8 @@ class Database implements LoggerAwareInterface
 
     /**
      * Constructor must be public for unit tests to pass, but this should not
-     * be used directly. Should only be called by singleton method.
+     * be used directly. References should only be retrieved via the LorisInstance
+     * object.
      */
     public function __construct()
     {
@@ -65,6 +69,7 @@ class Database implements LoggerAwareInterface
         // NDB_Factory will set an appropriate ApacheLogger after the fact.
         $this->setLogger(new \Psr\Log\NullLogger);
     }
+
     /**
      * Singleton design pattern implementation - creates the object
      * and also connects to the database if necessary.
@@ -81,53 +86,23 @@ class Database implements LoggerAwareInterface
      * @param bool   $trackChanges boolean determining if changes should be
      *                             logged to the history table
      *
-     * @return \Database
-     * @throws \DatabaseException
-     * @access public
+     * @return     \Database
+     * @throws     \DatabaseException
+     * @access     public
+     * @deprecated
      */
-    static function &singleton(
+    static function singleton(
         string $database,
         bool $trackChanges = true
     ): \Database {
-        $username = getenv("LORIS_{$database}_USERNAME");
-        $password = getenv("LORIS_{$database}_PASSWORD");
-        $host     = getenv("LORIS_{$database}_HOST");
-
-        static $connections = [];
-
-        // if no arguments, try to get the first connection or choke
-        if (empty($database)
-            && empty($username)
-            && empty($password)
-            && empty($host)
-        ) {
-            if (!empty($connections)) {
-                reset($connections);
-                $connection = current($connections);
-                return $connection;
-            } else {
-                throw new DatabaseException("No database connection exists");
-            }
-        } else {
-            // name the connection.
-            if ($trackChanges) {
-                $connection = md5($database.$host.$username.$password.'yep');
-            } else {
-                $connection = md5($database.$host.$username.$password.'nope');
-            }
-
-            // create the connection if none exists
-            if (empty($connections[$connection])) {
-                $connections[$connection] = new Database();
-                $connections[$connection]->connect(
-                    $database,
-                    $trackChanges
-                );
-            }
-
-            // return the connection
-            return $connections[$connection];
-        }
+        // We don't have access to $this->logger since it's a static function,
+        // so we need to use error_log() instead of $this->logger->warning().
+        error_log(
+            "Database::singleton is deprecated. "
+            . "Use the getDatabaseConnection method from your LorisInstance"
+            . " object instead."
+        );
+        return \NDB_Factory::singleton()->database();
     }
 
     /**
@@ -1616,4 +1591,18 @@ class Database implements LoggerAwareInterface
     {
         return $this->_getServerVariable('version_comment');
     }
+
+    /**
+     * Close the database connection.
+     *
+     * @return void
+     */
+    public function closeConnection(): void
+    {
+        $this->_preparedStoreHistory = null;
+
+        $this->_PDO        = null;
+        $this->_HistoryPDO = null;
+
+    }
 }
diff --git a/php/libraries/NDB_Client.class.inc b/php/libraries/NDB_Client.class.inc
@@ -53,9 +53,6 @@ class NDB_Client
 
         $config = $factory->config($configFile);
 
-        // The factory uses the Settings object to call Database::singleton().
-        // This is required so that further calls to Database::singleton()
-        // will work.
         $DB = $factory->database();
 
         // Register S3 stream wrapper if configured

diff --git a/php/libraries/NDB_Factory.class.inc b/php/libraries/NDB_Factory.class.inc
@@ -173,9 +173,10 @@ class NDB_Factory
         putenv("LORIS_{$dbname}_PASSWORD=" . $settings->dbPassword());
         putenv("LORIS_{$dbname}_HOST=" . $settings->dbHost());
 
-        $db_ref = \Database::singleton(
+        $db_ref = new Database();
+        $db_ref->connect(
             $settings->dbName(),
-            true
+            true,
         );
 
         // Unset the variables now that they're no longer needed.

diff --git a/test/unittests/Database_Test.php b/test/unittests/Database_Test.php
@@ -92,17 +92,7 @@ protected function setUp(): void
         $this->factory = NDB_Factory::singleton();
         $this->factory->reset();
         $this->config = $this->factory->Config(CONFIG_XML);
-        $database     = $this->config->getSetting('database');
-        $this->DB     = $this->factory->database(
-            $database['database'],
-            $database['username'],
-            $database['password'],
-            $database['host'],
-            true,
-        );
-
-        $this->factory->setDatabase($this->DB);
-        $this->factory->setConfig($this->config);
+        $this->DB     = $this->factory->database();
     }
 
     /**

diff --git a/test/unittests/Loris_PHPUnit_Database_TestCase.php b/test/unittests/Loris_PHPUnit_Database_TestCase.php
@@ -82,22 +82,6 @@ protected function setUp(): void
      */
     protected function createLorisDBConnection()
     {
-        $dbname = $this->factory->settings()->dbName();
-        putenv(
-            "LORIS_{$dbname}_USERNAME="
-            . $this->factory->settings()->dbUserName()
-        );
-        putenv(
-            "LORIS_{$dbname}_PASSWORD="
-            . $this->factory->settings()->dbPassword()
-        );
-        putenv(
-            "LORIS_{$dbname}_HOST="
-            . $this->factory->settings()->dbHost()
-        );
-        $this->database = Database::singleton(
-            $this->factory->settings()->dbName(),
-        );
+        $this->database = $this->factory->database();
     }
-
 }
diff --git a/test/unittests/UserTest.php b/test/unittests/UserTest.php
@@ -260,17 +260,7 @@ class UserTest extends TestCase
      * @var \Database | PHPUnit\Framework\MockObject\MockObject
      */
     private $_mockDB;
-    /**
-     * Test double for Database object for hasLoggedIn method
-     *
-     * @note This is needed for User::hasLoggedIn because it declares and uses
-     *       the database differently than other methods in the User class.
-     *       This can be changed when the rest of the User class updates how it
-     *       declares its database. - Alexandra Livadas
-     *
-     * @var NDB_Factory
-     */
-    private $_mockFactory;
+
     /**
      * Maps config names to values
      * Used to set behaviour of NDB_Config test double
@@ -289,27 +279,16 @@ protected function setUp(): void
         $this->_factory = \NDB_Factory::singleton();
         $this->_factory->reset();
         $this->_configMock = $this->_factory->Config(CONFIG_XML);
-        $database          = $this->_configMock->getSetting('database');
-        $this->_dbMock     = $this->_factory->database(
-            $database['database'],
-            $database['username'],
-            $database['password'],
-            $database['host'],
-            true
-        );
+        $this->_dbMock     = $this->_factory->database();
 
         $mockconfig = $this->getMockBuilder('NDB_Config')->getMock();
         $mockdb     = $this->getMockBuilder('Database')->getMock();
 
         '@phan-var \Database $mockdb';
         '@phan-var \NDB_Config $mockconfig';
 
-        $this->_mockDB      = $mockdb;
-        $this->_mockConfig  = $mockconfig;
-        $this->_mockFactory = \NDB_Factory::singleton();
-        $this->_mockFactory->setDatabase($this->_dbMock);
-
-        $this->_factory->setConfig($this->_mockConfig);
+        $this->_mockDB     = $mockdb;
+        $this->_mockConfig = $mockconfig;
 
         $this->_userInfoComplete       = $this->_userInfo;
         $this->_userInfoComplete['ID'] = '1';
@@ -332,6 +311,7 @@ protected function setUp(): void
         $this->_userInfo['Password_hash']         = $passwordHash;
         $this->_userInfoComplete['Password_hash'] = $passwordHash;
 
+        $this->_setUpTestDoublesForFactoryUser();
         $this->_user = \User::factory(self::USERNAME);
     }
 
@@ -344,6 +324,7 @@ protected function setUp(): void
     protected function tearDown(): void
     {
         parent::tearDown();
+        $this->_factory->database()->closeConnection();
         $this->_factory->reset();
     }
 
@@ -359,7 +340,6 @@ protected function tearDown(): void
      */
     public function testFactoryRetrievesUserInfo()
     {
-        $this->_setUpTestDoublesForFactoryUser();
         $this->_user = \User::factory(self::USERNAME);
         //validate _user Info
         $this->assertEquals($this->_userInfoComplete, $this->_user->getData());
@@ -642,6 +622,7 @@ public function testInsert()
         $newUserInfo           = $this->_userInfo;
         $newUserInfo['ID']     = 2;
         $newUserInfo['UserID'] = '968776';
+        $newUserInfo['Email']  = 'notjohn.doe@mcgill.ca';
         \User::insert($newUserInfo);
         $this->_otherUser = \User::factory('968776');
         $this->assertEquals('968776', $this->_otherUser->getUsername());
@@ -655,6 +636,14 @@ public function testInsert()
      */
     public function testUpdate()
     {
+        // Insert the user so that it can be updated.
+        $newUserInfo           = $this->_userInfo;
+        $newUserInfo['ID']     = 2;
+        $newUserInfo['UserID'] = '968776';
+        $newUserInfo['Email']  = 'notjohn.doe@mcgill.ca';
+        \User::insert($newUserInfo);
+
+        // Test the update.
         $this->_otherUser = \User::factory('968776');
         $newInfo          = ['ID' => '3'];
         $this->_otherUser->update($newInfo);
@@ -678,6 +667,9 @@ public function testUpdatePasswordWithExpiration()
 
         // Cause usePwnedPasswordsAPI config option to return false.
         $mockConfig = &$this->_mockConfig;
+
+        $this->_factory->setConfig($mockConfig);
+
         '@phan-var \PHPUnit\Framework\MockObject\MockObject $mockConfig';
         $mockConfig->expects($this->any())
             ->method('settingEnabled')
@@ -714,6 +706,9 @@ public function testUpdatePasswordWithoutExpiry()
             ->method('settingEnabled')
             ->willReturn(false);
 
+        $mockConfig = &$this->_mockConfig;
+        $this->_factory->setConfig($mockConfig);
+
         $this->_user->updatePassword(
             new \Password(\Utility::randomString(16))
         );

diff --git a/test/unittests/VisitTest.php b/test/unittests/VisitTest.php
@@ -61,10 +61,8 @@ protected function setUp(): void
         putenv("LORIS_{$database['database']}_PASSWORD={$database['password']}");
         putenv("LORIS_{$database['database']}_HOST={$database['host']}");
 
-        $this->DB = \Database::singleton(
-            $database['database'],
-            true,
-        );
+        $this->DB = $this->factory->database();
+
         $this->visitController = new \Loris\VisitController($this->DB);
 
         $v1 = new \Loris\Visit('V1');