2
\$\begingroup\$

I've taken this class and made it my own but I'm curious to know what other think of it? Are there things I'm missing? Anything you can recommend?

class Database {
    private $host = '';
    private $user = '';
    private $pass = '';
    private $dbname = '';

    private $dbh;
    private $error;
    private $stmt;

    public function __construct() {
        $dsn = 'mysql:dbname=' . $this -> dbname . ';host=' . $this -> host . '';
        // Set options
        $options = array(PDO::ATTR_PERSISTENT => true, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION);
        // Create a new PDO instanace
        try {
            $this -> dbh = new PDO($dsn, $this -> user, $this -> pass, $options);
        } catch(PDOException $e) {
            $this->error = $e -> getMessage();
        }
    }

    public function query($query) {
        $this -> stmt = $this -> dbh -> prepare($query);
    }

    public function bind($param, $value, $type = null) {
        if (is_null($type)) {
            switch (true) {
                case is_int($value) :
                    $type = PDO::PARAM_INT;
                    break;
                case is_bool($value) :
                    $type = PDO::PARAM_BOOL;
                    break;
                case is_null($value) :
                    $type = PDO::PARAM_NULL;
                    break;
                default :
                    $type = PDO::PARAM_STR;
            }
        }
        $this -> stmt -> bindValue($param, $value, $type);
    }

    public function execute() {
        return $this -> stmt -> execute();
    }

    public function fetchAll() {
        $this -> execute();
        return $this -> stmt -> fetchAll(PDO::FETCH_ASSOC);
    }

    public function fetch() {
        $this -> execute();
        return $this -> stmt -> fetch(PDO::FETCH_ASSOC);
    }

    public function rowCount() {
        return $this -> stmt -> rowCount();
    }

    public function countAll($arr) {
        return count($arr);
    }

    public function lastInsertId() {
        return $this -> dbh -> lastInsertId();
    }
}
\$\endgroup\$
1
\$\begingroup\$

I have set up something very similar with a couple changes. In __contruct() I assign the credentials to variables, then call an instance to start the PDO connection. I also use try-catch for each PDO statement to make sure that I am not getting any errors.

private $_pdo = null;
private $_host, $_dbname, $_uname, $_pass;
private $_sql, $_result;
private $_preparedStmt;

public function __construct($host, $dbname, $username, $password) {
    // You could run checks here to make sure that...
    // no values or specific values are not equal to ''
    $this->_host = $host;
    $this->_dbname = $dbname;
    $this->_uname = $username;
    $this->_pass = $password;
    $this->startPDO();
}

// ... Later on in code, under Private functions comment tag
private function startPDO() {
    try {
        $this->_pdo = new PDO('mysql:host='.$this->_host.';dbname='.$this->_dbname, $this->_uname, $this->_pass);
        $this->_pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    } catch (PDOException $e) {
        //$this->handleFatalErr($e->getMessage(), 'connection'); // 'die' error handling
        //$this->handleErr($e->getMessage(), 'connection'); // Non-'die' error handling
    }
}

In each function I check the connection, I would recommend something similar. You could either kill the script if there is no connection or you could re-connect using the current variables $this->_host, etc:

public function query($sql) {
    $this->checkConnection();
    $this->_sql = $sql; // This is used for logging and for a getter function
    try {
        $this->_result = $this->_pdo->query($this->_sql);
    } catch (PDOException $e) {
        //$this->handleFatalErr($e->getMessage(), 'connection');
    }
}

private function checkConnection() {
    ($this->getPDO() === null) ? die('There is no connection to the database, please re-establish connection.') : NULL;
        //$this->handleFatalErr(self::ERR_CONNECTION_LOST, 'CONNECTION_LOST');
}

private function getPDO() {
    return $this->_pdo;
}

public function close() {
    (isset($this->_pdo) && $this->_pdo != null) ? $this->_pdo = null : NULL;
}
\$\endgroup\$
0
\$\begingroup\$
    private $host = '';
    private $user = '';
    private $pass = '';
    private $dbname = '';

    // ....

    public function __construct() {
        $dsn = 'mysql:dbname=' . $this -> dbname . ';host=' . $this -> host . '';

        // ...
    }

Two things here:

  • How can I set the connection credentials? The properties are private and I see no setters or parameters to the constructor?

  • Why use PDO if you only use mysql?

And an other thing:

public function bind($param, $value, $type = null) {
    if (is_null($type)) {
        switch (true) {
            case is_int($value) :
                $type = PDO::PARAM_INT;
                break;
            case is_bool($value) :
                $type = PDO::PARAM_BOOL;
                break;
            case is_null($value) :
                $type = PDO::PARAM_NULL;
                break;
            default :
                $type = PDO::PARAM_STR;
        }
    }
    $this -> stmt -> bindValue($param, $value, $type);
}

This can be dangerous. Let's say you have code like this:

$db->bind('param', $_POST['id']);

Your method would detect it's a string, but it should be an integer!

After all your class doesn't bring much value, it mostly forwards calls to PDO.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply. The connection details are entered in the class itself with the private variables. I just took them out. Thanks for the param suggestion. Also, why doesn't this class bring value? It speeds up my process and handles PDO. \$\endgroup\$ – SeanWM Mar 19 '13 at 15:42
  • \$\begingroup\$ @SeanWM Most of the methods just forward the calls to other methods (And passing some constants, BTW you could set the fetch mode in pdo with the following function too: php.net/manual/en/pdostatement.setfetchmode.php). Except the bind() method, which IMO is dangerous to use. It's basicly a wrapper which reduces the functionality (because you can only use it for mysql in your case, and only a certain set of PDO methods) and security. \$\endgroup\$ – MarcDefiant Mar 20 '13 at 19:11
  • \$\begingroup\$ $db->bind('param', $_POST['id']); in my demo I just ran, it does in fact NOT return this as a string for me. It works as expected \$\endgroup\$ – JasonDavis Mar 25 '13 at 23:44
  • \$\begingroup\$ @jasondavis I can not reproduce that. Every $_POST value I pass is treated as string (because it is a string). Did you cast it before passing it to the function? \$\endgroup\$ – MarcDefiant Mar 26 '13 at 6:59
  • \$\begingroup\$ @Mogria I confess I didn't test a REAL post, only simulated so I get what you are saying, perhaps his function could be modified though to detect int \$\endgroup\$ – JasonDavis Mar 26 '13 at 7:03

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.