API for SQL queries with PHP PDO

Question

I'm writing a very simple CRUD application, and I'm wondering if the way I'm using static methods throughout the code makes sense. I'd very much like to simplify the code if possible.

Any feedback and criticism is very much welcome!

<?php

class Database {

    private const DB_DSN = "127.0.0.1";
    private const DB_USER = "root";
    private const DB_PASSWORD = "root";
    private const DB_NAME = "testDB";
    private const DB_PORT = 8889;

    static function connect() {
        return new PDO(
            "mysql:host=".self::DB_DSN.";port=".self::DB_PORT.
            ";dbname=".self::DB_NAME,
            self::DB_USER,
            self::DB_PASSWORD,
            array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION),
        );
    }

    static function create() {
        $pdo = new PDO(
            "mysql:host=".self::DB_DSN.";port=".self::DB_PORT,
            self::DB_USER,
            self::DB_PASSWORD,
            array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION),
        );
        $pdo->exec("CREATE DATABASE IF NOT EXISTS ".self::DB_NAME);
        return self::connect();
    }
}

class User {

    private const TABLE_NAME = "users";

    public $uid;
    public $username;
    public $email;
    public $hash;
    public $created;
    public $verified;
    public $pdo;

    private function __construct() {
    }

    public static function signUp($pdo, $username, $email, $password) {

        if (self::exists($pdo, $username)) {
            return false;
        }

        $hash = password_hash($password, PASSWORD_DEFAULT);
        $stmt = $pdo->prepare("INSERT INTO `users` SET `username`=?, `email`=?, `hash`=?");
        $stmt->execute([$username, $email, $hash]);
        return self::fromUId($pdo, $pdo->lastInsertId());
    }

    public static function login($pdo, $username, $password) {

        if (!self::exists($pdo, $username)) {
            return false;
        }

        $stmt = $pdo->prepare("SELECT * FROM `users` WHERE `username` = ?");
        $stmt->execute([$username]);
        $user = $stmt->fetchObject("User");
        $user->pdo = $pdo;
        return password_verify($password, $user->hash) ? $user : false;
    }

    public static function exists($pdo, $username) {

        $stmt = $pdo->prepare("SELECT `uid` FROM `users` WHERE `username` = ?");
        $stmt->execute([$username]);
        return $stmt->fetch(PDO::FETCH_COLUMN);
    }

    public static function fromUId($pdo, $uid) {

        $stmt = $pdo->prepare("SELECT * FROM `users` WHERE `uid` = ?");
        $stmt->execute([$uid]);
        $user = $stmt->fetchObject("User");
        $user->pdo = $pdo;
        return $user;
    }

    public function verify() {

        $stmt = $this->pdo->prepare("UPDATE `users` SET `verified` = 1 WHERE `uid` = ?");
        if ($stmt->execute([$this->uid])) {
            $this->verified = true;
            return true;
        } else {
            return false;
        }
    }
}

$db = Database::create();

$db->exec(
    "CREATE TABLE IF NOT EXISTS `users` (
        `uid` int NOT NULL PRIMARY KEY AUTO_INCREMENT,
        `username` varchar(100) NOT NULL UNIQUE,
        `email` varchar(100) NOT NULL UNIQUE,
        `verified` boolean DEFAULT 0,
        `hash` varchar(255) NOT NULL,
        `created` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP
    )"
);

$db->exec(
    "CREATE TABLE IF NOT EXISTS `images` (
        `id` int NOT NULL PRIMARY KEY AUTO_INCREMENT,
        `uid` int NOT NULL,
        `image` varchar(255) NOT NULL,
        `like_count` int NOT NULL DEFAULT 0
    )"
);

$user = User::signUp($db, 'JohnDoe', '[email protected]', '12345');

?>

Answer 1

Main question

I'm wondering if the way I'm using static methods throughout the code makes sense.

It seems to be fine to me, though the need to pass the PDO object around seems excessive. A better approach might be to have a static member property or getter method to access that object when needed. Otherwise the singleton pattern could be used - have a static method to get an instance (e.g. of the Database object) when needed which can store a static property on the class.

Initially I thought the connections in the Database methods connect and create were redundant but they aren’t. One could possibly abstract the common parts there though.

As @YourCommonSense suggested in a stack overflow answer the connection could be reused in the ‘create()‘ method:

static function create() {
    $pdo = new PDO(
        "mysql:host=".self::DB_DSN.";port=".self::DB_PORT,
        self::DB_USER,
        self::DB_PASSWORD,
        array(PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION),
    );
    $pdo->exec("CREATE DATABASE IF NOT EXISTS ".self::DB_NAME);
    $pdo->query("use " . self::DB_NAME);
    return $pdo;

Other review points

General

The code makes good use of constants and setting the scope on them. Have you looked at the PHP Standards Recommendations? I'd suggest PSR-1 and PSR-12.

Suggestions

Method visibility

Per PSR-12 4.4

Visibility MUST be declared on all methods.

This is done for the methods in the User class but not the methods in the Database class.

returning early

Most methods do a good job of calling return when certain conditions are met in order to prevent other calls from happening and this keeps indentation levels at a minimum.

In the User method verify the last lines are as follows:

if ($stmt->execute([$this->uid])) {
   $this->verified = true;
   return true;
} else {
   return false;
}

In this case the else is not needed - the return false can be moved out one level:

if ($stmt->execute([$this->uid])) {
    $this->verified = true;
    return true;
}
return false;

This could also be written with reversed logic:

if (!$stmt->execute([$this->uid])) {
    return false;
}
$this->verified = true;
return true;

If those lines in the case where execute returned a truethy value were longer then the decreased indentation level would help readability.

Note the highest voted user contributed note on the documentation for pdostatement::execute

Hopefully this saves time for folks: one should use $count = $stmt->rowCount() after $stmt->execute() in order to really determine if any an operation such as ' update ' or ' replace ' did succeed i.e. changed some data. Jean-Lou Dupont.

Perhaps a better condition is $stmt->rowCount() after execute() is called.

Array syntax

There isn't anything wrong with using array() but as of the time of writing, PHP 7 has Active support for versions 7.4 and 7.3, and since PHP 5.4 arrays can be declared with short array syntax (PHP 5.4) - which is used in the static methods in the User class but not in the Database class methods.

Documentation

It is wise to add docblocks above each method - as you saw I had to ask in a comment about the verify method. With ample documentation others (as well as your future self) can get a summary of what a method does, what inputs and outputs it may have, etc.

Answer 2

On the Database class

In my experience, a language layer never creates a database. It would require a create database privilege for one thing. Although it's sort of OK for the local environment but would you like your live application to connect the database under the root account? I highly doubt so.

Hence I find the create() method superfluous. But just for sake of refactoring I would make the connect method to accept a parameter that would tell it to use or not to use the dbname DSN argument so it could connect with or without setting the database.

What I would rather add to the database class is a handy "prepare and execute in one go" method like:

public function query($sql, $data = []) {
    $stmt = $this->pdo->prepare($sql);
    $stmt->execute($data);
    return $stmt;
}

as it will make all methods of the User class to lose a lot of weight. As a general rule, when you see repetitive code, think of creating a function to do all the repetitions in one go.

Also it would make sense to make $pdo a public property of the database class.

Now to the User class

First of all, I think that all your doubts with static class is for naught. Yes, in theory it's simple to call User::something() from anywhere without the need to pass around the instance of the user class. But... you need to pass around the instance of PDO anyway! The same problem, different angle.

So you can make your code much cleaner and doubtless by making all methods dynamic, and putting the pdo (or rather a Database class) instance inside. Then you will have to pass around the instance of the user class instead of the instance of the PDO class.

Another repetition that can be spotted is two methods that select a user by username. That's nearly identical code! Always try to reuse the code that already exists in your class. Moreover, for some reason login method runs these two identical queries one after another. But why? Is once not enough?

Another problem is signup function. It's a no-go when it just silently returns false when a user exists. It should be vocal about that. The generic solution is to throw a custom built exception that can be caught and conveyed to the user.

On the return values: Most of time it's not needed. After all, what would you do with that return value from the verify() method?

On the names: it is Better to be more explicit with the function names. "verify" is ambiguous - you never can tell what does it verify and why. The same goes for "exists".

The ActiveRecord vs. DataMapper controversy

From the way you are asking, I can tell you already feel the ambiguity of your class, which serves for two purposes at once, being both a user and a User persistence layer. That's always so with the ActiveRecord pattern to which your class belongs even if you don't probably realize that. You even tried to mae a distinction between these two roles, where static methods are manipulating the database and dynamic methods and properties are manipulating the class data.

You need to make just a logical step further and separate these roles explicitly. A better approach to be considered is to split this class in two - User class and a UserMapper class, where the latter one would do all the database work.

the Username controversy

As long as there is an email to identify the user, there is absolutely no sense in using a dedicated username field. In practice, it adds helluvalot of confusion. A user signs up with a user name and n email, then forgets it and signs up again with email as a username and everything becomes so entangled that it makes it really hard for the support staff. Make it only email to identify the user and get rid of that username nonsense completely. In case you plan to use it as a display name then make it a display name that is not used in the login process at all.

The code

So according to the above suggestion I would create three classes. The User itself

<?php
class User {
    public $uid;
    public $username;
    public $email;
    public $hash;
    public $created;
    public $verified;

    public function login($password)
    {
        return password_verify($password, $this->hash);
    }

}

A class with a custom exception

class UserException Extends InvalidArgumentException {}

And a UserMapper class

class UserMapper
{
    private $db;
    private $table = 'users';
    private $class = 'User';

    public function __construct(Database $db) {
        $this->db = $db;
    }
    public function signUp($email, $password)
    {
        if ($this->getByEmail($email)) {
            throw new UserException("User already exists");
        }
        $hash = password_hash($password, PASSWORD_DEFAULT);
        $sql = "INSERT INTO `$this->table` SET `email`=?, `hash`=?";
        $this->db->query($sql, [$email, $hash]);
        return $this->getById($this->db->pdo->lastInsertId());
    }
    public function getByEmail($email)
    {
        $sql = "SELECT * FROM `$this->table` WHERE `email` = ?";
        return $this->db->query($sql, [$email])->fetchObject($this->class);
    }
    public function getById($uid)
    {
        $sql = "SELECT * FROM `$this->table` WHERE `uid` = ?";
        return $this->db->query($sql, [$uid])->fetchObject($this->class);
    }
    public function setVerified()
    {
        $sqk = "UPDATE `$this->table` SET `verified` = 1 WHERE `uid` = ?";
        $this->db->query($sql, [$this->uid]);
    }
}

then it can be used like this

$userMapper = new UserMapper($db);
try {
    $user = UserMapper->signUp('[email protected]', '12345');
} catch (UserException $e) {
    $error = $e->getMessage;
    // show it back to user, etc
}

$user = $userMapper->getByEmail($_POST['email']);
if ($user && $user->login($_POST['password'])) {
     // authorize
} else {
     // invalid login or password
}