patternphpMinor
PHP PDO / MySQL helper/wrapper functions
Viewed 0 times
helperphpmysqlwrapperfunctionspdo
Problem
Further to my previous post for a user login script I've been learning PDO in order to migrate from mysql_ functions. As before with mysql_, I wanted to wrap the PDO code into wrapper functions, so this is what I've done here.
I've created two functions:
Since I'm new to PDO I would like feedback on a) any errors in my general usage of PDO and b) the helper functions:
```
function db_single_prepared_query($query, $parameters=array())
{
// see what type of query this is, use this later to customise return values
if (pr
I've created two functions:
db_connect(), which creates a connection and will be called at the start of each script
db_single_prepared_query(), which will run a single, prepared query, supporting just the four CRUD operations. I'll create more functions for other uses as required.
Since I'm new to PDO I would like feedback on a) any errors in my general usage of PDO and b) the helper functions:
function db_connect()
{
// Define connection as a static variable, to avoid connecting more than once
static $dbh;
// Try and connect to the database, if a connection has not been established yet
if(!isset($dbh))
{
try
{
$dsn = 'mysql:dbname=' . MYSQL_DBNAME . ';host=' . MYSQL_HOSTNAME . ';charset=utf8';
$dbh = new PDO($dsn, MYSQL_USERNAME, MYSQL_PASSWORD);
$dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);//which tells PDO to disable emulated prepared statements and use real prepared statements. This makes sure the statement and the values aren't parsed by PHP before sending it to the MySQL server (giving a possible attacker no chance to inject malicious SQL).
$dbh->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
catch(PDOException $e)
{
print "Cannot connect to database";
log_this("db_connect - " . $e->getMessage(), "error.log");
exit();
}
}
return $dbh;
}db_single_prepared_query() which calls db_connect(), which only connects once per script life:.```
function db_single_prepared_query($query, $parameters=array())
{
// see what type of query this is, use this later to customise return values
if (pr
Solution
Here are some of my observations. This is code review so the observations are about your code, but I will also answer your questions along the way.
1: Functions, not classes. Nothing wrong with that, just unexpected. Classes are better at compartiamentilizing names. They are the preferred choice for most programming tasks, but functions will do just fine in this case.
2: Wrapping means restricting yourself. But you restrict yourself unnecessarily. You can only ever have one database connection with these functions. This might seem fine now, but in the future you might want to connect to more than one database. Yes, you will. And defining the database access parameters as constants has the same problem.
3: In virtually all cases the third parameter of
and this will work just fine. I would also leave the
4: I would not use rowCount() for SELECT, since it might not always give the correct result. You have
5: Errors will occur when accessing a database. For now it's fine to just through a wobbler and exit with an error message. This is not acceptable in a production environment. Errors should be handled properly, or at least your function should give the user the ability to do so.
6: All incoming data should be treated with the utmost suspicion. The fact that you bind might help to prevent SQL injection in these functions, but lots of other types of attacks exist. Hackers will try to change the input and see what happens. All input should therefore be filtered and clear restrictions on their values should always be imposed.
1: Functions, not classes. Nothing wrong with that, just unexpected. Classes are better at compartiamentilizing names. They are the preferred choice for most programming tasks, but functions will do just fine in this case.
2: Wrapping means restricting yourself. But you restrict yourself unnecessarily. You can only ever have one database connection with these functions. This might seem fine now, but in the future you might want to connect to more than one database. Yes, you will. And defining the database access parameters as constants has the same problem.
3: In virtually all cases the third parameter of
bindValue() is not needed. Given what you try to do, you could easily leave it out. This would simplify your code quite a lot. To change your example:$parameters = array(':cid' => $cid,
':maxuid' => $maxuid);
$info = db_single_prepared_query($query, $parameters);and this will work just fine. I would also leave the
: out of the parameter keys and add it later, but it can be argued it should be there. 4: I would not use rowCount() for SELECT, since it might not always give the correct result. You have
count($rows), so why use it?5: Errors will occur when accessing a database. For now it's fine to just through a wobbler and exit with an error message. This is not acceptable in a production environment. Errors should be handled properly, or at least your function should give the user the ability to do so.
6: All incoming data should be treated with the utmost suspicion. The fact that you bind might help to prevent SQL injection in these functions, but lots of other types of attacks exist. Hackers will try to change the input and see what happens. All input should therefore be filtered and clear restrictions on their values should always be imposed.
Code Snippets
$parameters = array(':cid' => $cid,
':maxuid' => $maxuid);
$info = db_single_prepared_query($query, $parameters);Context
StackExchange Code Review Q#83209, answer score: 2
Revisions (0)
No revisions yet.