1
\$\begingroup\$

This is a PHP script I wrote it back in 2015 and have been updating it since.

It is a MySQLI wrapper called SqlObject (class) that was originally created to be used by multiple different calling functions so the same SQL connection would be used, unlike some other wrappers (or code) that opens a connection for each query.

The idea is simply, any SQL query that you are expecting a row as a result (or a variable) you call the getData($query, $mode) function where $query is the SQL string and $mode is a const: SqlObject::SINGLE_ROW or SqlObject::MULTI_ROW where SINGLE_ROW returns an associative array as a row from the query and MULTI_ROW returns an array of associative arrays (rows).

Any SQL query that you are not expecting a row from like INSERT, UPDATE, etc. you use the setData($query) method. setData returns TRUE or FALSE if the query was successful.

Error Handling

SqlObject has two kinds of error handling: hard exceptions where code has failed such as a broken connection and switched error handling. Upon a MySQLi error such as a bad query or duplicate entry, the error number is passed into a switch where the error can be handled appropriately. For example, if there is a DEADLOCK error when trying to setData, SqlObject will retry 5 more times before returning FALSE.

Wrapper Helpers

There are many helper functions that make general MySQL development less hellish.

insertQuery($table, $values)
updateQuery($table, $values)
isSqlSafe($str)
sanitizeForQuery($str, $blacklist = NULL, $whitelist = NULL)
escapeSpecialChars($str)
  • insertQuery - Returns an SQL query string for a table name from an associative array of $value -> $key pairs.
  • updateQuery - same as InsertQuery but is for the UPDATE command. It is up to the programmer to append the WHERE clause (if any) to the result of this before using it.
  • isSqlSafe - Lightly scans a string for bogus SQL injection attempts. Returns TRUE or FALSE.
  • sanitizeForQuery - Scans a string for potentially dangerous text that can lead to code injection. It takes a string as the first argument to scan, an optional array of strings to be appended to the blacklisted strings within the function, and an optional array of strings that are to be whitelisted from the default blacklist within the function.
  • escapeSpecialChars - Escapes and cleans up user input (or any) text that is to be included in an SQL query. This is NOT a safeguard from injections, it only ensures that typical characters that should be escaped are done so properly.

I would appreciate any feedback from the community.

Here is a list of all of the functions:

public function setData($query)
public function getData($query, $returnType = self::SINGLE_ROW)
public function get_table_names($filter = NULL)
function get_last_arraysize()
public function get_affected_rows()
public function get_last_insert_id()
public function get_last_query()
public function get_error_number()
public function get_error()
public function get_database_name()
public function get_username()
public function change_database($new_database, $username = NULL, $password = NULL)
public function terminate()
public static function insertQuery($table, $values)
public static function updateQuery($table, $values)
public static function isSqlSafe($str)
public static function sanitizeForQuery($str, $blacklist = NULL, $whitelist = NULL)
public static function escapeSpecialChars($str)
private function retry_deadlock($query)

Here is an example of the usage (this is a hypothetical use case):

$sql = new SqlObject();

//Reuse the $sql object for both functions
getNames($sql);
getOccupation('John Doe', $sql);

function getNames($sql = NULL){
    if($sql == NULL) { $sql = new SqlObject(); }

    $query = "SELECT `ID`,`name` FROM `table` WHERE 1";
    $result = $sql->getData($query, SqlObject::MULTI_ROW);

    $numRows = $sql->get_last_arraysize();
    for($i = 0; $i < $numRows; $i++){
        printf("Name (ID=%d): %s\n", $result[$i]['ID'], $result[$i]['name']);
    }
}

function getOccupation($name, $sql = NULL){
    if($sql == NULL) { $sql = new SqlObject(); }

    if(!SqlObject::is_sql_safe($name)){
        throw new Exception("Unsafe user input");
        return FALSE;
    }

    $query = "SELECT `occupation` FROM `jobs` WHERE `name` LIKE '$name'";
    $result = $sql->getData($query, SqlObject::SINGLE_ROW);     //Single row (assoc)

    if($result !== NULL){
        printf("Name: %s Occupation: %s\n", $result['occupation']);
    }
}

SqlObject.php

<?php
/*
SqlObject.php - A SQL php helper module.
Copyright (c) Jacob Psimos
All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are
met:

* The original author's name (Jacob A Psimos) remain in this file or any
    refactored versions.

* The original author's name (Jacob A Psimos) be included in the acknowledgements
    of the application using this library.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/


class SqlObject{


    /* CONFIGURABLE VARIABLES - hard code these or use object contructor every time */
    private $SERVER_ADDRESS = 'localhost';
    private $SERVER_PORT = 3306;
    private $DATABASE_NAME = '';
    private $USERNAME = '';
    private $PASSWORD = '';
    private $MYSQLI_CHARSET = 'utf8'; //change for a custom charset

    /* PRIVATE GLOBAL VARIABLES */
    private $connection = NULL;
    private $last_count = 0;
    private $last_query = '';
    const SINGLE_ROW = 0;
    const MULTI_ROW = 1;
    const ERR_DUPLICATE = 1062;
    const ERR_DEADLOCK = 1213;
    const RETRY_DEADLOCK_ATTEMPTS = 3;
    const ACCESS_DENIED_CHANGE_USER = 1873;
    const ACCESS_DENIED_DATABASE = 1044;
    const ACCESS_DENIED_USER = 1045;


    /*
        If no username and no password and no database is provided,
        it will attempt to open a connection using the variables in the CONFIGURABLE section
        above.  If your scripts will be using the same username and password and database most of the time,
        just hard code the variables above so you can just call:
            $sql = new SqlObject() and it will open without filling out the constructor every time
    */
    public function __construct($username = NULL, $password = NULL, $database = NULL){
        if($username != NULL && $password != NULL && $database != NULL){
            $this->USERNAME = $username;
            $this->PASSWORD = $password;
            $this->DATABASE = $database;
        }

        $this->connection = new mysqli($this->SERVER_ADDRESS, $this->USERNAME, $this->PASSWORD, $this->DATABASE_NAME, $this->SERVER_PORT);

        if($this->connection->connect_error){
            throw new Exception("SqlObject::construct() Connection error: " . $this->connection->connect_error);
        }else{
            mysqli_set_charset($this->connection, $this->MYSQLI_CHARSET);
            if($this->connection->error){
                throw new Exception("SqlObject::construct() Charset Error: " . $this->connection->error);
            }
        }
    }

    /*
        Use this function when a query is INSERT, UPDATE, etc
        where you know the SQL query is not going to return a row
    */
    public function setData($query){
        if($this->connection){
            $result = $this->connection->query($query);
            $this->last_query = $query;
            if($result == FALSE){
                /* How to handle certain SQL errors */
                switch($this->connection->errno){
                    case self::ERR_DEADLOCK:
                        return $this->retry_deadlock($query);
                    break;
                    case self::ERR_DUPLICATE:
                        return FALSE;   
                    break;
                    case self::ACCESS_DENIED_CHANGE_USER:
                        throw new Exception($this->connection->error);
                    break;
                    case self::ACCESS_DENIED_USER:
                        throw new Exception($this->connection->error);
                    break;
                    case self::ACCESS_DENIED_DATABASE:
                        throw new Exception($this->connection->error);
                    break;
                    default:
                        //file_put_contents('debug.txt', $this->connection->errno);
                        throw new Exception($this->connection->error);
                    break;  
                }
            }   //end error catching switch
            return TRUE;    //Successful
        }
        return FALSE;   //normally not reached
    }

    /*
        This this to retrieve rows using an SQL query.

        When returnType is SINGLE_ROW, only one row is returned even if there are many.
        When returnType is MULTI_ROW, an array of rows is returned.

        NULL is returned if the query returns nothing.
    */
    public function getData($query, $returnType = self::SINGLE_ROW){

        if(!$this->connection){
            throw new Exception('The Database connection is broken' . "\n" . $query);   
        }

        /* Run the provided query, if the query is invalid the return will be FALSE */
        $datum = $this->connection->query($query);
        $this->last_query = $query;

        if(!$datum){
            throw new Exception("SqlObject::getData() Error: " . $this->connection->error);
        }

        /* Single row (0) returns an associative array of the selected row */
        /* Multi row (1) returns an array of associative array(s) from the query */
        switch($returnType){
            case self::SINGLE_ROW:
                $result = $datum->fetch_assoc();
                $datum->free();
                return $result;
            break;
            case self::MULTI_ROW:
                $this->last_count = 0;
                $returnData = array();
                while($nextRow = $datum->fetch_assoc()){
                    array_push($returnData, $nextRow);
                    $this->last_count++;
                }
                $datum->free();
                if($this->last_count > 0){
                    return $returnData;
                }
            break;
            default:
                throw new Exception("Invalid argument for setData(); expected returnType is invalid");
            break;
        }
        return NULL;
    }


    /*
        Returns an array of strings containing all of the table names in the current database
    */
    public function get_table_names($filter = NULL){
        $query = $filter != NULL ? "SHOW TABLES LIKE '$filter'" : 'SHOW TABLES';
        $datum = $this->connection->query($query);

        //$this->last_query = $query;
        if(!$datum){
            throw new Exception($this->connection->error);  
        }

        $names = array();
        while($nextRow = $datum->fetch_array(MYSQLI_NUM)){
            if(count($datum) > 0){
                array_push($names, $nextRow[0]);
            }
        }
        $datum->free();
        return $names;
    }

    /* gets the size of the last returned array from getData */
    function get_last_arraysize(){
        return $this->last_count;
    }

    /* gets the number of rows affected by the last query */
    public function get_affected_rows(){
        return $this->connection->affected_rows;
    }

    /* returns the ID of the last INSERT query 
    NOTE this does not return the ID from an UPDATE query */
    public function get_last_insert_id(){
        return $this->connection->insert_id;
    }

    /* returns the last query that was executed */
    public function get_last_query(){
        return $this->last_query;   
    }

    /* pass back the sql error number */
    public function get_error_number(){
        return $this->connection->errno;    
    }

    /* return the sql error message */
    public function get_error(){
        return $this->connection->error;    
    }

    /* get the current database name */
    public function get_database_name(){
        return $this->DATABASE_NAME;    
    }

    /* get the current username */
    public function get_username(){
        return $this->USERNAME;
    }

    /* attempt to connect to a new database with optional different credentials */
    public function change_database($new_database, $username = NULL, $password = NULL){
        if(!$this->connection){
            throw new Exception("change_database($new_database) failed because of a broken connection");    
        }
        if($this->connection->change_user($username != NULL ? $username : $this->USERNAME,
                            $password != NULL ? $password : $this->PASSWORD, $new_database)){
            $this->DATABASE_NAME = $new_database;
            $this->USERNAME = $username != NULL ? $username : $this->USERNAME;
            $this->PASSWORD = $password != NULL ? $password : $this->PASSWORD;
        }else{
            throw new Exception($this->get_error());    
        }
    }

    /* end the sql connection */
    public function terminate(){
        if($this->connection){
            $this->connection->close();
            unset($this->connection);
        }
    }

    /*
        Returns an SQL query string given a table name and an associative array
        of key -> value items to be included in the query
    */
    public static function insertQuery($table, $values){
        $query = "INSERT INTO `$table`(";
        $firstpart = '';
        $lastpart = '';
        $len = count($values);
        $i = 0;
        foreach($values as $key => $value){
            $firstpart .= ($i < $len - 1) ? "`$key`," : "`$key`) VALUES(";
            if(is_numeric($value) && $value[0] != '+'){
                $lastpart .= ($i < $len - 1) ? "$value," : "$value)";
            }else{
                $lastpart .= ($i < $len - 1) ? "'$value'," : "'$value')";
            }
            $i++;
        }
        return ($query . $firstpart . $lastpart);
    }

    /*
        Returns an SQL UPDATE query string given a table name
        and an associative array of key -> value pairs for the update parameters.

        It is up to the programmer to append the WHERE clause (if needed) to the returned string
    */
    public static function updateQuery($table, $values){
        $query = "UPDATE `$table` SET ";
        $num = count($values);
        $i = 0;
        foreach($values as $key => $value){
            if(is_numeric($value) && $value[0] != '+'){
                $query .= "`$key`=$value";
            }else{
                $query .= "`$key`='$value'";
            }
            if($i < $num - 1){
                $query .= ', ';
            }
            $i += 1;
        }
        return $query;
    }

    /*
        A light scan of a string for common SQL injection commands.
        returns true or false if the string contains a commands

        *** This function does not detect all types of SQL injection ***
        *** Combine this with sanitizeForQuery() for stronger protection ***
    */
    public static function isSqlSafe($str){
        $select_safe = stripos($str, "SELECT `") === FALSE && stripos($str, "SELECT *") === FALSE;
        $insert_safe = stripos($str, "INSERT INTO `") === FALSE && stripos($str, "INSERT INTO *") === FALSE;
        $update_safe = stripos($str, "UPDATE `") === FALSE && stripos($str, "UPDATE *") === FALSE;
        $join_safe = stripos($str, "JOIN `") === FALSE && stripos($str, "INNER JOIN `") === FALSE;
        return $select_safe && $insert_safe && $update_safe && $join_safe && $bogus_quote;
    }

    /*
        Takes a string containing possibly harmful SQL text and characters and
        returns a sanitized version with those items removed.

        $blacklist - an array of strings that should be included in the sanitization default blacklisted
        $whitelist - an array of strings that should be whitelisted
    */
    public static function sanitizeForQuery($str, $blacklist = NULL, $whitelist = NULL){
    //These phraises are to be removed from the resulting value
        $blacklist1 = array(
            '<?php',
            '?>',
            '\x',
        );

        $str = SqlObject::escapeSpecialChars($str);

        /* Loop through all of the blacklisted phraises and remove them */
        foreach($blacklist1 as $blockedchar){
            if($whitelist !== NULL){
                if(!in_array($blockedchar, $whitelist)){
                    $temp = str_replace($blockedchar, '', $str);
                    unset($str);
                    $str = $temp;
                }
            }else{
                $temp = str_replace($blockedchar, '', $str);
                unset($str);
                $str = $temp;
            }
        }

        if($blacklist !== NULL){
            foreach($blacklist as $filter){
                $temp = str_replace($filter, '', $str);
                unset($str);
                $str = $temp;
            }
        }

        return $str;
    }

    /*
        Escape single quote, double quote, percent, underline (_)
        also replaces multiple consecutive \\ slashes with a \ single one
        so escapes dont get messed up
    */
    public static function escapeSpecialChars($str){
        $temp = preg_replace("/\\'+/", "\'", $str);
        $temp = preg_replace('/\"+/', '\"', $temp);
        $temp = preg_replace('/\%+/', '\%', $temp);
        $temp = preg_replace('/\_+/', '\_', $temp);
        return  preg_replace('/\\+/', '\\', $temp);
    }

    /* automatically close the connection when the object is disposed */
    public function __destruct(){
        if(isset($this->connection)){
            $this->connection->close();
            unset($this->connection);
        }
    }

    /* reattempt to run the query that caused a deadlock */
    private function retry_deadlock($query){
        for($i = 0; $i < self::RETRY_DEADLOCK_ATTEMPTS; $i++){
            $result = $this->connection->query($query);
            if($result){
                return TRUE;
            }
            sleep(1);
        }
        return FALSE;
    }

} /* end class */


?>
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Some thoughts below


This class is trying to do too much.

With this class you are attempting to:

  • store database connection (i.e. MySQLi object)
  • generate exceptions around DB connection issues
  • expose many methods that are already available on native MySQLi object.(Not sure about what value these add).
  • provide (faulty) logic to determine if a query string is "safe"
  • provide logic to sanitize/escape strings
  • provide queury string building capabilities (in some case but not in others?)
  • provide some sort of query re-try logic

You seem to be be taking a half-hearted approach to dependency injection when you should be committing fully to it. I will refer to your example code of:

function getNames($sql = NULL){
    if($sql == NULL) { $sql = new SqlObject(); }

If you were properly implementing dependency injection this should be:

function getNames(SqlObject $sql) {

Here you commit to passing a valid SqlObject dependency to the code that needs it. If this contract is not met, an exception will be thrown. This also alleviates the need to do any checking within the subsequent code to validate the dependency, instantiate the dependency if it doesn't exist, etc. Here the getName() function is made to expect the hard dependency and not even be able to execute without it.


You are totally ignoring the use of prepared statements. In this day and age, working with a database without extensively using prepared statements, should be a non-starter. For this reason alone, I would rethink your need for this class. Using prepared statements for insert/update operations would totally eliminate your need to have logic around preventing SQL injection.


Once you commit to moving towards prepared statements (i.e. you are refactoring your overall application), you might also think about dropping working with mysqli altogether in favor of PDO or other better database abstraction. PDO for example can be made to work in "exception mode" such that you don't need to spend your time introducing exception throwing/handling code on top of mysqli.


Why are you rolling your own logic for escaping strings for insertion into the database? mysqli already provides this.


Why are you, in some cases, passing a fully formed query to this class while at other times building the query within the class? Personally, I would rather see the query strings themselves housed in the logic that "owns" the model representation for your data, not hidden away in some central DB class. Does this mean that you end up potentially writing more SQL in your application code? Yes, it does. Should you consider this a problem? Probably not. This makes it easier for a developer working with a particular object model to see all of the SQL logic associated with that model in one place.


Your usage examples here are a bit trivial and using a functional approach to working with the data in the database vs. an object-oriented model approach. I would think that you need to be aligned in your approach for how you are developing your application.


Are you getting any value from this class vs. just directly working with mysqli? You are actually restricting the some of the capability of mysqli class from the calling code and perhaps encouraging bad coding behaviors (like retrieving all rows at once, which is rarely a good idea). I would prefer that, for each use case, the developer give specific thought around:

  • how they formulate their SQL
  • how they need to handle errors or unexpected results (including deadlocks)
  • what sort of data structures (i.e. objects vs. associative arrays) they want to read their data into
  • do they need transactions or not
  • etc.

rather than having some inflexible centralized approach being enforced via a wrapper class. Ideally the logic for each database access is co-located together in one place (i.e. methods on a model class) vs. being spread across a number of classes, obfuscating the logic in the process.

For example, I have rewritten your getName() example using only an injected mysqli object

function getNames(mysqli $mysqli){
    $query = "SELECT `ID`,`name` FROM `table` WHERE 1";
    $result = $mysqli->query($query);
    if($result === false) {
        // log error and return or throw exception
    }
    while($row = $mysqli->fetch_object()) {
        printf("Name (ID=%d): %s\n", $row->ID, $row->name);
    }
}

This code is no less concise/understandable/maintainable than it was when using your wrapper, so again, what value is the wrapper adding?


I would encourage you to use meaningful and specific variable/class/method names. For example, SqlObject is not really a meaningful name. Object is not meaningful (and redundant) as one can obviously see this class is meant to be used in a concrete object context and Sql is not very specific and tells the reader nothing about what this class does. Since this class is only designed to work with mysqli, I would think that, at a minimum, mysqli should be in the class name. I have a hard time thinking of what you might call this class based on the fact that it does too many different things, but even MysqliHelper would be more specific (but defaulting to a name like "helper" should be a red flag that the class is doing too much).


Don't hard code database credentials into your classes as is suggested in your comment. This should be derived from configuration, not in your code. Ideally, these shouldn't be a part of your code base at all, but rather derived from environmental configuration.


I like the fact that you seem to want to provide good comments to your code. I would suggest you take it to the next next and actually use proper PHPDoc comments. Working with your class in an appropriate IDE will become even easier with proper doc blocks in place.


Your public methods are very fragile, as you are not really validating the parameters that are passed (either through type-hinting or in-method validation). There are many cases where one can get to actually making the (relatively expensive) database call even when bad data is passed. Your code should fail early and fail loudly when dependencies or expectations around data types/values are not met.

This again is part of the problem in working with wrappers. You now have to introduce validation logic in your public methods vs. just having all that logic for working with the database in one place. Take my getNames() code example above. Here you can clearly see that there is no way for a non-empty string to be used for the query, but when you introduce this extra layer of a wrapper, from the viewpoint of the wrapper, you don't know where all these methods will be called, so in order to not have a fragile class or method, you need to add validation. You have added complexity to your application without adding any value.


$this->connection = new mysqli($this->SERVER_ADDRESS, $this->USERNAME, $this->PASSWORD, $this->DATABASE_NAME, $this->SERVER_PORT);

This should be the last line in your constructor, after you have validated the connection (and throw exception in cases of errors).


I don't see need for if($this->connection) checks in instance methods. You do a good job of throwing exceptions in constructor such that a concrete SqlObject cannot be instantiated unless this is true, so this code is just redundant.

Another related note is that, although you do a relatively good job of exiting early from your functions in exceptional cases (a very good practice), there are cases like in your setData() method where you have pretty much all the method code nested in an if conditional when early exit from function would eliminate the need for the conditional altogether. If you decide to keep the if($this->connection) check this check, a failed check should throw an exception. Try to be consistent on how you approach this in your code.


I would think all of your static functions could/should be eliminated in favor of using prepared statements, native functions, and taking the approach of actually just writing the SQL in model logic code rather than relying on inflexible query builder logic.


Be careful with applying database change logic on a single shared connection. In doing so, you probably need to add "change database" logic everywhere you use this class as you have now introduced uncertainty as to what the state (in terms of which database is in use) of this shared connection is at any point in your overall code execution. You have no way enforce that a particular database is used for a particular use case, without explicit selecting database for each case where this code is referenced.

I typically like to go with the approach of spawning a whole new connection for each database that may be needed by an application (this of course assuming your application is architected in such a way that you don't need to absolutely minimize the number of DB connections). You would then inject the appropriate connection to the appropriate consuming code.


I don't think database retry logic should live in this one central place. To me that is logic that should reside with any given model in that there may be some cases where a retry is needed and some cases where it is not.

I would also favor other methods to prevent/resolve deadlocks rather than just blind retry. If your table gets into a bad state, just blindly retrying queries over and over could actually exacerbate the problem.


Stylistically, I have some comments:

  • Don't use all upper case for non-constant properties.
  • Try not to mix camelCase and snake_case within any given class/library.
  • Spacing around parenthesis and brackets seems a little non-standard. Of course if this adheres to some agreed upon styleguide, you can ignore my comment, but I would suggest looking at PHP PSR styleguides and adopting those in absence of any other style guide.
\$\endgroup\$
1
  • \$\begingroup\$ Wow. Thank you for your meaningful response! You have taught me that concrete objects as type can be passed as args beyond Integer etc. I use this class because I enjoy having to make up my queries manually and the static methods are just helpers if I see fit. I usually scan the strings for garbage and then run the query, as far as database incorporation goes, I have classes that represent tables and these classes optionally construct themselves using a sqlobject followed with some other identifying parameter. Once again thanks for your time, I appreciate your help. \$\endgroup\$ Commented Feb 1, 2017 at 17:33

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.