Implentation of Data Access Object Pattern

Question

I'm tried to implement the Data Access Object Pattern. I have a base abstract class that is then extended for a specific table. I tried following some guides, but couldn't find a lot of info on this pattern.

<?php
namespace NicholasJohn16\Database;
use \PDO;
abstract class DataAccessObject
{
 /**
 * @var \PDO
 */
 protected $pdo;
 protected $_tablename;
 public function __construct($options = ['pdo' => null])
 {
 $this->pdo = $options['pdo'];
 }
 protected function select($id = 0)
 {
 $sql = "SELECT * FROM " . $this->_tablename;
 if ($id) {
 $sql .= " WHERE id = :id ";
 }
 $statement = $this->pdo->prepare($sql);
 if ($id) {
 $statement->bindParam('id', $id, PDO::PARAM_INT);
 }
 $statement->execute();
 return $id ? $statement->fetch() : $statement->fetchAll();
 }
 protected function insert($data, $columns = null)
 {
 $sql = "INSERT IGNORE INTO " . $this->_tablename;
 $keys = $columns ? $columns : array_keys($data);
 $names = array_map(function ($key) {
 return ":" . $key;
 }, $keys);
 $sql .= " (" . implode(', ', $keys) . ") ";
 $sql .= " VALUE ";
 $sql .= " ( " . implode(', ', $names) . "); ";
 $statement = $this->pdo->prepare($sql);
 $datum = is_null($columns) ? $data : array_intersect_key($data, array_flip($columns));
 $statement->execute($datum);
 return (int) $this->pdo->lastInsertId();
 }
 protected function update($id, $data)
 {
 $sql = "UPDATE " . $this->_tablename;
 $sql .= " SET ";
 $updates = [];
 foreach ($data as $key => $value) {
 $updates[] = "{$key} = :{$key}";
 }
 $sql .= implode(', ', $updates);
 $sql .= " WHERE id = :id";
 $statement = $this->pdo->prepare($sql);
 $statement->execute(array_merge($data, ['id' => $id]));
 $error = $statement->errorInfo();
 return $this->select($id);
 }
}

<?php
namespace NicholasJohn16\Database;
class Prices extends DataAccessObject
{
 protected $_tablename = 'prices';
 function addPrice($data)
 {
 return $this->insert($data);
 }
}

• Your Common Sense: Whoever voted minus, what was your reason?

Answer 1

Speaking specifically of DAO, as far as I can tell, it should provide an interchangeable interface. Which means, you shouldn't extend your implementation from the base class but rather use it as a dependency. This way, by simply passing another DAO instance as a constructor parameter, you'll be able to use multiple adapters that implement different data access methods. So your implementation should be changed like

class PricesModel
{
 protected $tablename = 'prices';
 protected $dao;
 public function __construct(DAOInterface $dao) {
 {
 $this->dao = $dao;
 $this->dao->setTableName($this->tablename);
 }
 function addPrice($data)
 {
 return $this->dao->insert($data);
 }
}

While your current implementation looks more like a TableGateway pattern, though I myself is not too strong in pattern taxonomy. Looking at your code from this point of view, I can suggest several improvements, given I already busied myself with a similar task.

First off, what is good here

• the idea itself. Strangely, but many PHP users stick to raw API calls, never thinking of reducing the boilerplate code
• the PDO object is a dependency
• there is no explicit error reporting to bloat the code, which is very good. All you need is to make sure that PDO's error reporting mode is set to exceptions and the rest should be left for the common error handling

And now to some essential issues

• Security is the main concern. In the update() method, you are adding input directly to SQL, which is a BIG NO. I see you tried to implement some white list in the insert() method, which is a good thing, but it's rather clumsy. Just go one step further and add another protected variable that contains the list of all columns in the table. And filter the $data variable's keys against it
  • to throw an error is much better than silently discard the stray column name
• avoid ambiguous methods. It costs you absolutely nothing to add a method that selects a single row. So I would suggest to have several methods of different purpose (the number can vary):
  • listBySql to get the rows based on arbitrary SQL (you'll definitely need it)
  • getBySql same as above but to get just one row
  • read - a CRUD method to read the record by its id
• INSERT IGNORE - I don't know where did you get that IGNORE from but you must definitely drop it out.
• the table and column names must be quoted. If you intend to use this class with different databases, a dedicated method must be added, which quotes each identifier before it's added to the query

Some minor issues

• it would be a good idea to make the identifier column configurable
• we don't use underscores in the property names anymore
• the $error = $statement->errorInfo(); looks alien and should be removed
• the insert/update methods can be slightly optimized. It's no use to run several loops where only one is enough.
• not sure if (int) with lastInsertId is a good idea. Identifiers can be strings.
• no sure if return $this->select($id); is really needed. Most of time you don't need the updated record while in case you need, it can be selected explicitly

• NicholasJohn16: Thanks for the feedback. The inputs for update() and insert() get filtered by filter_input before they get to the database. I used INSERT IGNORE cause I didn't care about duplicate data failing silently.
• Your Common Sense: That's odd. What makes you think that filter input has anything to do with SQL?
• Your Common Sense: Also, this approach smells procedural. A DAO is a class. And classes intended to be abstract. Independent. Self-sufficient. To show the same behavior in any environment, not in just some particular curated data flow. If your class depends on some external entity to work properly, it's not OOP, it's spaghetti
• Your Common Sense: And the same sort of applies to the IGNORE part. Suppressing errors is by no means an expected behavior. And a class shouldn't do that by default. That's too narrow-minded. I make it, you were using this class for just one single particular insert so far? To insert whatever prices? But what about inserting other entities? Users? Come on, at the very least insert should have a flag parameter, $ignore_errors, that you can set to true in such a rare case when error can be ignored. But by no means it should be a default behavior
• slepic: Wouldnt it be better to not mutate dao instance in the model constructor? ie by adding tableName argument to the dao's methods instead of setting it once for all calls.