8
\$\begingroup\$

I'm currently learning about PDO, and trying to connect to the database and CRUD in OOP way, this is a simple code that I use.

index.php

include_once("connection.php");
include_once("model.php");

$con    = new Connection();
$model  = new Model();

$model->insert();
$con->closeConnection();

connection.php

class Connection {

    protected $host = "localhost";
    protected $dbname = "pdo";
    protected $user = "root";
    protected $pass = "";
    protected $DBH;

    function __construct() {

        try {

            $this->DBH = new PDO("mysql:host=$this->host;dbname=$this->dbname", $this->user, $this->pass);
        }
        catch (PDOException $e) {

            echo $e->getMessage();
        }
    }

    public function closeConnection() {

        $this->DBH = null;
    }
}

model.php

class Model extends Connection {

    public function insert() {

        $STH = $this->DBH->prepare("INSERT INTO people (name) VALUES ('Yeaaap')");
        $STH->execute();
    }
}

Am I doing it in the right way?

\$\endgroup\$
  • \$\begingroup\$ you can get detailed information about PDO in my blog post here \$\endgroup\$ – dakshbhatt21 Jun 28 '15 at 14:14
8
\$\begingroup\$

Besides the already mentioned things I like to add a few of my own:

Connection charset You should set the charset of the connection the be the same of your php files (and definition when you use html):

Simply add , array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES 'utf8'") after $this->pass

Coding Style

The naming of class variables shouldn't be all uppercase, so rename $DBH to $dbh.

Additionally I'd advice you the read the Coding Style Guide from the PHP Framework Interop Group which many php framework developers pay attention to lately. http://www.php-fig.org/psr/psr-2/

There it says in Chapter 4.3 that you should put the opening curly brace of function on a new line.

User Input

As soon as some user input gets into your sql statement you should use parameterized queries, for example:

$stmt = $this->dbh->prepare("SELECT * FROM user WHERE passwort_hash = :passwort_hash");

$stmt->bindParam(":passwort_hash",md5($_POST['passwort']));
$stmt->execute();
...

This protects you against SQL-Injections.

\$\endgroup\$
  • 1
    \$\begingroup\$ It's recommended to set the charset in the dsn string nowadays. The PDO::MYSQL_ATTR_INIT_COMMAND can then be left out. Also: you only need it if you're connecting to a MySQL server (obviously) \$\endgroup\$ – Elias Van Ootegem Oct 23 '14 at 17:56
7
\$\begingroup\$

Aside from the point mentioned by Alienari there's a huge mistake in your design.

A Model shouldn't inherit from a Connection.

Usually OOP-CRUD is done in a fundamentally different way:

  1. Create and maintain a Connection
  2. Use said connection to run queries against a database
  3. Take the data from queries and put them into a Model

These three steps are usually done by a "DataAccessObject" (DAO) or similar.
You do something completely different. You say: Model is a kind of Connection and that's wrong.

\$\endgroup\$
  • \$\begingroup\$ The only useful answer! \$\endgroup\$ – hex494D49 Feb 13 '17 at 9:32
3
\$\begingroup\$

First things first: You want to know if you're doing things the right way. To that, the answer would be No, you're not. And here's why I say this:

  • for a kickoff, you say you're learning PDO, and want to use it in an Object Oriented way. But PDO stands for PHP Database Object. It's already Object Oriented. It's API is clean, consistent and the design complies with the S.O.L.I.D. principles quite nicely. It's imposiburu to wrap it, extend it or abstract it and get something better. The only thing that you can do with it is to use it as a base/core for a DBAL (database abstraction layer) or in an ORM. Check this answer of mine for more details
  • I've mentioned the S.O.L.I.D. principles already. Read what they're all about. They're not some abstract in-an-ideal-scenario kink of pattern or design philosophy. They're really common-sense principles to keep in mind and adhere to when writing code. Following them makes sense, it's easy and makes your code more flexible, easier to maintain and more portable
  • Like I've often said in previous reviews: A class should never, ever, control the flow of the application. A class that handles database connectivity should NEVER generate direct output. It's part of the application's inner workings. It should communicate to the code that calls it, not to the client that sent a request to the server. Therefore, your Connection class's methods have to be free of echo or print statements. Methods return or throw, then never echo
  • Just like any ordinary method, a constructor mustn't echo, but because of the job a constructor does, the rules are even more strict: if a constructor returns something (explicitly, or most commonly implicitly), its return value is an instance of the class. If the constructor encounters an error (like in your case: failure to connect to the DB), the class can't be constructed. This normally shouldn't happen, but if it does, you're in uncharted territory. It's an exceptional event, and this an Exception must be thrown. You, however, catch an exception and simply echo the error message. There's no stack trace, no error code, nothing... all you end up with is a client that gets an obscure DB error message, telling him a couple of things: what language is running server-side, what DB you're connecting to and possibly an indication as to how you're connecting and why it didn't work. That's a security issue, by anyones standard
  • Coding style is important. It may not seem that important at first, but trust me, it's worth the effort to conform to the most common PHP coding style. There is a standard, put forth by PHP-FIG. All major players (Zend, Symfony, CakePHP, Apple, ...) adhere to it. So should you.
  • I've covered this (quite extensively) in the answer I linked to in my first point of this list, and it is a bit of a dead-horse but: Scalability. The code you have now does not allow you to easily implement things like: re-use of prepared statements or working with transactions.

I'll post an update in the very near future, actually reviewing your code, but for now this list (and the links) should keep you busy for the time being...


On to the first bit of code, which I'll cover line by line (or almost line by line):

class Connection {

Yes, I have something to say about the first line already. 2 things, actually. The first is coding standards: PHP-FIG states that the opening brackets of class, interface, trait and method definitions go on the next line.
In this case, I'd also add that this class is clearly meant to be extended from, and you shouldn't be able to initialize it. Simply because initializing it would be quite pointless (there's no other methods to do something with the instance). So I'd declare it as an abstract class.

    protected $host = "localhost";
    protected $dbname = "pdo";
    protected $user = "root";
    protected $pass = "";
    protected $DBH;

Declaring properties up front is good. Great, even. And you've specified the sensible protected access modifier. Nice, but why are you assigning connection parameters as defaults? Shouldn't the user be able to determine how he wants to connect to which DB, as what user? And shouldn't he be able to set attributes for the PDO instance? Look at the documentation: the user should be able to control all of the arguments that the PDO constructor accepts. Which is exactly what a constructor is for, which brings us to:

    function __construct() {

        try {

            $this->DBH = new PDO("mysql:host=$this->host;dbname=$this->dbname", $this->user, $this->pass);
        }
        catch (PDOException $e) {

            echo $e->getMessage();
        }
    }

Like I said, the try-catch is evil. If PDO throws an exception, then echoing it won't help. The code that creates the instance of your class knows what arguments it passed, and so you can (and should) assume that that code can deal with the exception better than you can. Therefore: don't catch it. That's not your job. Your constructor should aslo take arguments that let the user connect to the DB he wants, not the one that is determined by the properties.
PS: I'm not going to mention it any more, but coding standards...

Now the last bit:

    public function closeConnection() {

        $this->DBH = null;
    }
}

This looks to be a bit of a pseudo-destructor. There's nothing wrong with the code itself, but really, if you look at how PDO works, you should know that PDO returns resultsets and prepared statements as instances of the PDOStatement class. If you're going to close the connection, you might have some code out there that represents a prepared statement (as a PDOStatement) that might be re-used at a later point. Prepared statements might (depending on the adapter) require PDOStatement::closeCursor to be called, but what's more: if you destroy the DB connection, how would a piece of code that is re-using a prepared statement supposed to know about it?
Honestly, there's no easy solution to this problem: you're either going to have to abstract the entire PDO interface away from the user (like Doctrine does), or you should ditch this method.

In the end, I wouldn't recommend using the kind of code to anyone, but if I were to be given the task to rewrite your code, it'd probably end up looking something like this:

abstract class Connection
{
    /**
     * @var string
     */
    protected $host = "localhost";

    /**
     * @var string
     */
    protected $dbname = "pdo";

    /**
     * @var string
     */
    protected $user = "root";

    /**
     * @var string
     */
    protected $pass = "";

    /**
     * @var PDO
     */
    protected $DBH = null;

    /**
     * @param array $dsnParams
     * @param string $usr
     * @param string $pass = ''
     * @param array $attributes = null
     * @throws \InvalidArgumentException
     */
    function __construct(array $dsnParams, $usr, $pass = '', array $attributes = null)
    {
        $requiredParams = ['type','host']
        foreach ($requiredParams as $key)
        {//these keys must exist
            if (!isset($dsnParams[$key]))
                throw new \InvalidArgumentException(
                    sprintf(
                        '%s key must be set',
                        $key
                    )
                );
        }
        $dsn = $dsnParams['type'].':host='.$dsnParams['host'];
        $optional = ['dbname','charset'];//add more if needed
        foreach ($optional as $key)
            $dsn .= ';'.$key.'='.$dsnParams[$key];//construct dsn string
        $this->DBH = new PDO(
            $dsn,
            $usr,
            $pass,
            $attributes
        );
    }
}

Am I happy with this code? Not really, is it better than what you've posted? IMHO, it is. It has doc-blocks to tell users what property will hold what. It throws exceptions and it allows me to configure the DB connection in the way I want it to. But the problem with that is still the same:

Why would I pass all of the information required to instantiate PDO to this class, instead of creating the instance myself?

\$\endgroup\$
2
\$\begingroup\$

Your code is mostly correct with one minor correction. Because the Connection class is inherited by your Model class there is no need instantiate an object of it. When you instantiate an object of type Model, the object is made up of the member functions and variables of the Connection class (dependent on access modifiers) as well as the Model class.

\$\endgroup\$
2
\$\begingroup\$

In addition to all the other points mentioned:

  • don't store the PHP file that contain database credentials inside the web root, as it could leak the credentials under some circumstances.
  • don't echo exception messages to the end-user, they might contain somewhat sensitive information.
  • don't just swallow exceptions: you catch a PDOException and echo it, but you don't handle it; You still try to insert the data, which will fail uncontrolled. I would just not catch the exception and let the calling code do that, or alternatively, catch it and wrap it in your own custom exception (which then will be handled by the calling code).

When you fixed the issues mentioned in the answers here you should post a follow-up question which contains a bit more code. Right now, I can only guess how Model might work (where does the data come from? how does model store it? how does insert actually work with real data?).

\$\endgroup\$
2
\$\begingroup\$

The trouble I'm foreseeing with this is that every instance of every model will open and maintain its own MySQL connection, and this all happens on a per-request basis. So if 10 people visit a page that calls 10 objects you've just occupied 100/250 connections, 250 being the default max on a fresh MySQL install.

You want things like this to scale in a linear way, not geometrically.

Instead, instantiate a single DB connection and pass that to your models. [Dependency Injection]

Secondly, I would offload calls to things like prepare() and execute() to the Connection object, as that is where you would want to handle related tasks like error handling. eg:

class Connection {
    function query($sql, $params=NULL) {
        if( ! $sth = $this->dbh->prepare($sql) ) {
            $err_arr = $this->dbh->errorInfo();
            $err_msg = sprintf("SQLSTATE ERR: %s<br />\nmySQL ERR: %s<br />\nMessage: %s<br />\n", $err_arr[0], $err_arr[1], $err_arr[2]);
            throw new Exception($err_msg);
        }
        if( ! $sth->execute($params) ) {
            $err_arr = $sth->errorInfo();
            $err_msg = sprintf("SQLSTATE ERR: %s<br />\nmySQL ERR: %s<br />\nMessage: %s<br />\n", $err_arr[0], $err_arr[1], $err_arr[2]);
            throw new Exception($err_msg);
        }
        return $sth->fetchAll(PDO::FETCH_ASSOC);
    }
}

class Model {
    function __construct($conn) {
        $this->conn = $conn;
    }
    function insert() {
        $sql = "INSERT INTO foo (bar, baz) VALUES (:id, :value)";
        $this->conn->query($sql, array(
            'id' => $this->id,
            'value' => $this->value));
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why only one connection? Create as many as you need, and inject them when and where you need them. You're not bound to a single connection \$\endgroup\$ – Elias Van Ootegem Oct 23 '14 at 17:52
  • \$\begingroup\$ @EliasVanOotegem well not specifically one and one only, but the minimal amount. One-per-object is most definitely too much. \$\endgroup\$ – Sammitch Oct 23 '14 at 17:56
  • \$\begingroup\$ I just want to add that most cases where one-dbh-per-application causes problems are a use-case for transactions rather than an overabundance of independant connections. \$\endgroup\$ – Sammitch Oct 23 '14 at 18:24
  • \$\begingroup\$ There's no way of knowing how many the OP might require at any given point. If he decides to re-use prepared statements, with the code as it is, these PDOStatements are to be managed per object, and he'll need one connection per instance. And yes, when you throw in transactions and (god forbid) half-arsed threading using PHP's pthreads, you're really descending into the circles of hell \$\endgroup\$ – Elias Van Ootegem Oct 23 '14 at 18:26

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.