PHP MySQL Database class

Question

This is a PHP database class. Yes, I know it's using the MySQL functions, which are deprecated, but I shall be updating it to MySQLi soon. Can you please review this code and give any comment on any improvements or changes you think I should make?

<?php

namespace Revolution;
if(!defined('IN_INDEX')) { die('Sorry, you cannot access this file.'); }
class engine
{
    private $initiated;
    private $connected;

    private $connection;

    final public function __construct() 
    { 
        $this->Initiate(); 
    }

    final public function Initiate()
    {
        global $_CONFIG;
        if(!$this->initiated)
        {
            $this->setMySQL('connect', mysql_connect);
            $this->setMySQL('pconnect', mysql_pconnect);
            $this->setMysql('select_db', mysql_select_db);
            $this->setMySQL('query', mysql_query);
            $this->setMySQL('num_rows', mysql_num_rows);
            $this->setMySQL('fetch_assoc', mysql_fetch_assoc);
            $this->setMySQL('fetch_array',mysql_fetch_array);   
            $this->setMySQL('result', mysql_result);
            $this->setMySQL('free_result', mysql_free_result);
            $this->setMySQL('escape_string', mysql_real_escape_string);

            $this->initiated = true;

            $this->connect($_CONFIG['mysql']['connection_type']);
        } 
    }

final public function setMySQL($key, $value)
{ 
    $this->mysql[$key] = $value;
}


/*-------------------------------Manage Connection-------------------------------------*/

final public function connect($type)
{
    global $core, $_CONFIG;
    if(!$this->connected)
    {
        $this->connection = $this->mysql[$type]($_CONFIG['mysql']['hostname'], $_CONFIG['mysql']['username'], $_CONFIG['mysql']['password']);

        if($this->connection)
        {
            $mydatabase = $this->mysql['select_db']($_CONFIG['mysql']['database'], $this->connection);

            if($mydatabase)
            {
                $this->connected = true;    
            }
            else
            {
                $core->systemError('MySQL Engine', 'MySQL could not connect to database');
            }
        }
        else
        {
            $core->systemError('MySQL Engine', 'MySQL could not connect to host');          
        }
    }
}

final public function disconnect()
{
    global $core;
    if($this->connected)
    {
        if($this->mysql['close'])
        {
            $this->connected = false;
        }
        else
        {
            $core->systemError('MySQL Engine', 'MySQL could not disconnect.');
        }
    }
}

/*-------------------------------Secure MySQL variables-------------------------------------*/

final public function secure($var)
{
    return $this->mysql['escape_string'](stripslashes(htmlspecialchars($var)));
}

/*-------------------------------Manage MySQL queries-------------------------------------*/

final public function query($sql)
{
    return $this->mysql['query']($sql, $this->connection) or die(mysql_error());
}

final public function num_rows($sql)
{
    return $this->mysql['num_rows']($this->mysql['query']($sql, $this->connection));
}

final public function result($sql)
{
    return $this->mysql['result']($this->mysql['query']($sql, $this->connection), 0);
}

final public function free_result($sql)
{
    return $this->mysql['free_result']($sql);
}

final public function fetch_array($sql)
{
    $query = $this->mysql['query']($sql, $this->connection);

    $data = array();

    while($row = $this->mysql['fetch_array']($query))
    {
        $data[] = $row;
    }

    return $data;
}

final public function fetch_assoc($sql)
{
    return $this->mysql['fetch_assoc']($this->mysql['query']($sql, $this->connection));
}




}
?>

Answer 1

namespace Revolution;
class engine

I do not understand these names. I would have expected to see something database related. Neither Revolution nor engine says database to me.

final public function __construct() 
{ 
    $this->Initiate(); 
}

final public function Initiate()
{
    global $_CONFIG;
    if(!$this->initiated)
    {
        $this->setMySQL('connect', mysql_connect);
        $this->setMySQL('pconnect', mysql_pconnect);
        $this->setMysql('select_db', mysql_select_db);
        $this->setMySQL('query', mysql_query);
        $this->setMySQL('num_rows', mysql_num_rows);
        $this->setMySQL('fetch_assoc', mysql_fetch_assoc);
        $this->setMySQL('fetch_array',mysql_fetch_array);   
        $this->setMySQL('result', mysql_result);
        $this->setMySQL('free_result', mysql_free_result);
        $this->setMySQL('escape_string', mysql_real_escape_string);

        $this->initiated = true;

        $this->connect($_CONFIG['mysql']['connection_type']);
    } 
}

As a general rule, you don't want to do your database connection in your constructor. You should be able to create an instance without having it connected to a database. What you might consider doing is creating a fetchConnectedInstance that runs the constructor and initializes it, returning the result. You should pass $_CONFIG into this function and Instance. That way you can connect to more than one database at once. Your version is almost a Singleton but not quite.

I don't see the point in things like

        $this->setMysql('select_db', mysql_select_db);
        $mydatabase = $this->mysql['select_db']($_CONFIG['mysql']['database'], $this->connection);

Note that $this->mysql['select_db'] is longer than mysql_select_db, so it's clear that this is not a convenience abbreviation. What is the benefit? It almost seems like you are trying to make a base class that you can reuse for other database types. Unfortunately, I don't know that you can rely on each database offering the same abilities. For example, a key/value database is not going to offer some of the capabilities that a relational database will.

If you are going to alias something, then there should be a reason. If there's a reason, you should comment on it so that the reader knows it. As a general rule, if you are clever, explain to the reader how it's clever. That way, the reader doesn't need to be clever just to read the code.

I don't like the use of $this->initiated. It doesn't seem necessary. Either don't care about that situation, or check something more direct like the existence of $this->mysql.

/*-------------------------------Secure MySQL variables-------------------------------------*/

final public function secure($var)
{
    return $this->mysql['escape_string'](stripslashes(htmlspecialchars($var)));
}

This is not the right way to handle this. The htmlspecialchars function should not be called before putting something in the database. It should be called after fetching something from the database and before outputting it in HTML. It should be the last thing called before outputting. Doing it this way risks someone finding a special input that will evade htmlspecialchars and then be enabled by the stripslashes/mysql_real_escape_string. Also, htmlspecialchars can change the data if used at this point. What if you want to store HTML in the database? E.g. user entered content.

You also aren't using bind parameters at all. In general, bind parameters are considered a safer way of handling things than using mysql_real_escape_string.

    if($this->mysql['close'])

What this does is check if $this->mysql['close'] has a true or false evaluation. You may have meant to do at least

    if ($this->mysql['close']())

However, that's still incorrect. That closes the last database connection that was open. That may or may not be the right connection. It really should be

    if ($this->mysql['close']($this->connection))

In the following two function signatures, you use $sql to represent two different things:

final public function query($sql)
final public function num_rows($sql)

In the first, $sql is a string representing a MySQL query (i.e. a SQL string). In the second, it is a query result handle. I find that $sql makes a lot of sense in the first signature and no sense in the second. I would suggest changing the second to something like

final public function num_rows($query)

You also have a function that doesn't quite do what one would expect:

final public function fetch_assoc($sql)
{
    return $this->mysql['fetch_assoc']($this->mysql['query']($sql, $this->connection));
}

What you do here is run a query based on the $sql string and then return the first row of the result. However, you now have no way to get subsequent rows, as you throw away the query result handle. You take care of this situation with a while in

final public function fetch_array($sql)
{
    $query = $this->mysql['query']($sql, $this->connection);

    $data = array();

    while($row = $this->mysql['fetch_array']($query))
    {
        $data[] = $row;
    }

    return $data;
}

This will usually work but can cause problems if the number of rows is large. You are essentially reading the entire result into memory and then processing it later. If there's a relatively small amount of data, this is fine. However, if there is a large amount of data, then you can overload the server if you have this happen in too many results at once. For this reason, it is more common to allow the caller to take care of the while loop portion. The caller can either save the data or use and discard the results immediately.

I don't want to say a lot about this, since you already noted it. It's worth repeating though. You would generally be better off using either mysqli or PDO to do this. You might even find that they offer enough that you don't need an intervening class to manage the connection.