Database interface and PDO adapter

Question

I'm writing my own framework in PHP and I want respect the SOLID principles.

I made this interface:

<?php

namespace System\Database;
use System\Config;

/**
 * Database wrapper interface
 */
interface Database
{
    /**
     * Connect to database
     * @param  Config $config
     * @return bool return true or throw Exception
     */
    public function connect(Config &$config) : bool;

    /**
     * Prepare a SQL query
     * @param  string $query       Query
     * @param  array  $params      Params to bind to query
     */
    public function prepare(string $query, array $params = []);

    /**
     * Execute prepared query, without return any datas
     */
    public function execute();

    /**
     * Execute prepared query and return all results
     */
    public function resultset();

    /**
     * Execute prepared query and return only a single row
     */
    public function single();

    /**
     * Return the number of row affected
     * @return int Row numbers
     */
    public function rowCount() : int;

    /**
     * Insert records in a table
     * @param  string $table Name of the table
     * @param  array  $data  Array with table fields and values - Ex: ['name' => 'test']
     */
    public function insertRecords(string $table, array $data);

    /**
     * Update records in a table
     * @param  string $table      Name of the table
     * @param  array  $changes    Array with table fields and values - Ex: ['name' => 'test']
     * @param  array  $conditions Conditions needed to perform it Ex: ['id' => 1]
     */
    public function updateRecords(string $table, array $changes, array $conditions);

    /**
     * Delete records in a table
     * @param  string $table      Name of the table
     * @param  string $conditions Conditions needed to perform it - Ex: "id = :id"
     * @param  array  $params     Params to replace in conditions
     * @return int                Row affected
     */
    public function deleteRecords(string $table, string $conditions, array $params = []) : int;

    /**
     * Returns the last inserted id
     * @return int ID
     */
    public function lastInsertId() : int;

    /**
     * Close the connection
     */
    public function closeConnection();

}
 ?>

Implemented by this class:

<?php
/*
* PDO Driver implementation
*/

namespace System\Database;

use System\Config;
use System\Database\Database;
use \PDO;

class PDODriver implements Database {
    private $pdo;
    private $stmt;
    private $connected = false;

    public function connect(Config &$config): bool
    {
        $connectionString = 'mysql:host='.$config->get('db_server').';port='.$config->get('db_port').';dbname='.$config->get('db_name');

        try{
            $this->pdo = new PDO(
                $connectionString,
                $config->get('db_username'),
                $config->get('db_password')
            );
            # We can now log any exceptions on Fatal error.
            $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

            # Disable emulation of prepared statements, use REAL prepared statements instead.
            $this->pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);

            $this->connected = true;
            return true;

        // Error handling
        }catch(PDOException $e){
            throw new \Exception("Failed to connect to DB: ". $e->getMessage(), 1);
        }
    }

    public function prepare(string $sql, array $params = [])
    {
        $this->stmt = $this->pdo->prepare($sql);
        if(!empty($params))
        {
            $this->bindParams($params);
        }
    }

    /**
     * Bind param value to prepared sql query
     * @param string $param
     * @param $value
     * @param $type
     */
    private function bind(string $param, $value, $type = null)
    {
        if(is_null($type))
        {
            switch (TRUE) {
                case is_int($value):
                    $type = PDO::PARAM_INT;
                    break;
                case is_bool($value):
                    $type = PDO::PARAM_BOOL;
                    break;
                case is_null($value):
                    $type = PDO::PARAM_NULL;
                    break;
                default:
                    $type = PDO::PARAM_STR;
            }
            $this->stmt->bindValue(':'.$param, $value, $type);
        }
    }

    /**
     * Bind a group of params
     * @param  array  $params Array with params and values Ex: ['name' => 'test']
     * @param  string $prefix Prefix to prepend to param name
     */
    private function bindParams(array $params, string $prefix = '')
    {
        foreach ($params as $key => $value) {
            $this->bind($prefix.$key, $value);
        }
    }

    /**
     * Eseque la query preparata
     */
    public function execute(){
        return $this->stmt->execute();
    }

    public function resultset()
    {
        $mode = PDO::FETCH_ASSOC;
        $this->execute();
        $this->stmt->fetchAll($mode);
    }

    public function single()
    {
        $mode = PDO::FETCH_ASSOC;
        $this->execute();
        $this->stmt->fetch($mode);
    }

    public function rowCount(): int
    {
        return $this->stmt->rowCount();
    }

    /**
     * Elimina record dal database. Es: (users, where id = :id, ['id' => 1])
     * @param string tabella
     * @param string $conditions campi e condizione
     * @param array $params valori delle condizioni
     * @return int affected rows
     */
    public function deleteRecords(string $table, string $conditions, array $params = []): int
    {
        $delete = "DELETE FROM {$table} WHERE {$conditions}";

        $this->prepare = $delete;
        if(!empty($params))
        {
            $this->bindParams($params);
        }
        $this->execute();

        return $this->rowCount();
    }

    /**
     * Aggiorna un record del database
     * @param string $table
     * @param array $changes con le modifiche [field => value]
     * @param array $conditions condizioni [id => 1]
     */
    public function updateRecords(string $table, array $changes, array $conditions)
    {
        $changesStr = '';
        $whereStr = '';
        $cond_array = [];

        foreach ($changes as $field => $value) {
            $changesStr .= "{$field}=:param_{$field},";
        }
        // rimuovo l'ultiam , in eccesso
        $changesStr = substr($changesStr, 0, -1);

        foreach($conditions as $condition => $value){
            $cond_array[] = "{$condition} = :where_{$condition}";
        }
        $whereStr = implode(' AND ', $cond_array);

        $this->prepare("UPDATE {$table} SET {$changesStr} WHERE {$whereStr}");

        //uso i prefissi per evitare sovrapposizioni tra parametri e condizioni
        $this->bindParams($changes, 'param_');
        $this->bindParams($conditions, 'where_');

        $this->execute();
    }

    /**
     * Inserisce record nel database
     * @param string $table tabella
     * @param array $data dati da inserire field => value
     * @return bool
     */
    public function insertRecords($table, $data)
    {
        $fieldsStr = '';
        $valuesStr = '';

        // genero la query
        foreach ($data as $f => $v) {
            $fieldsStr .= $f;
            $valuesStr .= ":{$f}";
        }

        // rimuovo la , in eccesso
        $fieldsStr = substr($fieldsStr, 0, -1);
        // rimuovo la , in eccesso
        $valuesStr = substr($valuesStr, 0, -1);

        $this->prepare("INSERT INTO {$table} ({$fieldsStr}) VALUES ({$valuesStr})");
        $this->bindParams($data);
        $this->execute();
        return true;
    }

    // Magic method clone is empty to prevent duplication of connection
    private function __clone(){
        return false;
    }
    private function __wakeup(){
        return false;
    }

    public function lastInsertId(): int{
        return $this->pdo->lastInsertId();
    }

    public function closeConnection(){
        $this->pdo = null;
    }

    // Get the connection
    public function getConnection(){
        return $this->pdo;
    }
}

?>

Is correct under the SOLID principles to insert the methods insertRecords, updateRecords and deletedRecords here or is better implement them in another class like DataMapper?

Your Common Sense · Accepted Answer · 2019-06-24 07:57:55Z

That's a pretty solid implementation, with many possible issues already solved. However, there is room for improvement still.

Here is a brief list:

Yes, you are absolutely right, a db wrapper must be separated from the data mapper, with insertRecords, updateRecords and deletedRecords moved into the latter.
Your database wrapper offers less features than original PDO, which never should be. PDO is a db wrapper of its own, and not a bad one. It makes no sense to duplicate the functionality that already exists in PDO, at the same time reducing the existing functionality.
- bind() function is dangerous. Sniffing the database type from the PHP variable type could lead to unwanted consequences. Instead, bind all parameters as strings, just like PDO does.
- overall there is so much code to recreate the functionality that already exists in PDO. For example, your resultset() method could be implemented like this
```
public function resultset($sql, $params = [], $mode = PDO::FETCH_ASSOC)
{
    $stmt = $this->pdo->prepare();
    $stmt->execute($params);
    return $stmt->fetchAll($mode);
}  
```
  as you can see, it is implemented using only native PDO methods and in fact easier to use as it does everything in one call as opposed to your own consequent calls to prepare() and resultset(). Not to mention it makes such functions like bindParams(), bind(), prepare(), execute() just useless.
Your data mapper functions are potentially prone to SQL injection through field names. For this reason I strongly recommend to create a real data mapper class where each mapper is related to a distinct table with all field names explicitly written in the class definition. But that will be another story, I would suggest you to write a mapper and then post it for the review.
$stmt by no means should be a class variable as it will make your class stateful, while it shouldn't be. The link is to my review of common mistakes in db wrappers you may find useful.
charset must be set in the DSN. See my article on the proper PDO connection
I don't really get why config is passed by reference. I would remove that ampersand.
the mode in resultset() and single() is hardcoded which I suppose is a typo. of course it should be a function parameter.
the code in resultset() and single() is duplicated. Besides, both methods duplicate the functionality already exists in PDO. I would suggest to create a single function query() that returns a PDOStatement from wich you will be able to get any result using the method chaining. An example can be found in this answer

Regarding the Interface.

I would say that before creating an Interface, you must consider the architecture. There are some issues that must be taken into consideration. As it was noted above, under no circumstances a database class must be stateful. It means that you have only two possibilities:

to use a leaky abstraction, so your class won't be 100% independent but it will return a PDOStatement. This way is easier to implement but in this case I don't see a much use for an Interface. Given your abstraction is already leaky (not all PDO methods are implemented and there is a fallback method to get a raw PDO instance) I would go this way. but it will not be 100% a good practice.
to make a proper abstraction, for which you will need to have at least two Interfaces - one for a database wrapper class and one for a statement class, each implementing 100% of PDO functionality.

Thank you for your reply and your links. They was very usefull. I've written some new code and I posted a new question here — fabrix
– fabrix, Commented Jun 27, 2019 at 22:06

Stack Exchange Network

Database interface and PDO adapter

1 Answer 1

You must log in to answer this question.

Linked

Hot Network Questions

Database interface and PDO adapter

1 Answer 1

You must log in to answer this question.

Linked

Related

Hot Network Questions