1
\$\begingroup\$

I'm trying to improve my php oop programming skills. As an exercise I wrote this class that will generate and execute dynamic sql queries. I've also writed a class to call various sanitization filters that php has built in.

QueryBuilder.php class

<?php

class QueryBuilder{

private $db = null;
private $stmt;
private $table;
private $param;
private $cols, $columns;
private $holders, $placehold;
private $fields, $field;

public $data;
public $results;

public function __construct(\PDO $db){
    $this->db = $db;
}

public function insert($table, array $data, array $columns){
    $holders = $this->setHolders($columns);
    $cols = $this->setColumns($columns);
    $stmt = $this->db->prepare("INSERT INTO $table ($cols) VALUES ($holders)");
    return $stmt->execute($data);
}

public function select($table, array $columns, $field, $param){
    $cols = $this->setColumns($columns);
    $stmt = $this->db->prepare("SELECT $cols FROM $table WHERE $field = ?");
    $stmt->execute(array($param));
    $result = $stmt->fetch();
    return json_encode($result);
}

public function edit($table, array $columns, array $data, $param){
    $fields = $this->setFields($columns);
    $stmt = $this->db->prepare("UPDATE $table SET $fields WHERE $param = ?");
    return $stmt->execute($data);
}

public function delete($table, array $data, $param){
    $stmt = $this->db->prepare("DELETE FROM $table WHERE $param = ?");
    return $stmt->execute($data);
}

private function setColumns(array $columns){
    $cols = implode(', ', array_values($columns));
    return $cols;
}

private function setFields(array $columns){
    $fields = implode(' = ?, ', array_values($columns));
    return $fields.' = ?';
}

private function setHolders(array $columns){
    $holders = array_fill(1 ,count($columns),'?');
    return implode(', ',array_values($holders));
}


}

?>

DataSanitizer.php class

<?php

class DataSanitizer{

private $value;
private $sanitized_value;

public function intSanitize(int $value){
    $sanitized_value = filter_var($value, FILTER_SANITIZE_NUMBER_INT);
    return $sanitized_value;
}

public function stringSanitize(string $value){
    $sanitized_value = filter_var($value, FILTER_SANITIZE_STRING);
    return $sanitized_value;
}

public function floatSanitize(float $value){
    $sanitized_value = filter_var($value, FILTER_SANITIZE_NUMBER_FLOAT);
    return $sanitized_value;
}

public function emailSanitize(string $value){
    $sanitized_value = filter_var($value, FILTER_SANITIZE_EMAIL);
    return $sanitized_value;
}

public function validateEmail(string $value){
  $sanitized_value = filter_var($value, FILTER_VALIDATE_EMAIL);
  return $sanitized_value;
}

}

?>

Classes usage example:

<?php
// Usaually i use the spl_autoloader_register();
require_once 'QueryBuilder.php';
reqiure_once 'DataSanitizer.php';

// this file holds the PDO connection stored inside the $db variable
require_once 'Config.php';

$query = new QueryBuilder($db);
$sanitize = new DataSanitizer;

$table = 'test_table';
$data = array($sanitize->stringSanitize('hello'),$sanitize->stringSanitize('world'));
$col = array('col1','col2');

$query->insert($table, $data, $col);

?>
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

The attempt is quite good, you managed to elude the most critical faults. There are only rather minor and architectural nitpicks.

First of all, I don't know what does DataSanitizer to do here but it shouldn't be any part of a query builder. Data sanitization is tangential to data persistence and they have nothing in common. So it should be just taken away.

As of the query builder, there are two main problems

  • a potential vulnerability which I explained recently in the other answer. At the very least all identifiers must be formatted according to database rules.
  • select is unusable in the real life. Nobody have seen such a method in the wild, it lurks only in such educational builders. If you want a query builder for SELECT, then either create a real one, with distinct methods for each SQL clause, or stick to a free-form query() method. Trust me, SQL is a precious and powerful programming language, do not diminish it to a crippled gibberish.

Also

  • Looks like class variables beside $db are just for decoration, cause they never used in the code? And for the good, so just ditch them away.
  • consider implementing PSR-4 Autoload for your classes.
  • usually, the data format for such methods as insert and edit is like

    'col1' => value1,
    'col2' => value2,
    

    so you have to provide only one array with data. However it has other drawbacks with security already explained above and so must be used with caution

All in all, I believe you would benefit from my article on the common mistakes in database wrappers even if yours is a query builder, not a wrapper.

\$\endgroup\$
6
  • \$\begingroup\$ DataSanitizer is a class to manage user input sanitization. The $db variable is an instance of PDO and is used in every $stmt declaration to call the prepared statements. I will consider to make more secure every aspects of the class and to improve the select method to be more usable in real life. \$\endgroup\$ Commented Jul 11, 2018 at 14:36
  • \$\begingroup\$ I do understand what DataSanitizer is. My point is that is has nothing to do with a query builder which we are reviewing. Regardless that, I don't see why would you use such a sanitization at all. What's the point in doing a ($sanitize->stringSanitize() call? What's the purpose? Particular purpose I mean not a vague "to sanitize". To sanitize what and why? \$\endgroup\$ Commented Jul 11, 2018 at 15:14
  • \$\begingroup\$ I want to use it when I need to get users data from a form. Prepared statements are safe against SQL injection but sanitizing inputs before passing them to the db is a standard security measure. So to avoid to write every time filter_var() in my controller called using ajax, I've decided to put the various functions inside a class. \$\endgroup\$ Commented Jul 11, 2018 at 18:04
  • \$\begingroup\$ I beg my pardon, nothing standard in making some magical gestures which purpose is unknown to you. You still fail to name the actual use of such "measure" so I suppose such a sansitization is not a conscious act but a cargo cult programming. \$\endgroup\$ Commented Jul 11, 2018 at 18:27
  • 2
    \$\begingroup\$ I suggest you to use this class on purpose. To consider why you should use it and how. If you want to pre-sanitize your input data, then you must consider the strategy first. How your application would know which rule to apply to which input field. What to do if validation fails, and so on. Just don't use it blindly, only because it's a "standard measure". every sanitize filter does certain job- so you should investigate and decide whether you need it and why. your current approach gives you only a false feeling of safety. but there is no all-embracing sanitization that makes your data "safe" \$\endgroup\$ Commented Jul 12, 2018 at 3:54

Not the answer you're looking for? Browse other questions tagged or ask your own question.