5
\$\begingroup\$

I wrote this CRUD class in PHP that has methods to do database interactions, but I want to know if it is good/secure/elegant?

It works fine and all and it hasn't ever given me any troubles, but I'm really trying to improve my coding habits, so any feedback would be great!

class DbCrud {

    private $conn;

    /**
     * Connect to the database
     * @param string $host
     * @param string $user
     * @param string $pass
     * @param string $dbname
     */
    function __construct($host='localhost',$user,$pass,$dbname) {
        $this->conn = new mysqli($host, $user, $pass, $dbname);
    }


    /**
     * Disconnect from database
     */
    function __destruct() {
        $this->conn->close();
    }


    /**
     * Add items to database
     * @param string $table
     * @param array $items formatted as 'column'=>'value'
     */
   function create($table,$items) {
       $numcol = count($items);
       $count = 0;
       $query = "INSERT INTO $table (";
       foreach($items as $key=>$value) {
           $key = $this->conn->real_escape_string($key);
           $count++;
           $query .= "$key";
           if($count < $numcol) {
               $query .= ", ";
           }
        }
        $count = 0;
        $query .= ") VALUES (";
        foreach($items as $key=>$value) {
            $key = $this->conn->real_escape_string($value);
            $count++;
            $query .= "'$value'";
            if($count < $numcol) {
                $query .= ", ";
            }
        }
        $query .= ")";
        $this->conn->query($query);
    }


    /**
     * Read from the database
     * @param string $table
     * @param string $selection
     * @param array $conditions formatted as WHERE column=>value
     * @param array $order formatted as column=>direction
     * @return array $data formatted as a multidementional array where first key is the row and the second key is the column name. Returns 0 if no data.
     */
    function read($table,$selection="*",$conditions="",$order="") {
        $numcol = count($conditions);
        $count = 0;
        $query = "SELECT $selection FROM $table";
        if($conditions != "") {
            $query .= " WHERE ";
            foreach($conditions as $key=>$value) {
                if(substr($value,0,1) == "/") {
                    $value = str_replace("/","",$value);
                }
                $query .= "$key='".$this->conn->real_escape_string($value)."'";
                $count++;
                if($count < $numcol) {
                    $query .= " AND ";
                }
            }
        }
        if($order != "") {
            foreach($order as $key=>$value) {
                $query .= " ORDER BY $key $value";
            }
        }
        $count=0;
        $results = $this->conn->query($query); 
        if($results->num_rows == 0) { return 0;} // Return 0 if no value exists
        while($row = mysqli_fetch_assoc($results)) {
            foreach($row as $key=>$value) {
                $data[$count][$key] = $value;
            }
            $count++;
        }
        return $data;
    }


    /**
     * Update database fields
     * @param string $table
     * @param array $items formattd as column=>value
     * @param array $conditions formatted as WHERE column=>value
     */
    function update($table,$items,$conditions) {
        $numcol = count($items);
        $count = 0;
        $query = "UPDATE $table SET ";
        foreach($items as $key=>$value) {
            $key = $this->conn->real_escape_string($key);
            $count++;
            $query .= "$key='$value'";
            if($count < $numcol) {
                $query .= ", ";
            }
        }
        $count = 0;
        $numcol = count($conditions);
        $query .= " WHERE ";
        foreach($conditions as $key=>$value) {
            $key = $this->conn->real_escape_string($key);
            $count++;
            $query .= "$key='$value'";
            if($count < $numcol) {
                $query .= " AND ";
            }
        }
        $this->conn->query($query);
    }

    /**
     * Delete from the database
     * @param string $table
     * @param array $conditions formatted as WHERE column=>value
     */
    function delete($table,$conditions) {
        $query = "DELETE FROM $table WHERE ";
        $numcol = count($conditions);
        foreach($conditions as $key=>$value) {
            $key = $this->conn->real_escape_string($key);
            $count++;
            $query .= "$key='$value'";
            if($count < $numcol) {
                $query .= " AND ";
            }
        }
        $this->conn->query($query);
    }

    /**
     * Perform a custom query on the database
     * @param string $query SQL to be executed
     */

    function custom($query) {
        $this->conn->real_escape_string($query);
        $this->conn->query($query);
    }
}
\$\endgroup\$
3
\$\begingroup\$

Secure

I haven't actually run the code, but from looking over it, it seems that you are open to SQL injections in multiple places:

  • In your create function, you have this: $key=$this->conn->real_escape_string($value);. I'm pretty sure that is supposed to be $value = ..., so right now you are wide open to SQL injection (which are possible into insert statements).
  • Injection into ORDER BY is possible, so you should either defend against it in your read function or document that it is not protected.
  • in your update function, you escape the key, but not the value (which is much more likely to be user controlled), so this is not save either.
  • same problem in delete function.
  • In your create function, you escape the key values, but you have no single quotes around them, which makes the escaping pointless. I would just remove the escaping (because otherwise it looks secure, even when its not), and document that it is not save.
  • the escaping in the custom query doesn't make sense to me. You cannot just escape the whole query, because it will most likely break it. Only variable input should be escaped.

General Approach

I would use PDO or mysqli directly instead of your class, because even though it's a couple of lines more that I have to write, I have a lot more flexibility (LIMIT, JOIN, etc).

I could also use prepared statements, which are generally considered the better approach to preventing SQL injection (because they are harder to mess up, and it's easier to see when they are messed up).

\$\endgroup\$
3
\$\begingroup\$

First:

function __construct($host='localhost',$user,$pass,$dbname) {

You shouldn't follow an optional argument by other mandatory params.

function __construct($host,$user,$pass,$dbname) {

or maybe

function __construct(array $config) {
    $defaults = array(
        'host'   => 'localhost',
        'user'   => 'root',
        'pass'   => 'root',
        'dbname' => 'default'
    );

    foreach($defaults as $key=>$value) {
        if(!isset($config[$key])) {
            $config[$key] = $value;
        }
    }

    $this->conn = new mysqli($config['host'], $config['user'], $config['pass'], $config['dbname']);
}

morover:

function create($table,$items) {

I think there is a possible naming conflicts. With "Create" I think to "Create a table". Maybe it's better rename this with "addRow" or "add".

/**
 * Read from the database
 * @param string $table
 * @param string $selection
 * @param array $conditions formatted as WHERE column=>value
 * @param array $order formatted as column=>direction
 * @return array $data formatted as a multidementional array where first key is the row and the second key is the column name. Returns 0 if no data.
 */
function read($table,$selection="*",$conditions="",$order="") {

see the $order param. It's an array as PHPDoc said, or it's a string? maybe you should write:

function read($table,$selection="*",$conditions="",$order=array()) {

and inside the function

if(!empty($order)) { // was if($order != "")

and for function/code itself, I think it's better a way like that (for example the create method but same procedure could be used anywhere):

$keys   = array_keys($items);
$values = array();

foreach($items as $val) {
    $values[] = $this->conn->real_escape_string($val);
}

$query = "INSERT INTO `$table` (`".implode("`,`",$keys."`) VALUES ('".implode("','",$values)."')";
\$\endgroup\$
  • \$\begingroup\$ Why would create be confusing? Yes it is a SQL keyword but it is commonly used by Rails, Django, Doctrine ETC. The whole point of a crud layer is to abstract out the database details. addRow for example would be odd choice is you ever switched to a no-sql database for example. \$\endgroup\$ – papirtiger Feb 5 '15 at 14:00
  • 1
    \$\begingroup\$ @papirtiger simply because this is a simple DB class. So a DB::create method I think immediately to a Database creation. \$\endgroup\$ – Luca Rainone Feb 5 '15 at 14:06
2
\$\begingroup\$

There are two things which I think your class really needs, which both regards your read() method. The first is that I have no way whatsoever to use the LIMIT keyword. It is good practice to use this keyword when reading from a database, as the database engine can make optimizations.The second is that I cannot use the JOIN keyword, which is also critical to write resource friendly SQL. The fewer times you "hit" the database, the faster/efficient your application will be. If you can fetch all the required information in one database call that is to prefer.

Then there are some less important things that I still think should be mentioned. The keyword DISTINCT can also be very useful.

When inserting rows. What if the row already exists? Some might use the ON DUPLICATE or IF NOT EXISTS keywords, but I cannot find a way to do this.

Of course all this can be solved by using your custom method, but I think that defeats the purpose, since the things I have mentioned are quite common.

Happy coding out there!

\$\endgroup\$

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.