4
\$\begingroup\$

I have created a DB connector class with PDO.

Do I need to add, delete or edit anything in this code?

<?php
class Config extends model{

    // The state of the config
    public $configState = array();

    // THE only instance of the class
    private static $instance;

    public function __construct(){}

    /**
     *    Returns THE instance of 'Config'.
     *    The config is automatically initialized if it wasn't.
     *
     *    @return    object
     **/
    public static function getInstance(){
        if ( !isset(self::$instance)){
            self::$instance = new self;
        }
        self::$instance->startConf();
        return self::$instance;
    }

    public function startConf(){
        if (empty($this->configState)){
            $this->resetState();
        }
        return $this;
    }

    /**
     *    Stores datas in the Config.
     *    Example: $instance->foo = 'bar';
     * @param $key
     * @param $val
     */
    public function __set( $key , $val ){
        $this->configState[$key] = $val;
    }

    /**
     * Gets datas from the Config.
     * Example: echo $instance->foo;
     *
     * @param $key
     * @return bool
     */
    public function __get( $key ){
        return (isset($this->configState[$key]))? $this->configState[$key]: false;
    }

    public function __isset( $key ){
        return isset($this->configState[$key]);
    }

    public function __unset( $key ){
        unset( $this->configState[$key] );
    }

    public function destroy(){
        $this->configState = array();
    }

    public function resetState(){
        $this->configState = array();
        $this->configState['show_php_errors'] = true;
        $this->configState['SSL_security'] = true;
        $this->configState['XFO_security'] = true;
        $this->configState['XSS_security'] = true;
        $this->configState['CSRF_security'] = true;
        $this->configState['db_type'] = "mysql";
        $this->configState['db_host'] = "localhost";
        $this->configState['db_port'] = 3306;
        $this->configState['db_name'] = "demo";
        $this->configState['db_user'] = "root";
        $this->configState['db_pass'] = "";
        $this->configState['db_path'] = "";
    }
}
    class PDOConnection{
        private $instance;

        private $dsn;
        private $username;
        private $password;
        private $options = [];

        /**
         * constructor
         *
         * @param $dsn
         * @param $username
         * @param $password
         * @param array $options
         */
        public function __construct($dsn, $username, $password, array $options = []) {
            $this->dsn      = $dsn;
            $this->username = $username;
            $this->password = $password;
            $this->options  = $options;
        }

        /**
         * Setting attributes on instance
         *
         * @param $name
         * @param $value
         * @return mixed|void
         */
        public function setAttribute($name, $value) {
            if(!$this->instance instanceof PDO) {
                throw new LogicException('Cannot set PDO attribute. Please make sure you are connected using the connect() method.');
            }
            if($this->instance->setAttribute($name, $value) === false) {
                throw new LogicException('Could not set PDO attribute: ' . $name);
            }
        }

        /**
         * Setting options
         *
         * @param $name
         * @param $value
         */
        public function setOption($name, $value) {
            $this->options[$name] = $value;
        }

        /**
         * getting connection
         * @return PDO
         */
        public function getConnection() {
            if(!$this->instance instanceof PDO) {
                throw new LogicException('No database connection established.');
            }
            return $this->instance;

        }

        /**
         * connecting to database
         *
         * @throws ErrorException
         */
        public function connect() {
            try {
                $this->instance = new PDO($this->dsn, $this->username, $this->password, $this->options);
            }catch(PDOException $exception) {
                throw new ErrorException('Could not connect to the database!', null, $exception);
            }
        }

        /**
         * disconnecting from database
         */
        public function disconnect() {
            $this->instance = null;
        }
    }

class database{
    private $dbh;
    private $executed = false;
    private $stmt;

    /**
     * constructor
     **/
    public function __construct(){
        $conf = Config::getInstance();
        switch($conf->db_type){
            case "mysql":
                $dsn = "mysql:host=$conf->db_host;port=$conf->db_port;dbname=$conf->db_name";
                break;
            case "sqlite":
                $dsn = "sqlite:$conf->db_path;";
                break;
            case "postgresql":
                $dsn = "pgsql:host=$conf->db_host;port=$conf->db_port;dbname=$conf->db_name";
                break;
            default:
                $dsn = "mysql:host=$conf->db_host;port=$conf->db_port;dbname=$conf->db_name";
        }
        $dsn .= ';charset=utf8';
        $connection = new PDOConnection($dsn, $conf->db_user, $conf->db_pass, array(
            PDO::MYSQL_ATTR_INIT_COMMAND    => 'SET NAMES utf8',
            PDO::ATTR_PERSISTENT            => true,
            PDO::ATTR_TIMEOUT               => 60*60*60*60,
            PDO::ATTR_ERRMODE               => PDO::ERRMODE_EXCEPTION
        ));
        $connection->connect();
        $this->dbh = $connection->getConnection();
    }

    /**
     * Provides access to the application PDO instance.
     *
     * @return \PDO
     */
    public function pdo() {
        return $this->dbh;
    }

    /**
     * set query statement
     *
     * @param $query
     *
     * @return $this
     */
    public function query($query){
        /** @noinspection PhpUndefinedMethodInspection */
        $this->stmt = $this->dbh->prepare($query);
        return $this;
    }

    /**
     * binding database
     *
     * @param      $param
     * @param      $value
     * @param null $type
     *
     * @return $this
     */
    public function bind($param, $value, $type = null){
        if (is_null($type)) {
            switch (true) {
                case is_string($value):
                    $type = PDO::PARAM_STR;
                    break;
                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;
            }
        }
        /** @noinspection PhpUndefinedMethodInspection */
        $this->stmt->bindValue($param, $value, $type);
        return $this;
    }

    /**
     * executing query statement
     *
     * @return $this
     */
    public function execute(){
        /** @noinspection PhpUndefinedMethodInspection */
        $this->stmt->execute();
        $this->executed = true;
        return $this;
    }

    /**
     * fetching all result
     *
     * @param int $fetch
     *
     * @param null $class
     * @param array $args
     * @return mixed
     */
    public function FetchAll($fetch = PDO::FETCH_ASSOC, $class = null, array $args = []){
        $this->execute();
        if(!is_null($class) && in_array($fetch, [PDO::FETCH_CLASS, PDO::FETCH_OBJ])) {
            /** @noinspection PhpUndefinedMethodInspection */
            return $this->stmt->fetchAll(PDO::FETCH_CLASS, $class, $args);
        }
        /** @noinspection PhpUndefinedMethodInspection */
        return $this->stmt->fetchAll($fetch);
    }

    /**
     * fetching first result only
     *
     * @param int $fetch
     *
     * @param null $class
     * @param array $args
     * @return mixed
     */
    public function FetchOne($fetch = PDO::FETCH_ASSOC, $class = null, array $args = []){
        $this->execute();
        if(!is_null($class) && in_array($fetch, [PDO::FETCH_CLASS, PDO::FETCH_OBJ])) {
            /** @noinspection PhpUndefinedMethodInspection */
            return $this->stmt->fetchObject($class, $args);
        }
        /** @noinspection PhpUndefinedMethodInspection */
        return $this->stmt->fetch($fetch);
    }

    /**
     * fetching column
     *
     * @param int $columnNumber
     *
     * @return mixed
     */
    public function FetchColumn($columnNumber=0){
        $this->execute();
        /** @noinspection PhpUndefinedMethodInspection */
        return $this->stmt->fetchColumn($columnNumber);
    }

    /**
     * counting rows
     *
     * @return mixed
     */
    public function rowCount(){
        /** @noinspection PhpUndefinedMethodInspection */
        return $this->stmt->rowCount();
    }

    /**
     * counting columns
     * @return mixed
     */
    public function columnCount(){
        /** @noinspection PhpUndefinedMethodInspection */
        return $this->stmt->columnCount();
    }

    /**
     * getting last inserted ID
     * @return string
     */
    public function lastInsertId(){
        return $this->dbh->lastInsertId();
    }

    /**
     * starting transaction
     *
     * @return bool
     */
    public function beginTransaction(){
        return $this->dbh->beginTransaction();
    }

    /**
     * ending transaction
     * @return bool
     */
    public function endTransaction(){
        /** @noinspection PhpUndefinedMethodInspection */
        return $this->dbh->commit();
    }

    /**
     * transaction savepoint
     *
     * @param $savepoint_name
     *
     * @return $this
     */
    public function TransactionSavepoint($savepoint_name){
        $this->query("SAVEPOINT :savepointname");
        $this->bind(':savepointname',$savepoint_name);
        $this->execute();
        return $this;
    }

    /**
     * canceling transaction
     *
     * @return bool
     */
    public function cancelTransaction(){
        /** @noinspection PhpUndefinedMethodInspection */
        return $this->dbh->rollBack();
    }

    /**
     * debuging dump parameters
     *
     * @return mixed
     */
    public function debugDumpParams(){
        /** @noinspection PhpUndefinedMethodInspection */
        return $this->stmt->debugDumpParams();
    }

    /**
     * Reset the execution flag.
     */
    public function closeCursor() {
        /** @noinspection PhpUndefinedMethodInspection */
        $this->stmt->closeCursor();
        $this->executed = false;
    }
}

and Usage:

$db = new database();
$info = $db->query("SEKECT * FROM `table`")->FetchAll();

I need to be sure that this classes are secure for injections and they are optimized for memory usage; any & all other feedback is also welcome.

\$\endgroup\$
  • \$\begingroup\$ I think you meant to type "SELECT * FROM table" rather than SEKECT \$\endgroup\$ – Phrancis Mar 5 '16 at 16:46
3
\$\begingroup\$

PDOConnection doesn't seem to serve any purpose. The only thing it seems to do is hold an instance of PDO, so why not use PDO itself?

database also only seems to be a wrapper of PDO:

  • query is simply a renamed prepare
  • bind a renamed bindValue
  • execute is execute
  • FetchAll is fetchAll, except that it also executes (same with FetchColumn)
  • beginTransaction is beginTransaction, but endTransaction is commit and cancelTransaction is rollBack.

So you have a wrapper that slightly renames the functions of PDO, adds very minor changes (FetchOne can be fetchObject or fetch), and limits its power (lots of functions are missing).

Additionally, your comments are not nearly as in-depth as the official PDO documentation. The PHP documentation is often not great, but still, for eg fetchAll, I can read what parameters it accepts, what it returns, etc. Yours only says fetching all result. So now I have to look into your actual code (because I can't trust that it does exactly what fetchAll does).

This may easily create confusion. At a minimum, I would stick to the original names (doesn't really matter if yours are better or worse, PDO is quite well established, so most people will be familiar with it).

But really, I'm not sure that it is worth it to have a class like this. Everyone who is newly added to your project will have to get familiar with your not-quite-PDO class. If it's just this one class, that may be acceptable, but if you handle similar situations the same way, and always create thin wrappers that slightly change how the details of something work, introducing new developers to your project will be difficult because they will have to put a lot of work into getting to know your code base, and they may introduce bugs (because they assume that your wrappers will work like the original thing, while you slightly change the behaviour).

Misc

  • be more consistent with your naming. Classes should start with an upper-case character, methods with a lower-case character.
  • I need to be sure that this classes are secure for injections: This really depends on how you use the class. You still provide the possibility to bind parameters, so just make sure that you always make use of that. Never pass any string that contains variable content (user input, values from the database, ...) to query, and you will be fine.
\$\endgroup\$
  • \$\begingroup\$ thank you for your advices. So as I understand this classes are not bad, but it may be better to changes only names? and what you mean in "because I can't trust that it does exactly what fetchAll does"? please tell me concret what change to create better class??? \$\endgroup\$ – maia bardavelidze Mar 5 '16 at 22:42
  • \$\begingroup\$ @maiabardavelidze well, like I said, I don't think that the class is really needed. It doesn't seem like you really add much to the functionality of PDO by creating such a thin wrapper. And I meant that it's not clear from just looking at the methods that your FetchAll does what PDOs fetchAll does (and it doesn't). So I can't just use PDOs documentation. And I can't use yours, because it's not very in-depth. So I have to look at the actual code. \$\endgroup\$ – tim Mar 5 '16 at 23:05
  • \$\begingroup\$ @maiabardavelidze But yes, if you think that you really do need a class such as this (and again, I don't think you do), having the same names as PDO and thoroughly documenting any differences that your methods have from PDOs methods would be a good idea. So eg for FetchAll, you should document that it also calls execute, etc. \$\endgroup\$ – tim Mar 5 '16 at 23:11
  • \$\begingroup\$ in documentation everything is ok. but I need to know what functionality I must add in my class??? P.s. it is usually to use class like this... \$\endgroup\$ – maia bardavelidze Mar 5 '16 at 23:32

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.