patternphpMinor
Extended PDO class for MySQL
Viewed 0 times
extendedmysqlforclasspdo
Problem
I recently started with object oriented programming in PHP and I am wondering if my database class is on the right track.
Am I doing something wrong? If so, how can I improve it?
Here is the class:
```
setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}catch(PDOException $e){
return $e->getMessage();
//die('Something went wrong, please try again.');
}
}
public function queryInsert($tableName, $insertKey, $insertData, $executeData){
$prepare = parent::prepare("INSERT INTO {$tableName} ({$insertKey}) VALUES ({$insertData})");
try{
$prepare->execute(
$executeData
);
}catch(PDOException $e){
return $e->getMessage();
}
}
public function queryUpdate($tableName, $insertData, $executeData, $insertWhereKey, $insertWhereValue){
$prepare = parent::prepare("UPDATE {$tableName} SET {$insertData} WHERE {$insertWhereKey} = {$insertWhereValue}");
try{
$prepare->execute(
$executeData
);
}catch(PDOException $e){
return $e->getMessage();
}
}
public function queryDelete($tableName, $deleteWhereKey, $deleteWhereValue){
$prepare = parent::prepare("DELETE FROM {$tableName} WHERE {$deleteWhereKey} = {$deleteWhereValue}");
try{
$prepare->execute();
}catch(PDOException $e){
return $e->getMessage();
}
}
public function querySelect($tableName, $selectData, $sellectWhereKey = null, $selectWhereValue = null, $orderByKey = null, $orderByValue = null){
if(isset($selectWhereKey, $selectWhereValue)){
$prepare = parent::prepare("SELECT {$selectData} FROM {$tableName} WHERE $insertWhere");
}
elseif(isset($orde
Am I doing something wrong? If so, how can I improve it?
Here is the class:
```
setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}catch(PDOException $e){
return $e->getMessage();
//die('Something went wrong, please try again.');
}
}
public function queryInsert($tableName, $insertKey, $insertData, $executeData){
$prepare = parent::prepare("INSERT INTO {$tableName} ({$insertKey}) VALUES ({$insertData})");
try{
$prepare->execute(
$executeData
);
}catch(PDOException $e){
return $e->getMessage();
}
}
public function queryUpdate($tableName, $insertData, $executeData, $insertWhereKey, $insertWhereValue){
$prepare = parent::prepare("UPDATE {$tableName} SET {$insertData} WHERE {$insertWhereKey} = {$insertWhereValue}");
try{
$prepare->execute(
$executeData
);
}catch(PDOException $e){
return $e->getMessage();
}
}
public function queryDelete($tableName, $deleteWhereKey, $deleteWhereValue){
$prepare = parent::prepare("DELETE FROM {$tableName} WHERE {$deleteWhereKey} = {$deleteWhereValue}");
try{
$prepare->execute();
}catch(PDOException $e){
return $e->getMessage();
}
}
public function querySelect($tableName, $selectData, $sellectWhereKey = null, $selectWhereValue = null, $orderByKey = null, $orderByValue = null){
if(isset($selectWhereKey, $selectWhereValue)){
$prepare = parent::prepare("SELECT {$selectData} FROM {$tableName} WHERE $insertWhere");
}
elseif(isset($orde
Solution
Your approach vs PDO
First of, you take perfectionally acceptable exceptions, and transform them into error strings, which are harder to handle.
And I'm also not sure if all your the methods really make things easier.
Let's take
Without your code, it's not that much more to write, and it is a lot clearer what is actually happening.
Improving your approach
Now, I'm not generally a fan of wrapping/extending PDO like this, but you can improve your methods to make it a bit more usable.
First of, documentation. It is completely unclear from the outside how to use your code. This can also easily lead to security issues, as
Secondly, it's really not needed that a user passes
Thirdly, as mentioned, use exceptions instead of error strings.
A better method signature might be this:
Now it can be used like this:
That's already a bit nicer, but really not that much simpler than the original query (and harder to use, as it's not really clear what the passed values need to be), so I'm not sure if it's worth it.
I would probably stick to the well-known and simple PDO interface. If you want to simplify it, or don't like to write plain SQL queries, something like a query builder or an orm might be worth looking into.
Security
Your approach completely disguises which values are safe and which are not, which will lead to security issues.
Basically, only
For some of the other values, a user may not expect it to be secure (such as
For some values, this is devestating. Having such an interface as yours, I would completely expect eg
Misc
First of, you take perfectionally acceptable exceptions, and transform them into error strings, which are harder to handle.
And I'm also not sure if all your the methods really make things easier.
Let's take
insert as an example:// with your code:
$result = $db->queryInsert("myTable", "key1, key2, key3", ":value1, :value2, :value3", ["value1", "value2", "value3"]);
if ($result == "well, what do I even check here?") {
// handle error
}
// without your code:
$prepare = $db->prepare("INSERT INTO myTable (key1, key2, key3) VALUES (:value1, :value2, :value3)");
try {
$prepare->execute(["value1", "value2", "value3"]);
} catch(PDOException $e) {
// handle error
}Without your code, it's not that much more to write, and it is a lot clearer what is actually happening.
Improving your approach
Now, I'm not generally a fan of wrapping/extending PDO like this, but you can improve your methods to make it a bit more usable.
First of, documentation. It is completely unclear from the outside how to use your code. This can also easily lead to security issues, as
$insertKey and $insertData should not be user-supplied. Secondly, it's really not needed that a user passes
$insertData themselves. It makes your method harder to use, it doesn't provide any additional functionality for a user, and as said above, it can easily lead to vulnerabilities.Thirdly, as mentioned, use exceptions instead of error strings.
A better method signature might be this:
public function queryInsert($tableName, $insertKey, $insertData) throws PDOExceptionNow it can be used like this:
try {
$db->queryInsert("myTable", "key1, key2, key3", ["value1", "value2", "value3"]);
} catch(PDOException $e) {
// handle error
}That's already a bit nicer, but really not that much simpler than the original query (and harder to use, as it's not really clear what the passed values need to be), so I'm not sure if it's worth it.
I would probably stick to the well-known and simple PDO interface. If you want to simplify it, or don't like to write plain SQL queries, something like a query builder or an orm might be worth looking into.
Security
Your approach completely disguises which values are safe and which are not, which will lead to security issues.
Basically, only
$executeData may contain user input. For some of the other values, a user may not expect it to be secure (such as
$tableName), but just to be safe, I would still validate it.For some values, this is devestating. Having such an interface as yours, I would completely expect eg
$insertWhereValue to be secure. Misc
- Bug:
$insertWheredoesn't exist.
- You have a couple of spelling mistakes, such as
sellectormysel.
Code Snippets
// with your code:
$result = $db->queryInsert("myTable", "key1, key2, key3", ":value1, :value2, :value3", ["value1", "value2", "value3"]);
if ($result == "well, what do I even check here?") {
// handle error
}
// without your code:
$prepare = $db->prepare("INSERT INTO myTable (key1, key2, key3) VALUES (:value1, :value2, :value3)");
try {
$prepare->execute(["value1", "value2", "value3"]);
} catch(PDOException $e) {
// handle error
}public function queryInsert($tableName, $insertKey, $insertData) throws PDOExceptiontry {
$db->queryInsert("myTable", "key1, key2, key3", ["value1", "value2", "value3"]);
} catch(PDOException $e) {
// handle error
}Context
StackExchange Code Review Q#117413, answer score: 2
Revisions (0)
No revisions yet.