Selecting and inserting MySQL records using PDO

Question

This PHP class connects to and queries the database MuSKL. I am a novice, and I ask you to comment on the approach and style of code.

<?php 
class MysqlAccess {
 private $dsn = 'mysql:dbname=bid;host=127.0.0.1';
 private $user = 'bid_root';
 private $password = 'e1e2e3e4e5er';
 private static $_db;
 private $_PDO;
 public $fetchQueryResult;
 private function __construct() {
 try {
 $this->_PDO = new PDO($this->dsn, $this->user, $this->password);
 $this->_PDO->exec('SET NAMES utf8');
 } catch (PDOException $e) {
 echo $e->getMessage();
 }
 }
 protected function __clone() {}
 static public function getInstance() {
 if (is_null(self::$_db)) {
 self::$_db = new self();
 }
 return self::$_db;
 }
 public function querySelect($tableName, array $arrayColum) {
 if (!is_array($arrayColum) or ! is_string($tableName)) {
 die('Неверный формат данных в запросе SELECT в запросе SQL');
 }
 $colums;
 foreach ($arrayColum as $value) {
 if (!is_string($value)) {
 die('Неверный формат данных в запросе SQL');
 }
 $colums .= SequrityData::SequrityReturnData($value) . ', ';
 }
 $colums = substr($colums, 0, -2);
 // запрос к базе данных
 $results = $this->_PDO->query('SELECT ' . $colums . ' FROM ' . SequrityData::SequrityReturnData($tableName));
 $this->fetchQueryResult($results);
 }
 public function queryInsert($tableName, array $arrayColums) {
 if (!is_array($arrayColums) or ! is_string($tableName)) {
 die('Неверный формат данных в запросе SELECT в запросе SQL');
 }
 $colums;
 foreach ($arrayColums as $key => $value) {
 if (!is_string($value) or ! is_string($key)) {
 die('Неверный формат данных в запросе SQL');
 }
 $colums .= SequrityData::SequrityReturnData($key) . ', ';
 $values .= '\'' . SequrityData::SequrityReturnData($value) . '\', ';
 }
 $colums = substr($colums, 0, -2);
 $values = substr($values, 0, -2);
 $tableName = SequrityData::SequrityReturnData($tableName);
 $insert = "INSERT INTO $tableName ($colums) VALUES ($values)";
 $this->_PDO->query($insert);
 }
 function __set($name, $value) {
 if (!is_string($name) && !is_string($value)) { // проверка на строку
 die('Неверный формат в сеттере');
 }
 $this->$name = $value;
 }
 private function fetchQueryResult($results) {
 $arrResult = $results->fetchAll(PDO::FETCH_ASSOC);
 foreach ($arrResult as $key => $value) {
 $this->$key = $value;
 }
 }
}

• tim: What does SequrityReturnData do?

Answer 1

Manually escaping values is generally a bad thing to do, you should use prepared statements instead.

When you generate the query, instead of concatenating directly the values you should concatenate placeholders and the same time generate an array with the values and the placeholders as keys.

public function queryInsert($tableName, array $arrayColums) {
 if (!is_array($arrayColums) or ! is_string($tableName)) {
 die('Неверный формат данных в запросе SELECT в запросе SQL');
 }
 $colums = $placeholders = '';
 $values = array();
 foreach ($arrayColums as $key => $value) {
 if (!is_string($value) or ! is_string($key)) {
 die('Неверный формат данных в запросе SQL');
 }
 $colums .= SequrityData::SequrityReturnData($key) . ', ';
 $placeholders .= ':' . SequrityData::SequrityReturnData($key) . ', ';
 $values[':' . SequrityData::SequrityReturnData($key)] = $value
 }
 $colums = substr($colums, 0, -2);
 $placeholders = substr($placeholders, 0, -2);
 $tableName = SequrityData::SequrityReturnData($tableName);
 $insert = "INSERT INTO $tableName ($colums) VALUES ($placeholders)";
 $query = $this->_PDO->prepare($insert);
 $query->execute($values);
}

Also, most of the times is generally not a good idea to call die() or echoing directly from inside a class. What you should do when something unexpected happens is to throw an exception and then on the client code you can catch it, log it and display a friendly error message to the user.

Inside class:

if (!is_string($value) or ! is_string($key)) {
 throw new MysqlAccessException('Неверный формат данных в запросе SQL');
}

Client code:

try {
 $conn->queryInsert(...);
}
catch (MysqlAccessException $e) {
 $logger = new ExceptionLogger();
 $logger->error($e);
 $notifier = new Notifier();
 $notifier->error('Oops! We are sorry but there was an error, please try again later.');
}

It's also a good idea to catch any PDOExceptions inside the class, log them, and then inside the catch block throw a more generic type of exception like MysqlAccessException to catch on the client code. You don't want to catch PDOExceptions on the client code because they are very specific, and if you decide in the future to change the database class and use something else instead of PDO you will have the to update the client code as well.

try {
 $query->execute($values);
}
catch (PDOException $e) {
 $logger = new ExceptionLogger();
 $logger->error($e);
 throw new MysqlAccessException('Unable to execute insert.');
}

An other thing is that you should never hardcode database credentials in your database class. You should pass them through the constructor. A nice idea would be to have a dedicated config class that is responsible for providing the app configuration data by parsing a config file. This will also enable you to decouple the logic from the data and have different config files for production and development environments. An other reason why you don't want to hardcode this kind of data, is that if you use a versioning system like git and you are not the only person that maintains the code you don't want to commit sensitive data to the repository.

• Sᴀᴍ Onᴇᴌᴀ: The OPs code had one call to SequrityData::SequrityReturnData($key) ( as well as two other calls with other values as parameters). The suggested code contains three calls to SequrityData::SequrityReturnData($key). Bearing in mind the OP did not disclose the internals of that method, it may be worth mentioning that it may be beneficial to only call the method once and store the return value in a variable to be used later instead of calling it multiple times.