Custom PDO wrapper class

Question

I somewhat recently asked a question on here for advice on a PDO function that I had written. I got some good advice from a user, and went with it. I had used that function for about 3 months which is also about how long I have been using PDO in general, until someone mentioned to me that they would have made it a class.

I have never worked with classes before in PHP, and I wanted to give it a shot. So I did.

Now, since this is my first class, I wanted to get some advice on it and some constructive criticism. I want my code to keep on improving, and I want to know the best way to do some things. I don't know if my class is written well, and knowing how I can improve it will definitely help me out.

I've hosted the project on my GitHub. The project is being developed there, and everyone is welcome to contribute to it.

class GrumpyPdo {

    private $host, $user, $pass, $db, $charset;
    private $dsn, $databaseObject;

    private $killOnError = true;
    private $echoOnError = false;

    //these options can obviously be changed but not "on the fly" (yet).
    private $opt = [
        PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES   => false,
    ];

    function __construct($host, $user, $pass, $db, $charset = "utf8") {

        $this->setData([
            "host" => $host,
            "user" => $user,
            "pass" => $pass,
            "db" => $db,
            "charset" => $charset,
        ]);

        $this->createDBObject();

    }

    function query($query, $values = []) {
        if(!empty($values)) {
            $stmt = $this->databaseObject->prepare($query);
            $stmt->execute($values);
        } else {
            $stmt = $this->databaseObject->query($query);
        }
        return $stmt;
    }

    function setData($data) {
        if(!is_array($data)) {
            $this->handleError("GrumpyPDO: Error Code SD13; The setData method requires an array with \$key => \$value pairs.");
        }
        foreach($data as $k => $v) {
            $this->{$k} = $v;
        }
        return true;
    }

    function createDBObject() {
        $this->dsn = "mysql:host={$this->host};dbname={$this->db};charset={$this->charset}";
        $this->databaseObject = new PDO($this->dsn, $this->user, $this->pass, $this->opt);
    }

    function handleError($error) {
        error_log($error);
        if($this->echoOnError) echo $error;
        if($this->killOnError) die($error);
    }


}

Answer 1

Assuming that there is no other code in the class that you deleted here, I'm not sure why you store all the connection details in the object. I recommend to inline createDBObject into the constructor and drop the now unused fields.

After this inline there is also no usage of your setData method and you can drop it, too. In general I would not work with string identifiers, if you are not forced to. If you rename you field user to username you have to keep in mind the string and fix it to. Not all IDEs will support you here.

The following code will do the same as yours:

<?php

class GrumpyPdo
{
    private $databaseObject;

    private $killOnError = true;
    private $echoOnError = false;

    private $opt = [
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES => false,
    ];

    function __construct($host, $user, $pass, $db, $charset = "utf8")
    {
        $dsn = "mysql:host={$host};dbname={$db};charset={$charset}";
        $this->databaseObject = new PDO($dsn, $user, $pass, $this->opt);
    }

    function query($query, $values = [])
    {
        if (empty($values)) {
            $stmt = $this->databaseObject->prepare($query);
            $stmt->execute($values);
        } else {
            $stmt = $this->databaseObject->query($query);
        }
        return $stmt;
    }

    function handleError($error)
    {
        error_log($error);
        if ($this->echoOnError) echo $error;
        if ($this->killOnError) die($error);
    }
}

Except the fact that your handleError is not used any longer. It is up to you, if you also want to drop it or rebuild your query method to catch PDOException and handle them with your method somehow.

---

From an OOP-point of view, as an slightly different approach, you might want to extend PDO instead of a delegation (with PHP 7.1 strict typing):

<?php

class GrumpyPdo extends \PDO
{
    /**
     * @var bool
     */
    private $killOnError = true;
    /**
     * @var bool
     */
    private $echoOnError = false;

    public function __construct(string $host, string $user, string $pass, string $db, string $charset = "utf8")
    {
        parent::__construct("mysql:host={$host};dbname={$db};charset={$charset}", $user, $pass, [
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_EMULATE_PREPARES => false,
        ]);
    }

    public function run(string $query, array $values = []): PDOStatement
    {
        try {
            if (empty($values)) {
                $stmt = $this->prepare($query);
                $stmt->execute($values);
            } else {
                $stmt = $this->query($query);
            }
        } catch (PDOException $e) {
            $this->handleError($e->getMessage()); // if you die, you will lose the stack trace?
            throw  $e;
        }
        return $stmt;
    }

    private function handleError(string $error): void
    {
        error_log($error);
        if ($this->echoOnError) echo $error;
        if ($this->killOnError) die($error);
    }
}

As stated in the comment, you need to think about your "error handling", if you want to stay with these debug flags. And as a drawback of PHP inheritance in this case, it is required to rename query to something else like run, because you can't override the original PDO method.

I nearly missed one last thing. In OOP you always need to be aware of the visibility of your methods and field. Please check my usage of private and public.

Answer 2

Basically this class is rather good. Most of "my first database classes" fall into more than one category in my Your first database wrapper's mistakes list, but yours falls into just one. Here it is:

A database module should never decide on it's own, whether to die on error or whether to echo the error out. It just makes no sense. There are other modules. If each one would implement its own error reporting, it would be a complete mess.

All your class should be doing is just raising an error. Whereas how it would be processed should be decided elsewhere. which makes handleError() function a wrong one.

Not to mention that it's only used to handle one single error. A function to handle just a single user case is rather strange.

Anyway, like I said, your database class should only raise an error, and for this purpose you could just throw an exception:

        if(!is_array($data)) {
            throw new InvalidArgumentException("GrumpyPDO: Error Code SD13; The setData method requires an array with \$key => \$value pairs.");
        }

Finally, this custom error message is superfluous: if you try to iterate something that is not array, PHP will warn you already:

Warning: Invalid argument supplied for foreach()

which is basically just slightly less verbose version of your custom error message.

Given the useful advise from the other answer, your code could be like this

class MyPDO extends PDO
{
    public function __construct($dsn, $username = NULL, $password = NULL, $options = [])
    {
        $default_options = [
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_EMULATE_PREPARES => false,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        ];
        $options = array_replace($default_options, $options);
        parent::__construct($dsn, $username, $password, $options);
    }
    public function run($sql, $args = NULL)
    {
        if (!$args)
        {
             return $this->query($sql);
        }
        $stmt = $this->prepare($sql);
        $stmt->execute($args);
        return $stmt;
    }
}