5
\$\begingroup\$

I want to create an educational example of a Data mapper type ORM. You know, it is always interesting to know how the stuff works, but when you take a fully functional software package, it's just impossible to understand its core principles.

If you take even a relatively concise implementation, an Atlas ORM for example, and try to dig it out in order to understand what a Data mapper is, you'd simply give up, unable to tell what is related to the basic pattern and what is added to satisfy other patterns/good practices/real life issues.

So my goal is to create a simplest Data Mapper possible, just to demonstrate the principle, implementing basic CRUD operations. I wanted to make it less than 100 lines of code and succeeded, with help of two default considerations:

  • An unique identifier for the record is assumed to be called "id"
  • A table name is equal to the lowercase form of a base class name.

I'd like to know is it possible to make it better, so, the questions are:

  • how to make it better compliant to the original Data Mapper pattern
  • how to make it closer to the real life usage (in its current form it's hardly usable at all)
  • any other suggestions or improvements you might have

The code and the usage example are below:

<?php
include 'init.php';

$pdo->query("create temporary table dmtestuser (id int primary key auto_increment, name varchar(25))");

$dm = new PrimitiveDM($pdo);

class dmTestUser {}

$user = new dmTestUser();
$user->name = "Joe";
$dm->save($user);

$dm->delete($user);

$user = new dmTestUser();
$user->name = "Bob";
$dm->save($user);

$id = $user->id;

$user = $dm->find('dmTestUser', $id);
$user->name = "Jane";
$dm->save($user);

$userList = $dm->findBySql('dmTestUser', "SELECT * FROM dmtestuser");
var_dump($userList);


class PrimitiveDM {

    protected $db;
    protected $driver;

    public function __construct(\PDO $pdo, $driver = 'mysql')
    {
        $this->db = $pdo;
        $this->driver = $driver;
    }

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

    protected function escapeIdent($ident)
    {
        switch ($this->driver)
        {
            case 'mysql':
                return "`".str_replace("`", "``", $ident)."`";

            default:
                throw new \Exception("You must define escape rules for the driver ($driver)");
        }
    }

    public function findBySql($class, $sql, $params = [])
    {
        return $this->query($sql, $params)->fetchAll(\PDO::FETCH_CLASS, $class);
    }

    public function find($class, $id)
    {
        $table = $this->escapeIdent(strtolower(basename($class)));
        $sql = "SELECT * FROM $table WHERE id = ?";
        return $this->query($sql, [$id])->fetchObject($class);

    }

    public function delete($object)
    {
        $table = $this->escapeIdent(strtolower(basename(get_class($object))));
        $sql = "DELETE FROM $table WHERE id = ?";
        $this->query($sql, [$object->id]);

    }
    public function save($object)
    {
        $table = $this->escapeIdent(strtolower(basename(get_class($object))));
        $properties = get_object_vars($object);
        $params = array_values($properties);

        if (!empty($object->id))
        {
            $set = '';
            foreach($properties as $name => $value)
            {
                $set .= $this->escapeIdent($name) . " = ?,";
            }
            $set = substr($set, 0, -1);
            $sql = "UPDATE $table SET $set WHERE id = ?";
            $params[] = $object->id;
            $this->query($sql, $params);

        } else {

            $names = '';
            foreach($properties as $name => $value)
            {
                $names .= $this->escapeIdent($name) . ",";
            }
            $names = substr($names, 0, -1);
            $values = str_repeat('?,', count($params) - 1) . '?';
            $sql = "INSERT INTO $table ($names) VALUES ($values)";
            $this->query($sql, $params);
            $object->id = $this->db->lastInsertId();
        }
    }
}
\$\endgroup\$
8
\$\begingroup\$

I don't fully understand the desire for a "generic" data mapper. Oftentimes, flexibility leads to complexity and fragility in your application. Take for example your delete() method. It just arbitrarily accepts whatever object is passed and starts deleting things from the database. What is to stop someone from just creating a new object named similarly to a table you don't want deletes happening against (like a configuration table) and arbitrarily destroying records in the database?

This flexibility can lead to bad habits like doing SELECT * queries, overuse of publicly accessible properties on your objects, and using PDO::fetch_object() which can circumvent class constructor behavior.

I guess as an educational exercise like this perhaps has value, but for a real-world production-level application, I think this class is pretty much a non-starter.

I know it might seem like a lot of extra code to have to build "model" classes for each of your database entities, but I would strongly suggest doing just this, as you will likely find over time that a one-size-fits all mapper such as this just becomes a mess when trying to adapt it to the various use cases you may have in a more complex application.

I DO think that what you have may be informative towards such classes, as a lot of what you have written might be well-aligned with an abstract base model class which is extended for each entity type. A "search by id" function for example, is probably pretty common across all classes, where you might find that an update method might be hard to implement in a base class and should perhaps be defined as an abstract method there.

Some more specific notes follow.


I don't understand the need for $driver and escaping you are doing here. If you go with an inheritance model where inheriting class specify the tables/columns they related to, you can get away from all this unnecessary escaping of database object names.


Consider using more meaningful/specific variable and method names. For example

$db => $pdo (since it holds a PDO object)
query() => prepare() (since this method does not perform a query at all, but rather creates a prepared statment)
find() => findById()

Should your save() method actually be broken up into create() and update() methods? I would think that a programmer should be explicit about what they are expecting to happen here. And you should not ever let the class get set up to where id would be empty if the record has been persisted to the database.


Your code really only considers happy path. What if prepared statements fail? What if select queries return zero rows?


You are leaking implementation details outside this class. Why would you be returning PDOStatement objects to the caller like in your query() method. If this class wants to be a mapper, it should ONLY be a mapper, not a general purposes class for preparing statements.


findBySql() is an odd method. It accepts arbitrary query input, so how to you know that the query is even a SELECT vs. some other type of query. Why would a mapper class even need to accept arbitrary SQL?


Why not use PDO::quote() for escaping string rather than your own logic?


Your public methods really do nothing to validate that the arguments being passed are suitable to be working with. You should never get so far as to prepare a statement on the database (a relatively expensive operation) if you have not validated that you have reasonable values to work with.

I would suggest you add guarding code at the first few lines in this methods to validate the input and fail with InvalidArgumentException or similar before you get to executing method logic. Fail early and fail loudly.


You repeat this code: strtolower(basename(get_class($object))) in several methods. Perhaps your class needs a getTableNameFromClass() method that can be used in your functions, or this should be part of escapeIdent().


I challenge your thinking that a class name and table name should be the same with the exception of camel casing. Most relational databases treat database object names (tables, columns, etc.) in a case-insensitive fashion or could have problems if the database is run on different OS that vary on case sensitivity with regard to the physical file names in which the data is stored. So many consider it best practice to simply remove any potential chance for error and use snake_case in relational databases.

\$\endgroup\$
6
  • \$\begingroup\$ Thank you a lot for such a mind-blowing reviw. So I clearly see now that for the every data class there should be a sister datamapper class, that is responsible for the field list, a table name, an identifier field and, probably, relations (though I doubt I'll go that far). I'll set to work right away. \$\endgroup\$ Feb 14 '17 at 20:51
  • \$\begingroup\$ I'll only take the liberty to defend some minor issues. query() does actually run a query. save() is inspired by doctrine's persist()/Eloquent's save(), buth universal upsert methods. PDO::quote() escapes strings, while I need to escape identifiers (but based on your suggestion indeed I can drop it out). If a prepared statement fails, it will throw an exception. Generally, that means some serious problem and should result in a site-wide 503 error that is a business of another component. So I honestly don't see what should I do here. \$\endgroup\$ Feb 14 '17 at 21:00
  • \$\begingroup\$ And one more point to defend: the goal were to utilize the PDO's ability to create objects. But it seems that this feature is rather useless for whatever real life application. \$\endgroup\$ Feb 15 '17 at 8:18
  • \$\begingroup\$ @YourCommonSense I am not saying that fetching data directly into a class with PDO::fetchObject() is useless, but rather that it circumvents the class constructor when hydrating object properties (only calling constructor after properties have been saturated), which oftentimes may not be desirable behavior. WRT exceptions, I am glad to hear you are using PDO in exception mode (a good thing). This was not clear from code being used as there were no try-catch blocks like one would typically see. Of course, if you just want the exceptions to bubble up this would not be needed. \$\endgroup\$
    – Mike Brant
    Feb 15 '17 at 14:27
  • \$\begingroup\$ I meant, if I should go down that road and create a distinct datamapper class for the every class that have to handled though an ORM, this datamapper class would take care of instantiating an object, thus making this PDO feature rather unemployed - right? \$\endgroup\$ Feb 15 '17 at 15:10
5
\$\begingroup\$

Here's a funny question for you: if this exact same code returned arrays, instead of objects, would it still be a data mapper? ;-)

Regardless, it should be kind-of easy to specify alternative primary key columns. I suppose something like:

public function setPrimary($class, $col) {
    $this->primary[$class] = $col;
}

And then any time you need the primary column:

$primary = $this->primary[$class] ?? 'id';

I presume something similar would be possible for mapping class names to table names:

public function setTable($class, $table) {
    $this->table[$class] = $table;
}

// ...

$table = $this->table[$class] ?? $class;

As far as real-life usage: ah, there's the rub. "Real life" usage is going to require a lot more than 100 lines (or 1000 lines) can give. :-/

\$\endgroup\$
1
  • \$\begingroup\$ Thank you Paul! The first question is really mind-changing. Indeed there will be no difference. So I should go for a mapper class. \$\endgroup\$ Feb 15 '17 at 7:27
3
\$\begingroup\$

I'm not sure if it was intentional but it appears that for the most part PSR-2 is being followed, except that in save() the opening braces for the control structures are on the next line for the if statements but on the same line for the else statement.

if (!empty($object->id))
{
    ...
} else {

And similarly for the foreach.

Perhaps it feels like the code is self-documenting enough but it wouldn’t hurt to add docblocks above each class and method...


The code within save() to generate the UPDATE statement generates the variable set with the following lines:

$set = '';
foreach($properties as $name => $value)
{
    $set .= $this->escapeIdent($name) . " = ?,";
}
$set = substr($set, 0, -1); 

Whenever I see code like the last line to remove the last comma, it makes me cringe. Instead of appending to a string the loop could push values into an array and then join them with join():

$set = [];
foreach($properties as $name => $value)
{
    $set[] = $this->escapeIdent($name) . " = ?";
}
$fieldsToSet = join(',', $set);

While it may require slightly more computational power, a functional approach could be used and would require three less lines of code:

$set = implode(',', array_map(function($name) {
    return $this->escapeIdent($name) . " = ?";
}, array_keys($properties)));

And I noticed that if id is set, that will be included in the list of fields to set - while it likely won't lead to changing the value, it may be wise to remove that from the list of fields to set.

And when getting the list of fields to set - a functional approach makes it a single liner, since nothing needs to be appended to each value returned from the method call:

$names = implode(',', array_map([$this, 'escapeIdent'], array_keys($properties)));

That takes a block of 6 lines down to 1! Combined with the change to generate $set this could be a reduction of 9 lines... could allow for expansion of your code and keeping it under 100 lines.

One could argue that $names could be better named - e.g. $fieldsToSet, $setFields, etc.

\$\endgroup\$

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.