3
\$\begingroup\$

I am reviewing the database class for a live website. I am not sure if the current connect function is the best way to create a pdo object as it appears to create a new connection every time it is called. I have read so many different opinions and examples I'm not sure which approach to take.

This quote from https://phpdelusions.net/pdo prompted me to seek codereview's advice.

The connection has to be made only once! No connects in every function. No connects in every class constructor. Otherwise, multiple connections will be created, which will eventually kill your database server. Thus, a sole PDO instance has to be created and then used through whole script execution.

Every function in the Database class creates a new connection with $db = self::connect(). So far there has not been any issues with this approach but I am worried about what will happen when the site begins to get more users.

class Database {

    private static function connect() {
        if ($_SESSION['base'] === "https://example.com/" || !file_exists(__DIR__ . '../../DEVMACHINE')) {
            $key = 'live';
        } else {
            $key = 'local';
        }
        $database = include(realpath(dirname(__FILE__)) . '/../../config/config.php');
        $dsn = $database[$key]['dsn'];
        $username = $database[$key]['username'];
        $password = $database[$key]['password'];
        $error_log_path = realpath(dirname(__FILE__)) . '/../../error.log';
        try {
            $db = new PDO($dsn, $username, $password, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);
             return $db;
        } catch (PDOException $e) {
            error_log(date('l, F d, Y') . " Caught" . $e->getMessage() . $e->__toString() . PHP_EOL, 3, $error_log_path);
            die('oops!');
        }
    }

    public static function delete($id) {
        $db = self::connect();
//            ^^^^^^^^^^^^^^^^
        $query = "DELETE FROM `table` WHERE `id` = :id";
        try {
            $statement = $db->prepare($query);
            $statement->bindValue(':id', $id);
            $success = $statement->execute();
            $deleted = $statement->rowCount();
            $statement->closeCursor();
            if (!$success) {
                throw new Exception("Delete Error");
            }
        } catch (Exception $e) {
            error_log(date('l, F d, Y') . "delete() DID NOT DELETE!" . $e->getMessage() . $e->__toString(), 3, $GLOBALS['error_log_path']);
        }
    }

    public static function foo() {
        $db = self::connect();
//            ^^^^^^^^^^^^^^^^

        // Use $db variable in another SQL statement
    }        

}

I tested the following code based on an example from https://phpdelusions.net/pdo/pdo_wrapper#dependency_injection it seems to work correctly and uses the same pdo instance instead of creating a new one. The code below is the change. Note that only a single connection is made, and self::$db is only initialized once, and then reused on subsequent calls to Database::connect().

class Database {
    private static $db;

    private static function connect() {
        try {
            if (self::$db === null) {
                self::$db = new PDO($dsn, $username, $password, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);
            }
            return self::$db;
\$\endgroup\$
12
  • \$\begingroup\$ Welcome to Code Review! Given the statement: "I am reviewing the database class for a live website" does that mean that you did not write/do not maintain the code? \$\endgroup\$ Mar 28, 2022 at 18:36
  • \$\begingroup\$ No it does not mean that. \$\endgroup\$
    – jdf
    Mar 28, 2022 at 18:41
  • \$\begingroup\$ okay thanks - we just want to make sure because For moral, practical, and legal reasons, we are only able to review code that you wrote or code that you maintain. \$\endgroup\$ Mar 28, 2022 at 18:49
  • \$\begingroup\$ It has been a while since I've written much PHP. I know PHP supports threads now. Beware that this code is not thread-safe if you are explicitly creating threads in PHP. My gut feeling though, is that this change should be fine for most web applications, since they tend to be single-threaded. \$\endgroup\$ Mar 28, 2022 at 19:42
  • \$\begingroup\$ Speaking as a developer who lives in legacy code: It should be noted that this does have potential to change behavior in the application. Things like temporary tables and session variables live in the connection. If the code isn't careful to cleanup after itself, this change could be a source of difficult to diagnose bugs. \$\endgroup\$
    – jpheldson
    Apr 4, 2022 at 20:55

1 Answer 1

3
\$\begingroup\$

Since refactoring the connect() method to use a static variable, you have essentially created a singleton. In most use cases, a singleton is an anti-pattern. If you can ensure your PHP application is only run in a single thread, this should be fine. Static variables are not thread-safe, so just be aware of that in the future.

Overall, refactoring the connect() method to reuse a PDO instance is largely transparent to the rest of the application. All methods in the Database class are static already. I admit I lack recent knowledge about whether or not PHP applications are single or multi-threaded. Unless something major has changed in the last 15 years, most web applications or scripts are only ever executed in a single thread. Only when you use the Thread class directly should you run into problems.

Error handling seems to be the bigger problem.

  • Avoid try-catch-log-die: I am always suspicious of code that looks like try { ... } catch (ex) { log(ex); die("message"); }. If all you do is log the exception, don't even bother catching the exception. Let the exception crash the application. Instead, catch unhandled exceptions further up the callstack so you can return a general "Oops! Something went wrong" page to end users. While rendering the page, log the exception. This centralizes logging logic for use cases where you cannot handle the exception. Do not catch exceptions you cannot handle.

    Logging an exception is not handling it.

  • Avoid Swallowing Exceptions: This is, by far, the biggest issue in the code. The delete method catches all exceptions, logs them, and then proceeds as if no error happened. Callers will not know whether or not the method executed with problems. Again, the advice here is the same as the last bullet point: let the exception crash the application. Catch the exception higher up in the call stack, log it, then show a general "error page" to end users.

\$\endgroup\$
2

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.