patternphpMinor
PDO PHP DB Class
Viewed 0 times
phpclasspdo
Problem
I've been building a PDO PHP DB class and I just wonder if someone could give me some feedback (regarding security, how the code is written and how I can optimize it).
This class is not finished function wise, but I would like the code I have currently to be reviewed.
```
null,
"BOUNDS" => null),
$_insert = array(
"SQL" => null,
"BOUNDS" => null),
$_update = array(
"SQL" => null,
"BOUNDS" => null),
$_delete = array(
"SQL" => null,
"BOUNDS" => null),
$_where = null,
$_results = null, // array
$_countRow = false,
$_countCol = false;
public static function init() {
if(!isset(static::$init)) {
static::$init = new DB();
}
return static::$init;
}
private function __construct() {
try {
$this->_pdo = new PDO('mysql:host=' . Config::get('mysql/host') . ';dbname=' . Config::get('mysql/db'), Config::get('mysql/username'), Config::get('mysql/password'));
$this->_pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$this->_pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_OBJ);
} catch(PDOException $e) {
$this->_error = $e->getMessage();
return $this->_error;
}
}
public function query() {
}
public function prepare() {
// create sql-string
$this->_select["SQL"] == null ? $this->_sql .= $this->_select["SQL"] : $this->_sql .= "{$this->_select['SQL']}; ";
$this->_insert["SQL"] == null ? $this->_sql .= $this->_insert["SQL"] : $this->_sql .= "{$this->_insert['SQL']}; ";
$this->_update["SQL"] == null ? $this->_sql .= $this->_update["SQL"] : $this->_sql .= "{$this->_update['SQL']}; ";
$this->_delete["SQL"] ==
This class is not finished function wise, but I would like the code I have currently to be reviewed.
```
null,
"BOUNDS" => null),
$_insert = array(
"SQL" => null,
"BOUNDS" => null),
$_update = array(
"SQL" => null,
"BOUNDS" => null),
$_delete = array(
"SQL" => null,
"BOUNDS" => null),
$_where = null,
$_results = null, // array
$_countRow = false,
$_countCol = false;
public static function init() {
if(!isset(static::$init)) {
static::$init = new DB();
}
return static::$init;
}
private function __construct() {
try {
$this->_pdo = new PDO('mysql:host=' . Config::get('mysql/host') . ';dbname=' . Config::get('mysql/db'), Config::get('mysql/username'), Config::get('mysql/password'));
$this->_pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$this->_pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_OBJ);
} catch(PDOException $e) {
$this->_error = $e->getMessage();
return $this->_error;
}
}
public function query() {
}
public function prepare() {
// create sql-string
$this->_select["SQL"] == null ? $this->_sql .= $this->_select["SQL"] : $this->_sql .= "{$this->_select['SQL']}; ";
$this->_insert["SQL"] == null ? $this->_sql .= $this->_insert["SQL"] : $this->_sql .= "{$this->_insert['SQL']}; ";
$this->_update["SQL"] == null ? $this->_sql .= $this->_update["SQL"] : $this->_sql .= "{$this->_update['SQL']}; ";
$this->_delete["SQL"] ==
Solution
Keep it flexible and simple
I've recently written a similar class, so I know why you would want to do something like this. At first it seems much easier just to put the needed SQL in a string, and bind values or variables to that, but classes like these make complex statements easier to maintain and reuse.
The use of chaining is good.
As for the security, that seems fine to me. You bind ALL your parameters. Perhaps this is a bit too much? With your system you also cannot bind parameters later, or repeatedly. You basically hide away the binding in your class. I find this limiting, which is the opposite of what you should be aiming at.
In my class I don't hide away the bindings, I prepare the statement and allow for binding of values in two ways:
A: At the statement building stage:
an example would be:
B: At the statement execution stage:
an example would be:
In this last example I can reuse the prepared statement multiple times with different value bindings. I've decided not to allow variable bindings. Note that I use named placeholders, so I don't have to worry about the order of the bindings.
I see a lot of repeated code in your class. This will only get worse when you futher complete your class. The cause is a design decision you made. You use a different array for each SQL command. Your array's contain the same information, you preform the same operations on them, so they are, and should be, the same. Use one array.
A point which should really be addressed is the fact that you've decided to use one class to make the connection to the database, build a statement and execute it. In PDO itself two classes are used for this, for a very good reason. You really don't want to open a new connection for each new statement, you should reuse the connection. I have two classes:
This way I can reuse the database connection for each new statement. Of course I have put each class in its own PHP file.
By putting the SQL in classes I have tried to simplify writing SQL without sacrificing too much flexibility. With my classes you can still write out the SQL in a string and use it directly or you can build a statement using methods of the query class.
You seemed to have concentrated on the bindings mainly, and you forgot about the other things your class should enable.
I've recently written a similar class, so I know why you would want to do something like this. At first it seems much easier just to put the needed SQL in a string, and bind values or variables to that, but classes like these make complex statements easier to maintain and reuse.
The use of chaining is good.
As for the security, that seems fine to me. You bind ALL your parameters. Perhaps this is a bit too much? With your system you also cannot bind parameters later, or repeatedly. You basically hide away the binding in your class. I find this limiting, which is the opposite of what you should be aiming at.
In my class I don't hide away the bindings, I prepare the statement and allow for binding of values in two ways:
A: At the statement building stage:
public function where($conditions,$bindings = NULL)an example would be:
$query->where('ProductID = :ProductID',array('ProductID' => $productID))
->where('SupplierID = :SupplierID',array('SupplierID' => $supplierID));
$data = $query->execute();B: At the statement execution stage:
public function execute($bindings = NULL)an example would be:
$query->where('ProductID = :ProductID')
->where('SupplierID = :SupplierID');
$data = $query->execute(array('ProductID' => $productID,
'SupplierID' => $supplierID));In this last example I can reuse the prepared statement multiple times with different value bindings. I've decided not to allow variable bindings. Note that I use named placeholders, so I don't have to worry about the order of the bindings.
I see a lot of repeated code in your class. This will only get worse when you futher complete your class. The cause is a design decision you made. You use a different array for each SQL command. Your array's contain the same information, you preform the same operations on them, so they are, and should be, the same. Use one array.
A point which should really be addressed is the fact that you've decided to use one class to make the connection to the database, build a statement and execute it. In PDO itself two classes are used for this, for a very good reason. You really don't want to open a new connection for each new statement, you should reuse the connection. I have two classes:
class database extends PDO
// make and maintain a connection to the database
{
public function __construct($dataSource)
}
class query
// perform queries on a database
{
protected $statement = NULL;
public function __construct($db,$query = '')
}
$accessBigData = ['host' => 'localhost',
'type' => 'mysql',
'database' => 'bigdata',
'username' => 'me!',
'password' => 'a_secret'];
$dbBigData = new database($accessBigData);
$query1 = new query($dbBigData);
$query2 = new query($dbBigData);This way I can reuse the database connection for each new statement. Of course I have put each class in its own PHP file.
By putting the SQL in classes I have tried to simplify writing SQL without sacrificing too much flexibility. With my classes you can still write out the SQL in a string and use it directly or you can build a statement using methods of the query class.
You seemed to have concentrated on the bindings mainly, and you forgot about the other things your class should enable.
Code Snippets
public function where($conditions,$bindings = NULL)$query->where('ProductID = :ProductID',array('ProductID' => $productID))
->where('SupplierID = :SupplierID',array('SupplierID' => $supplierID));
$data = $query->execute();public function execute($bindings = NULL)$query->where('ProductID = :ProductID')
->where('SupplierID = :SupplierID');
$data = $query->execute(array('ProductID' => $productID,
'SupplierID' => $supplierID));class database extends PDO
// make and maintain a connection to the database
{
<....>
public function __construct($dataSource)
<....>
}
class query
// perform queries on a database
{
protected $statement = NULL;
<....>
public function __construct($db,$query = '')
<....>
}
$accessBigData = ['host' => 'localhost',
'type' => 'mysql',
'database' => 'bigdata',
'username' => 'me!',
'password' => 'a_secret'];
$dbBigData = new database($accessBigData);
$query1 = new query($dbBigData);
$query2 = new query($dbBigData);Context
StackExchange Code Review Q#69385, answer score: 3
Revisions (0)
No revisions yet.