1
\$\begingroup\$

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);
    }


}
\$\endgroup\$
  • \$\begingroup\$ also, no one seemed to notice that your condition is backwards in your query method. should be if(!empty($values)) note the !.. \$\endgroup\$ – I wrestled a bear once. Oct 17 '17 at 15:00
  • 1
    \$\begingroup\$ @Iwrestledabearonce. Thanks, I did notice this a while back and it has been fixed in the github project. I'll actually fix that here too I think as it is just a typo. I may also add current code as an edit to the bottom so people can see how it has evolved. Thanks again! \$\endgroup\$ – Grumpy says Reinstate Monica Oct 17 '17 at 15:06
  • \$\begingroup\$ wow man, new code looks sweet \$\endgroup\$ – I wrestled a bear once. Oct 17 '17 at 15:12
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Does the extend method only work with PHP 7.1? Is that what you were saying? Also, what benefit does that give me vs not extending it? \$\endgroup\$ – Grumpy says Reinstate Monica Oct 13 '17 at 16:53
  • \$\begingroup\$ Also, what does the : PDOStatement and : voiddo? \$\endgroup\$ – Grumpy says Reinstate Monica Oct 13 '17 at 16:54
  • \$\begingroup\$ No, inheritance should work since PHP 5, maybe even 4. The return type and parameter types were introduced with PHP 7.0 and 7.1. If your compiler is complaining, just delete the colon and the type and the types in the parameter lists. On the other hand, if you are using an old PHP version and are creating a new project, think about starting with 7.1 right away. Most hosters should support it now. \$\endgroup\$ – mheinzerling Oct 13 '17 at 16:58
  • \$\begingroup\$ I would like to support slightly older versions of PHP too. I think I'm going to take bits and pieces of your answer and update my code accordingly with all the new information you have given me :) \$\endgroup\$ – Grumpy says Reinstate Monica Oct 13 '17 at 17:01
  • \$\begingroup\$ Hey, do you mind if I contribute some of the work to you on the GitHub page and link to your stackoverflow profile? As I have used many suggestions from your code \$\endgroup\$ – Grumpy says Reinstate Monica Oct 13 '17 at 17:27
1
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I like the way you rewrote the run() function, I'm not sure why I didn't think of that before. Along with many other good points, thank you for your answer! \$\endgroup\$ – Grumpy says Reinstate Monica Oct 16 '17 at 12:56
  • \$\begingroup\$ you may also note the way default options are implemented. you still has the ability to overwrite any of them, making PDO::FETCH_OBJ default fetch mode, or adding other options. \$\endgroup\$ – Your Common Sense Oct 16 '17 at 12:59
  • \$\begingroup\$ Well the question doesn't have current code anymore, you should check out the github project to see the changes I have made, I completely changed the way options work with the class. I wasn't sure if I should update the code in the question with the newest edition of the code, as it can invalidate answers. \$\endgroup\$ – Grumpy says Reinstate Monica Oct 16 '17 at 13:21
  • \$\begingroup\$ Ok, I checked there. Although it's not that tidy ($options is a 2-level nested array) and flexible(you cannot rewrite just one PDO option), but it's usable. \$\endgroup\$ – Your Common Sense Oct 16 '17 at 13:30
  • \$\begingroup\$ Yeah I was trying to think of a way to make it all optional parameters but I ran into an issue where basically if you wanted to pass the attributes you would also have to pass the charset which I don't want. I want them to be optional \$\endgroup\$ – Grumpy says Reinstate Monica Oct 16 '17 at 13:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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