Database connection and DataMapper

Question

Following my previous question Database interface and PDO adapter, I wrote a new simple class to connect to Database and run a query returning a PDO statement.

 <?php

namespace System\Database;

use \PDO;

/**
 * Database connection class
 */
class DB
{
    protected $pdo;

    /**
     * Connect to database
     * @param array $config
     */
    public function __construct(array $dbConfig)
    {
        $connStr = 'mysql:host='.$dbConfig['server'].';port='.$dbConfig['port'].';dbname='.$dbConfig['name'].';charset='.$dbConfig['charset'];

        try{
            $this->pdo = new PDO(
                $connStr,
                $dbConfig['username'],
                $dbConfig['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);

            return true;

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

    }

    /**
     * Execute a $sql query with or without params. If $params is Multidimensional array bind variables passed by
     * @param  string $sql    Query to execute
     * @param  array  $params Params
     * @return PDOStatement
     */
    public function run(string $sql, array $params = [])
    {
        if(empty($params)){
            return $this->pdo->query($sql);
        }

        $stmt = $this->pdo->prepare($sql);

        if(is_array($params[0])){
            foreach ($params as $param) {
                $stmt->bindParam(
                    $param['name'],
                    $param['value'],
                    $param['pdoType'],
                    $param['length']
                );
            }
            $stmt->execute();
        }else{
            $stmt->execute($params);
        }

        return $stmt;
    }

    /**
     * Returns the last inserted id
     * @return int ID
     */
    public function lastInsertId(): int
    {
        return $this->pdo->lastInsertId();
    }

    /**
     * Close the connection
     */
    public function closeConnection()
    {
        $this->pdo = null;
    }

}
 ?>

After that I'm writing a data mapper class as follow:

<?php

namespace System\Database;

use System\Database\DB;
use \PDO;

/**
 *
 */
abstract class DataMapper
{
    protected $db;
    // name of the table
    protected $table;
    // table columns with info
    protected $_fields = array();

    // primary key
    protected $_primary;
    // flag if primary key is Auto Increment
    protected $_primaryAI = false;

    // protected $fillable = [];

    protected $_pdoBindTypes = [
        'char' => PDO::PARAM_STR,
        'int' => PDO::PARAM_INT,
        'bool' => PDO::PARAM_BOOL,
        'date' => PDO::PARAM_STR,
        'time' => PDO::PARAM_INT,
        'text' => PDO::PARAM_STR,
        'blob' => PDO::PARAM_LOB,
        'binary' => PDO::PARAM_LOB
    ];

    public function __construct(DB $db)
    {
        $this->db = $db;
        if(!empty($this->table))
        {
            $this->getTableFields();
        }
    }

    /**
     *  Get column list form table
     *  [internal function]
     */
    protected function getTableFields()
    {
        // Fetch all columns and store in $this->_fields
        $fields = $this->db->run("DESCRIBE $this->table")->fetchAll(PDO::FETCH_ASSOC);
        foreach($fields as $field){
            $this->_fields[$field['Field']] = $this->parseColumnType($field['Type']);

            if($field['Key'] == 'PRI'){
                $this->_primary = $field['Field'];
                $this->_primaryAI = ($field['Extra'] == 'auto_increment');
            }
        }
    }

    /**
     *  Parse PDO-produced column type
     *  [internal function]
     */
    protected function parseColumnType($colType)
    {
        $colInfo = array();
        $colParts = explode(" ", $colType);
        if($fparen = strpos($colParts[0], "("))
        {
            $colInfo['type'] = substr($colParts[0], 0, $fparen);
            $colInfo['length']  = (int)str_replace(")", "", substr($colParts[0], $fparen+1));
            $colInfo['attributes'] = isset($colParts[1]) ? $colParts[1] : NULL;
        }
        else
        {
            $colInfo['type'] = $colParts[0];
        }

        // PDO Bind types
        $pdoType = '';
        foreach($this->_pdoBindTypes as $pKey => $pType)
        {
            if(strpos(' '.strtolower($colInfo['type']).' ', $pKey)) {
                $colInfo['pdoType'] = $pType;
                break;
            } else {
                $colInfo['pdoType'] = PDO::PARAM_STR;
            }
        }

        return $colInfo;
    }

    /**
     *  Get info of the column
     *  [internal function]
     * @param  string $column Column name
     * @return array          Type and lenght
     */
    protected function getColumnType(string $column): array
    {
        return [
            'pdoType' => $this->_fields[$column]['pdoType'],
            'length' => isset($this->_fields[$column]['length']) ?: NULL,
        ];
    }

    /**
     * Return array with all info for binding
     * [internal function]
     * @param  string $column column name
     * @param  [type] $value  value
     * @return array
     */
    protected function bindColumnType(string $column, $value): array
    {
        return array_merge(
            [
                'name' => ":{$column}",
                'value' => $value,
            ],
            $this->getColumnType($column)
        );
    }


    /**
     * Find a record by id and return a row
     * @param  [type] $id
     * @param  [type] $mode PDO Fetch Mode
     * @return [type]       row
     */
    public function findById($id, $mode = PDO::FETCH_ASSOC)
    {
        // TODO: if not setted, throw exception
        if(empty($this->table) or empty($this->_primary)) return false;

        $pk = $this->_primary;

        return $this->db
            ->run("SELECT * FROM $this->table WHERE $pk = :$pk", [$this->bindColumnType($pk, $id)])
            ->fetch($mode);
    }

    /**
     * Insert a new record
     * @param array $data data to insert [field => value]
     */
    public function insert($data)
    {
        $fieldsStr = '';
        $valuesStr = '';

        $dataValues = array();

        // generate sql query
        foreach ($data as $f => $v) {
            // check if exist the column name
            if(array_key_exists($f, $this->_fields))
            {
                // if there is a primary key field in $data and it's "auto increment", skip
                if($f == $this->_primary and $this->_primaryAI) continue;

                $fieldsStr .= $f.',';
                $valuesStr .= ":{$f},";

                $dataValues[] = $this->bindColumnType($f, $v);
            }
        }

        // remove last ,
        $fieldsStr = substr($fieldsStr, 0, -1);
        // remove last ,
        $valuesStr = substr($valuesStr, 0, -1);

        $this->db->run("INSERT INTO {$this->table} ({$fieldsStr}) VALUES ({$valuesStr})", $dataValues);
    }

    //TODO: update, delete
}




 ?>

I'm on the right way? Is It correct to get the list of table columns from the db to check the fields to be entered? Or is it better declare them in the child class like this:

<?php

namespace App;

use System\Database\DataMapper;

/**
 *
 */
class User extends DataMapper
{
    protected $table = 'users';

    protected $fillable = [
        'firstname' => 'string:20',
        'lastname' => 'string:20',
        'age' => 'int:3',
        'email' => 'email',
    ];

    public function test(){
        // do something
    }
}

 ?>

In the data mapper I should parse these informations to set the correct type and lenght for each field.

Answer 1

First of all, again, it's a very good approach. You are going in the right direction. Such a separation is a cornerstone of the data manipulation.

Regarding the particular implementation,

for the DB class I would suggest 2 things

• I don't think this approach with arrays in $params is really needed. I bet you made it just because PDO has such parameters, but not ouf of some real life necessity. Am I right? I would leave just $stmt->execute($params);, at least untill there will be a real case where you would need such a precise binding.
• for some reason you removed a very important method, getConnection(). You must keep it in your class until you implement every method and property supported by PDO. Until then use getConnection() to access them.

Now to the most interesting part, the data mapper.

Definitely, I would go for the second approach. Simply because it is much simpler and explicit. Just look at the code for the first approach? It's a mess. If I learned anything during my career is that the best code is a code that is easier to support. And of course the simpler the code the easier the support. Besides, the less code you have, the less errors it contain.

Besides, the latter approach is more explicit. You can get the idea of the data structure by just looking at the class. It helps a lot.

Hint: If you want some automation, you can use your database sniffer for a code generator, just like all major frameworks do: they don't use it for the Mapper directly, but use it to create Entity classes out of the database structure.

So, in the end I would get rid of this clumsy automation and leave only action methods:

abstract class BaseDataMapper
{
    protected $db;
    protected $table;
    protected $_primary = 'id';
    protected $_fillable = [];

    public function __construct(DB $db)
    {
        $this->db = $db;
        if (!$this->table) {
            throw new Exception("Table name is not defined");
        }
    }
    public function findById($id, $mode = PDO::FETCH_ASSOC)
    {
        $sql = "SELECT * FROM `$this->table` WHERE `$this->_primary` = ?";
        return $this->db->run($sql, [$id])->fetch($mode);
    }
    public function insert($data, $insertPk = false)
    {
        if (!insertPk) unset($data[$this->_primary]);
        $dataNames = array_keys($data);
        if (array_diff($dataNames, $this->_fillable)) {
            throw new Exception("Disallowed field name in the insert data");
        }
        $fieldsStr = "`".implode("`, `", $dataNames)."`";
        $valuesStr = str_repeat('?,', count($data) - 1) . '?';
        $sql = "INSERT INTO {$this->table} ({$fieldsStr}) VALUES ({$valuesStr})";
        $this->db->run($sql, array_values($data));
    }
}

As you can see, I also removed a lot of code.

• it makes sense to check the table only once
• I would make the primary key business a local affair for the insert method
• I would rather check whether $data contains illegal fields and fail explicitly than siletntly ignore them
• there are simpler ways to create a prepared INSERT query

Now this mapper could be used with the UserMapper class you posted in your question.

class UserMapper extends BaseDataMapper
{
    protected $table = 'users';

    protected $_fillable = [
        'firstname',
        'lastname',
        'age',
        'email',
    ];

    public function test(){
        // do something
    }
}

One final note: this mapper class works with arrays, whereas Data Mappers are usually deal with classes. It means a Data Mapper class is a sister class for a Data class (User), and therefore return (and accept) an object rather than an array. But it would be a different story.