User Class and MySQL Connection Handler

Question

Mainly used Stack Overflow, so first time over here for a code review. I am trying to make some enhancements to a legacy application. I have some minor experience using Laravel, but this application is using quite a heavy procedural approach, and I am struggling to work out the best way to structure thing in an OO sense. I have tried to adopt a repository pattern just to separate tasks between the separate entities, and I would be keen to get some feedback.

Firstly I have my basic User class model with its constructor and getter methods.

<?php

class User {

    // Properties
    private $id;
    private $username;
    private $password;
    private $email_address;
    private $first_name;
    private $last_name;
    private $group_id;

    function __construct($username, $password, $email_address, $first_name, $last_name, $group_id, $id = 0) {
        $this->id = $id;
        $this->username = $username;
        $this->password = $password;
        $this->email_address = $email_address;
        $this->first_name = $first_name;
        $this->last_name = $last_name;
        $this->group_id = $group_id;
    }

    // Methods
    public function getID(){
        return $this->id;
    }

    public function getUsername(){
        return $this->username;
    }

    function getPassword(){
        return $this->password;
    }

    function getEmailAddress(){
        return $this->email_address;
    }

    function getFirstName(){
        return $this->first_name;
    }

    function getLastName(){
        return $this->last_name;
    }

    function getGroupID(){
        return $this->group_id;
    }
}

Then I have my MySQL connector, I am a bit unsure about this due to the existence of MySQLi, it just feels like I am duplicating everything inside it. Though at least its allowing me to only specify the connection in one place.

<?php

include('config.php');

class MySQLConnector {

    private $dbUser = DBUSER;
    private $dbPass = DBPWD;
    private $dbName = DBNAME;
    private $dbHost = DBHOST;
    private $link;

    function __construct() {
        $this->link = new mysqli($this->dbHost, $this->dbUser, $this->dbPass, $this->dbName);
    }

    function __destruct() {
        $this->close();
    }

    function close() {
        $this->link->close();
    }

    public function query($query) {
        return $this->link->query($query);
    }

    // Get the data return int result

    function escapeString($query) {
        return $this->link->escape_string($query);
    }

    function prepare($query) {
        return $this->link->prepare($query);
    }

    function numRows($result) {
        return $result->num_rows;
    }

    // Gets array of query results

    function fetchAssoc($result) {
        return $result->fetch_assoc();
    }

    // Fetches all result rows as an associative array, a numeric array, or both

    function fetchArray($result, $resultType = MYSQLI_ASSOC) {
        return $result->fetch_array($resultType);
    }

    // Get a result row as an enumerated array

    function fetchAll($result, $resultType = MYSQLI_BOTH) {
        return $result->fetch_all($resultType);
    }

    // Free all MySQL result memory

    function fetchRow($result) {
        return $result->fetch_row();
    }

}

I think defined an interface called Repository that certain ones will follow.

<?php

interface Repository {

    public function getById($id);

    public function getAll();

    public function insert(Object $object);

    public function update(Object $object);

    public function delete($id);

}

Then lastly I have the UserRepository class, which is acting like a middleman between the User and MySQLConnector classes. I am not sure if I have taken the correct approach here, as it feels like I am duplicating code in places such as the insert and update statements, but I can't think of a way to optimise it further.

<?php

require_once('Repository.php');
require_once('MySQLConnector.php');

class UserRepository implements Repository {

    private $table = 'user_list';
    private $db_obj;

    public function __construct() {
        $this->db_obj = new MySQLConnector();
    }

    public function getById($id) {
        $query = 'select * from ' . $this->table . ' where id = '.$id;
        $result = $this->db_obj->query($query);
        return $this->db_obj->fetchAssoc($result);
    }

    public function getAll() {
        $query = 'select * from ' . $this->table;
        $result = $this->db_obj->query($query);
        return $this->db_obj->fetchAll($result);
    }

    public function insert($object) {

        $username = $this->db_obj->escapeString($object->getUsername());
        $firstName = $this->db_obj->escapeString($object->getFirstName());
        $lastName = $this->db_obj->escapeString($object->getLastName());
        $email = $this->db_obj->escapeString($object->getEmailAddress());
        $groupID = $this->db_obj->escapeString($object->getGroupID());
        $password = password_hash($this->db_obj->escapeString($object->getPassword()), PASSWORD_DEFAULT);

        $query = "insert into " . $this->table . " (username, first_name, last_name, email_address, password, group_id) values (? , ?  , ? , ? , ?, ? )";
        $stmt = $this->db_obj->prepare($query);
        $stmt->bind_param('sssssi', $username, $firstName, $lastName, $email, $password, $groupID);
        return $stmt->execute();
    }

    public function update($object) {

        $id = $this->db_obj->escapeString($object->getID());
        $username = $this->db_obj->escapeString($object->getUsername());
        $firstName = $this->db_obj->escapeString($object->getFirstName());
        $lastName = $this->db_obj->escapeString($object->getLastName());
        $email = $this->db_obj->escapeString($object->getEmailAddress());
        $groupID = $this->db_obj->escapeString($object->getGroupID());
        $password = password_hash($this->db_obj->escapeString($object->getPassword()), PASSWORD_DEFAULT);

        $query = "UPDATE " . $this->table . " SET username = ?, first_name = ?, last_name = ?, email_address = ?, password = ?, group_id = ? WHERE id = ?";
        $stmt = $this->db_obj->prepare($query);
        $stmt->bind_param('sssssii', $username, $firstName, $lastName, $email, $password, $groupID, $id);
        return $stmt->execute();
    }

    public function delete($id) {
        $id = $this->db_obj->escapeString($id);

        $query = "DELETE FROM " . $this->table . " WHERE id = ?";
        $stmt = $this->db_obj->prepare($query);
        $stmt->bind_param('i', $id);
        return$stmt->execute();
    }
}

I am aware that I am missing some validation and error checking throughout the code. Regarding validation I want to do some checks of the format of data being supplied, whilst I need to do some further research regarding the appropriate ways for trying and catching errors. Would just be grateful to know if I am going down the correct approach, and any suggestions people may have.

Answer 1

First of all, that's quite a robust approach from the architectural point of view. The only drawbacks are in the inconsistent implementation.

1. Autoload.

Forget about require_once forever.

• use namespaces
• store your classes in catalogs the the same name as namespaces
• either call spl_autload_register() manually with a simple function that will require your classes automatically, or tell Composer to do that for you

2. Connection.

This issue is rather big. In no time you will get "Too many connections error" because every instance of every repository will create its own database connection. An instance of the database connection class, be it vanilla mysqli or your own class, has to be made only once!

Then, this instance should be passed to other classes via constructor, i.e.

public function __construct(MySQLConnector $db) {
    $this->db_obj = $db;
}

3. Connection class.

Well yes, I don't see much reason in having such a class. At least some functionality has to be added. Given mysqli prepared statements are quite inconvenient, I would at least add a method to do the reoutine binding, based on my mysqli helper function:

public function preparedQuery($sql, $params, $types = "")
{
    $types = $types ?: str_repeat("s", count($params));
    $stmt = $this->link->prepare($sql);
    $stmt->bind_param($types, ...$params);
    $stmt->execute();
    return $stmt;
}

4. Escaping table names.

There should be an escapeIdent() function which you should always apply to all table and column names as you'll never know which one would coincide with a MySQL keyword

function escapeIdent($field){
    return "`".str_replace("`", "``", $field)."`";
}

5. Prepared statements.

This is the most important issue. You've got everything confused here, having useless double "protection" in some places and no protection at all at others.

1. Forget about escapeString() method. Remove it from your class. You will never ever need this function.
2. Remove all calls to this function from the repository as well.

3. Replace ALL (in the meaning 100%) date variables in your queries with placeholders. There should never be a horror like

   $query = 'select * from ' . $this->table . ' where id = '.$id;
   

   seriously, where have you been all this time when everyone was talking about SQL injection?

4. The prepared_query() method from the above is to help

   public function getById($id) {
       $table = $this->db_obj->escapeIdent($this->table);
       $query = "select * from $table where id = ?";
       $result = $this->db_obj->preparedQuery($query, [$id])->get_result();
       return $this->db_obj->fetchAssoc($result);
   }
   

6. Automation.

See this answer to How to avoid code repetition with PHP SQL prepared statements? for some ideas in order to make your repository methods less verbose.

7. Other issues

I am aware that I am missing some validation and error checking throughout the code.

Quite contrary, there should be none.

• validations does not belong to a repository class
• neither does error checking. What are going to do in this checking code anyway? Read my article on the proper PHP error reporting.

public function getAll();

Trust me, you will never ever need such a function in the real life. A function you need instead is getBySQL().