patternphpMinor
MySQL PDO class
Viewed 0 times
mysqlclasspdo
Problem
Please give me any comment about these codes. Does it enough to prevent SQL injection? What I have to do to make the code better?
```
mPDO = new PDO("mysql:host=$dbHost;dbname=$dbName", "$dbUser", "$dbPass");
//$this->mPDO->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
$this->mPDO->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_WARNING);
}
catch(PDOException $e){
die($e->getMessage());
}
}
/**
* Method for executing query
* @param string $query
* @data array Used on queryUpdate method
* @return array Result of query
*/
public function query($query){
$exec = $this->mPDO->prepare($query);
if($data) $exec->execute($data);
else $exec->execute();
$result = $exec->fetchAll();
return $result;
}
/**
* Method for selecting data
* @param $table string
* @param $column array
* @return array Result of query
*/
public function querySelect($table, $column, $where=NULL, $limit=NULL){
if($column!="*"){
$column = $this->buildColumn($column);
}
if(isset($where)){
$condition = $this->BuildWhere($where);
$query = "SELECT {$column} FROM {$table} {$condition}";
}
else {
$query = "SELECT {$column} FROM {$table}";
}
if(isset($limit)){
$query .= " LIMIT {$limit}";
}
$exec = $this->mPDO->prepare($query);
if(isset($where)){
$exec->execute(array_values($where));
}
else{
$exec->execute();
}
return $exec->fetchAll();
}
/**
* Method for insert
* @param string $tableName
* @param ar
```
mPDO = new PDO("mysql:host=$dbHost;dbname=$dbName", "$dbUser", "$dbPass");
//$this->mPDO->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
$this->mPDO->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_WARNING);
}
catch(PDOException $e){
die($e->getMessage());
}
}
/**
* Method for executing query
* @param string $query
* @data array Used on queryUpdate method
* @return array Result of query
*/
public function query($query){
$exec = $this->mPDO->prepare($query);
if($data) $exec->execute($data);
else $exec->execute();
$result = $exec->fetchAll();
return $result;
}
/**
* Method for selecting data
* @param $table string
* @param $column array
* @return array Result of query
*/
public function querySelect($table, $column, $where=NULL, $limit=NULL){
if($column!="*"){
$column = $this->buildColumn($column);
}
if(isset($where)){
$condition = $this->BuildWhere($where);
$query = "SELECT {$column} FROM {$table} {$condition}";
}
else {
$query = "SELECT {$column} FROM {$table}";
}
if(isset($limit)){
$query .= " LIMIT {$limit}";
}
$exec = $this->mPDO->prepare($query);
if(isset($where)){
$exec->execute(array_values($where));
}
else{
$exec->execute();
}
return $exec->fetchAll();
}
/**
* Method for insert
* @param string $tableName
* @param ar
Solution
- Your
BuildInsert-method is the only one which usesmysql_real_escape_string. Why? Why not just use parametrized queries like in your "select", "update" and "delete" cases?
- Your
querymethod uses a variable$datawhich is not defined. Probably a missing parameter.
-
if($exec->execute()){
return "Insert into database succeed.";
}
else{
return "Insert into database failed.";
}This is bad, don't return a string when a
bool would suffice. What if you want to translate your application?- Sometimes you use method names beginning with a lower case like
queryInsertand in other cases you start with an upper case likeQueryUpdate. Be consistent.
$mQuery- this could be replaced by a local variable. Except you want to extend your class so you can fetch the last query. Otherwise: ditch it.
$mDbHost- not used, ditch it.
Update
-
This:
if($exec->execute(array_values($data))){
return true;
}
else{
return false;
}can be written as:
return $exec->execute(array_values($data));-
There's also a special case for update and delete which might return a count of affected rows. I would solve it like that:
if($exec->execute($paramVal)){
return $exec->rowCount();
}
else {
return false;
}That way you can check if the query failed by using the
!== or ===-operators e.g.:$rowsDeleted = $yourpdo->queryDelete("posts", array("PostID" => 5));
// $rowsDeleted might be 0 if the post with id "5" does not exist so
// check with ===
if($rowsDeleted === false) {
echo "There was an error";
} else {
echo "{$rowsDeleted} rows affected";
}The wording for your documentation would be
returns the number of rows affected or FALSE on error.Code Snippets
if($exec->execute()){
return "Insert into database succeed.";
}
else{
return "Insert into database failed.";
}if($exec->execute(array_values($data))){
return true;
}
else{
return false;
}return $exec->execute(array_values($data));if($exec->execute($paramVal)){
return $exec->rowCount();
}
else {
return false;
}$rowsDeleted = $yourpdo->queryDelete("posts", array("PostID" => 5));
// $rowsDeleted might be 0 if the post with id "5" does not exist so
// check with ===
if($rowsDeleted === false) {
echo "There was an error";
} else {
echo "{$rowsDeleted} rows affected";
}Context
StackExchange Code Review Q#5262, answer score: 2
Revisions (0)
No revisions yet.