Dynamic PHP PDO insert method

Question

This is my attempt at making a class for dynamic PDO queries in PHP.

The code works, but I would like the hear what I can improve to make it more secure and readable.

class DB 

{


    private static $connection;


    public static function setConnection ($connection) 

    {

        static::$connection = $connection;

    }


    public static function insert($table, $data = []) 

    {

        try {

            $columnsArray = array_keys($data);
            $columnsString = implode(',', $columnsArray);

            $valuesArray = array_values($data);
            $valuesCount = count($valuesArray);

            $valuesPlaceholder = '';
            for ($i=0; $i < $valuesCount; $i++) { 
                $valuesPlaceholder .= '?,';
            }
            $valuesPlaceholder = rtrim($valuesPlaceholder, ',');


            $query = "INSERT INTO $table ($columnsString) VALUES ($valuesPlaceholder)";

            $statement = static::$connection->prepare($query);

            $statement->execute($valuesArray);

        } catch (\PDOException $e) {

            die("Insert failed: " . $e->getMessage());

        }


    }

}

Usage example

//setting the connection in app entry point
$config = require_once "config.php";
$dbConnection = Connection::make($config['database']);

DB::setConnection($dbConnection);


//using DB class in a controller
class PostController

{
    //inserting a post to posts table with name and content columns
    public function create() 

    {

        $name = $_POST['postName'];
        $content = $_POST['postContent'];

        DB::insert('posts', [

            'name' => $name,
            'content' => $content

        ]);

        return redirect('somewhere');

    }


}

Answer 1

There are several areas for improvement.

SQL injection.

There is a fair possibility for the SQL injection. You are adding variables (namely array keys) into SQL query absolutely untreated, which is a big red flag.

Although your intended usage scenario is safe, but you cannot foresee any possible use case. There was an infamous SQL injection in Drupal 7 that used a code almost identical to yours - an array key is put into the query untreated. As you can see, it led to a severe vulnerability. Therefore, your function ought to offer a 100% safe query execution, no matter how it's called.

However, the best solution you can get will question the function's existence itself. Because the best way to protect the field names is to whitelist them, but the best way to whitelist table fields is... to list them explicitly in the query. And if you list them in the query, there will be no need for such a function at all. It shouldn't be a problem, however, as you are going to list your field names right in the code this way or another. Given you can use positional placeholders, the code could be reasonably small:

    DB::query('INSERT INTO posts (name, content) VALUES (?,?)', [
        $_POST['postName'], $_POST['postContent']
    ]);

Whereas a function like DB::insert() could be justified not as a free-to-use function, but as method belongs to an ORM, that operates a predefined set of object's properties only. In this case, when it's guaranteed that only predefined field names are allowed to the query, such a function could be used safely.

If you are 100% determined to use this function as is, at least make sure you are quoting and escaping field and table names properly.

Error reporting

The error reporting is completely flawed in this function. It tells everyone that you don't even consider your site to go live. Because present error reporting is only useful for the single-user dev mode, while it would be completely useless (and even harmful) on a live site. I've got a comprehensive guide on PHP error reporting, but in short, you must leave an error/exception alone and let it to bubble up to the site-wide handler, instead of catching it on the spot.

A generic query function.

From the code of your function it is evident that you don't have a generic query function (or you don't reuse its code which is equally bad). There should be a function that accepts a query and an array with parameters, and returns a statement:

public static function query($query, $data = [])
{
    $statement = static::$connection->prepare($query);
    $statement->execute($data);
    return $statement;
}

and then it could be used in your insert() method (as well as in many other methods):

$query = "INSERT INTO $table ($columnsString) VALUES ($valuesPlaceholder)";
static::query($query, $valuesArray);

(but again - remember that $table and values in $columnsString must be either quoted/escaped or - preferably - hardcoded in the class definition).

A proper Model

Your code hints that you are intending to follow the MVC pattern. However, the code in your controller is not reusable, it means that the pattern is broken. Imagine you are going to create a command-line utility to create posts. Or any other method like creating a post through e-mail. You will inevitably duplicate this insert call, which is against the very purpose of the MVC pattern.

So, to make it proper, you must create a Post Service with a create() method in it, whic would be called in your Controller. It will make your Controller thin, and the insert code reusable.

Answer 2

You are using the prepared statement with placeholders properly, so regarding security, I would only advise you to not display the actual error details to the end user -- you don't want naughty people getting any funny ideas.

As for readability, I try not to declare single-use variables, though it can be good to declare a single-use variable to clarify the data being processed.

I don't prefer the over-delimit, then rtrim() technique. Building a temporary array of ? then imploding it with commas seems cleaner to me.

$columns = implode(',', array_keys($data));

$placeholders = implode(',', array_fill(0, count($data), '?'));

$values = array_values($data);

Answer 3

        try {

            $columnsArray = array_keys($data);
            $columnsString = implode(',', $columnsArray);

            $valuesArray = array_values($data);
            $valuesCount = count($valuesArray);

            $valuesPlaceholder = '';
            for ($i=0; $i < $valuesCount; $i++) { 
                $valuesPlaceholder .= '?,';
            }
            $valuesPlaceholder = rtrim($valuesPlaceholder, ',');

None of this code can throw a PDOException. So there's no point in having it in the try block. It's not like it was in a separate function that included all of this function. The scope of the block is fully under your control here.

        $valuesCount = count($data);
        if ($valuesCount < 1) {
            return;
        }

        $columnsArray = array_keys($data);
        $columnsString = implode(',', $columnsArray);

        $valuesPlaceholder = '?';
        for ($i = 1; $i < $valuesCount; $i++) { 
            $valuesPlaceholder .= ',?';
        }

        $valuesArray = array_values($data);

        try {

Now we don't even waste time processing if there is no data. You can change the return to a die if you prefer that behavior (with no data, the SQL would have been malformed and triggered the exception).

Having it count $data rather than the values is just so that we don't have to call array_values if it's not needed. Counting any of $data, the values, or the keys should return the same result. In fact, if one were different, it would break the function's behavior.

I changed the placeholder handling to avoid the rtrim. With the early return, it is no longer necessary. This variant will handle any positive integer properly.