PHP framework building: MySQL Connection and query class

Question

I am building a PHP framework and would like to get some feedback on a few different sections of the project so far. I consider myself still a neophyte in PHP so I would like to ask if I'm going about completing these different tasks in an efficient and or correct way.

This section is of the MySQL Connection class I have created for it. So for I have created the basic operations for connecting and querying the database. I have posted below the class code and a script showing how it can be use.

I would like to get feedback on if this is the proper way to create this type of class and an efficient way to complete database querying tasks.

MySQL Connection Class

class MysqlConnection{ //Connetion login private $host; private $user; private $password; private $database; private $connection; //Query being used public $query; /** * @param String $host - server host * @param String $user - username * @param String $database - database name * @param String $password - username password * @return Null */ public function __construct($host,$user,$password,$database){ if(isset($host,$user,$password,$database)){ //If parameter supplied $this->host = $host; $this->user = $user; $this->password = $password; $this->database = $database; }else{ return false; } } /** * Send query to database * * @param String $query - query statment * * Note: This allow a query to the DB if * the class is going to be used withing a fuction * or method */ public function Query($query){ if(isset($this->connection)){ if(($this->query = @$this->connection->query($query))){ return $this->query; } return false; }else{ return false; } } public function LastId(){ return $this->connection->insert_id; } /** * Result */ public function Result($quresults = null){ if(empty($quresults) && !empty($this->query)){ return $this->query->fetch_assoc(); } return $quresults->fetch_assoc(); } /** * Inserts a new row into the database. * * @param Array $data - key column name value data * @param String $table - name of DB table * @return results on success else mysql error string on failure * */ public function insert($data, $table) { $columns = ""; $values = ""; foreach ($data as $column => $value) { $columns .= ($columns == "") ? "" : ", "; $columns .= $column; $values .= ($values == "") ? "" : ", "; $values .= "'".addslashes($value)."'"; } $sql = "insert into $table ($columns) values ($values)"; if(isset($this->connection)){ if(!($this->query = @$this->connection->query($sql))){ return false; } return $this->query; }else{ trigger_error('MySQLConnection not opened',E_USER_ERROR); } } /** * Update a row in DB * * @param Array $data - key is the column name and value is data * @param String $table - name of table to update row * @param string $where - where clause */ public function update($data, $table, $where) { if(isset($this->connection)){ foreach ($data as $column => $value) { $sql = "UPDATE $table SET $column = '".addslashes($value)."' WHERE $where"; if(!@$this->connection->query($sql)){ return false; } } return true; }else{ trigger_error('MySQLConnection not opened',E_USER_ERROR); } } /** * Select rows from the database * * */ public function select($select = '*',$table, $where) { if(isset($this->connection)){ $sql = "SELECT ".$select." FROM $table WHERE $where"; if(($result = $this->connection->query($sql))){ if($result->num_rows > 0){ return ($this->query = $result); } return false; } return false; }else{ trigger_error('MySQLConnection not opened',E_USER_ERROR); } } /** * Delete row from */ public function delete($table,$where){ if(isset($this->connection)){ $sql = "DELETE FROM ".$table." WHERE ".$where; $this->connection->query($sql); if($this->connection->affected_rows > 0){ return true; } return false; } } /** * Close DB connection * * @param void * @return void */ public function Close(){ $this->connection->close(); } /** * Open DB connection using parameters given * in the construct * * @param void * @return boolean - TRUE on success else FALSE on failure */ public function Open(){ $this->connection = @new mysqli($this->host,$this->user,$this->password,$this->database); //return !!$this->connection->connect_errno; if($this->connection->connect_errno){ return false; }else{ return $this->connection; } } /** * Get last DB connection or query error * * @param void * @return DB connection or query error string * * Dev Note: Modify method to allowing opionsel * return value */ public function GetError() { if(!empty($this->connection->connect_error)){ return $this->connection->connect_error; }elseif(!empty($this->connection->error)){ return $this->connection->error; } } /** * Escape special characters by * adding slashes * * @param String - string to escape characters * @return String - escape string */ public function AddSlashes($string){ return addslashes($string); } /** * Break an array up into comma seperated value list * * @param Array $array - an array or strings * @return String of CSV list */ public function PBreak($array){ if(is_array($array)){ return implode(',', $string); } } /** * Close open DB connections * * @param void * @return void */ public function __destruct(){ if(!empty($this->connection)){ @$this->connection->close(); } } } 

Script using it's methods

 //Conntect to DB $dbconnection = $core->CreateCore('mysqlconnection','database','localhost','root','password','dummy'); $dbconnection->Open(); $queryReturn = $dbconnection->Query("SELECT * FROM dummy WHERE num = 1"); echo "<pre>"; //Using Result() var_dump($dbconnection->Result()); echo "</pre>"; /** * equivalent using QuerySelecti * * The dummy table columns are * num INT, text VARCHAR, other VARCHAR */ //Using insert() method $dbconnection->insert(array('num'=>4,'text'=>'New insert','other'=>'This is a text'),'dummy'); //Using LastId() method echo "<pre>"; //Using Result() var_dump($dbconnection->LastId()); echo "</pre>"; //using GetError() method if(!$dbconnection->GetError()){ $dbconnection->update(array('num'=>4,'text'=>'Another insert','other'=>'This is NEW text'),'dummy','num = 4'); } //using select() method $queryReturn = $dbconnection->select("num, text, other",'dummy','num = 4'); echo "<pre>"; var_dump($dbconnection->Result()); echo "</pre>"; //Using update method $dbconnection->delete('dummy','num = 4'); //Using close() method $dbconnection->Close(); 

Any feedback would be appreciated.

Answer 1

Use early returns to simplify if statements

This code from the insert function:

 if(isset($this->connection)){ if(($this->query = @$this->connection->query($query))){ return $this->query; } return false; }else{ return false; } 

Can be rewritten like this:

 if(!isset($this->connection)){ return false; } if(($this->query = @$this->connection->query($query))){ return $this->query; } return false; 

You can use this principle in other functions as well (for example for the isset check in update, delete and select).

In this case, I would make the setting of $this->query more explicit:

 if(!isset($this->connection)){ return false; } $this->query = @$this->connection->query($query); return $this->query; 

You don't need a separate command to return false, as query returns false on failure.

Further if simplifications

This from your select function:

 if(($result = $this->connection->query($sql))){ if($result->num_rows > 0){ return ($this->query = $result); } return false; } return false; 

can be collapsed to one if statement:

 $result = $this->connection->query($sql); if($result && $result->num_rows > 0){ return ($this->query = $result); } return false; 

And this from your delete function:

 if($this->connection->affected_rows > 0){ return true; } return false; 

can be rewritten like this:

 return $this->connection->affected_rows > 0; 

Result function

Do you expect a use case where you need to pass a parameter to Result? If not, I would remove it, it just complicates your code.

Either way, right now, if I pass null (or nothing), and $this->query is empty, fetch_assoc will be called on null.

update function

Right now, multiple updates can be executed with one call of update. If one fails, the rest are not executed. But the previously executed once are not rolled back either.

And the caller doesn't know which statements got executed and which didn't. I think that this might cause problems in the future.

SQL injection

addslashes does not adequately defend against SQL injections (see for example here, here, or here).

See also this answer where I link to more resources about SQL Injections and prevention of it.

Also, it might be confusing for a future user of your class that some values are escaped while others are not (and thus the escaping is the task of the caller). $table, $columns, $column, $where, and of course $query are not escaped, while $value is.

Style Guidelines

• generally, function names should start with a lower-case letter (you can even see it here in the code highlighter: the upper-case functions are rendered the same as the class, while the lower-case functions are not).
• a minor point: try to be consistent with your curly brackets and the spaces around them (){ vs ) {).
• be consistent with your indentation.
• be consistent with string concatenation (FROM $table WHERE $where vs FROM ".$table." WHERE ".$where).
• be consistent with your single- and double-quotes ("num, text, other" vs 'dummy').

General Approach

For me, your example calls:

insert(array('num'=>4,'text'=>'New insert','other'=>'This is a text'),'dummy') select("num, text, other",'dummy','num = 4') 

are a bit hard to read. You could instead implement a query builder to make them look something like this:

new QueryBuilder().select("num", "other").from("dummy").where("num = ?", 4) 

Note also that the 4 is separate from the sql syntax, thus making it easier to use prepared statements when executing such a query.

For ideas for a querybuilder interface, you could look at existing once, such as FluentPDO or the QueryBuilder of Doctrine. You could also take a look at the source code of FluentPDO to get some ideas (it's just a couple of files).

Answer 2

In addition to previous answer:

• You shouldn't use @, use try {} catch () {} instead.
• You shouldn't use trigger_error, use exceptions instead.
• You should use statement preparing and argument binding instead of concatenation, this would add another security layer between user and database, and you wouldn't need to use addslashes at all.
• You should create interface(for example DbConnection) with all your methods and then implement it, this allows extending your library for another databases
• Try to split your class into connection itself, it's state (pattern State), query(pattern Query-Object) and query builder(pattern Builder).
• As additional: try to map objects to tables :)