3
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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().

\$\endgroup\$
  • \$\begingroup\$ Wow thank you for the comprehensive answer, just wanted to add a few points: 1) Have you used escape_mysql_identifier and escapeIdent interchangeably above? As I am not sure if they were meant to be the same function. Is there a better way to call it as right now I would need to put the same function in every repository class wouldn't I. 2) That was my mistake with the prepared statements, I have use them elsewhere and the inline SQL just slipped by. Does doing this completely nullify needing to escape string then? I am just working through your message now so start making changes. \$\endgroup\$ – James Pavett Jan 16 at 10:17
  • \$\begingroup\$ Yes, the same function, and yes, the point of prepared statements it does protect your queries, while escaping doesnt. \$\endgroup\$ – Your Common Sense Jan 16 at 10:35
  • \$\begingroup\$ Great. Would it be better for escapeIdent to be statically available under the MySQLConnector class? Just so it can be used elsewhere without duplicating? \$\endgroup\$ – James Pavett Jan 16 at 10:54
  • \$\begingroup\$ It should be a part of MySQLConnector, not repository. I was mistaken if I said otherwise. I don't see a reason to make it static, as it will never be used apart from other methods. \$\endgroup\$ – Your Common Sense Jan 16 at 10:56
  • \$\begingroup\$ Yea I think it was just the $this-> that made me think it belonged to the repository, but that has been changed now. Is there an accepted file structure when working like this? For me it feels like my plain classes (i.e. models like User) and stuff that acts like controllers (i.e. the repositories) should be separated slightly. \$\endgroup\$ – James Pavett Jan 16 at 11:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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