Reference: What is a perfect code sample using the

Question

This is to create a community learning resource. The goal is to have examples of good code that do not repeat the awful mistakes that can so often be found in copy/pasted PHP code. I have requested it be made Community Wiki.

This is not meant as a coding contest. It's not about finding the fastest or most compact way to do a query - it's to provide a good, readable reference especially for newbies.

Every day, there is a huge influx of questions with really bad code snippets using the mysql_* family of functions on Stack Overflow. While it is usually best to direct those people towards PDO, it sometimes is neither possible (e.g. inherited legacy software) nor a realistic expectation (users are already using it in their project).

Common problems with code using the mysql_* library include:

• SQL injection in values
• SQL injection in LIMIT clauses and dynamic table names
• No error reporting ("Why does this query not work?")
• Broken error reporting (that is, errors always occur even when the code is put into production)
• Cross-site scripting (XSS) injection in value output

Let's write a PHP code sample that does the following using the mySQL_* family of functions:

• Accept two POST values, id (numeric) and name (a string)
• Do an UPDATE query on a table tablename, changing the name column in the row with the ID id
• On failure, exit graciously, but show the detailed error only in production mode. trigger_error() will suffice; alternatively use a method of your choosing
• Output the message "$name updated."

And does not show any of the weaknesses listed above.

It should be as simple as possible. It ideally doesn't contain any functions or classes. The goal is not to create a copy/pasteable library, but to show the minimum of what needs to be done to make database querying safe.

Bonus points for good comments.

The goal is to make this question a resource that a user can link to when encountering a question asker who has bad code (even though it isn't the focus of the question at all) or is confronted with a failing query and doesn't know how to fix it.

To pre-empt PDO discussion:

Yes, it will often be preferable to direct the individuals writing those questions to PDO. When it is an option, we should do so. It is, however, not always possible - sometimes, the question asker is working on legacy code, or has already come a long way with this library, and is unlikely to change it now. Also, the mysql_* family of functions is perfectly safe if used properly. So no "use PDO" answers here please.

Answer 1

My stab at it. Tried to keep it as simple as possible, while still maintaining some real-world conveniences.

Handles unicode and uses loose comparison for readability. Be nice ;-)

<?php

header('Content-type: text/html; charset=utf-8');
error_reporting(E_ALL | E_STRICT);
ini_set('display_errors', 1);
// display_errors can be changed to 0 in production mode to
// suppress PHP's error messages

/*
Can be used for testing
$_POST['id'] = 1;
$_POST['name'] = 'Markus';
*/

$config = array(
    'host' => '127.0.0.1', 
    'user' => 'my_user', 
    'pass' => 'my_pass', 
    'db' => 'my_database'
);

# Connect and disable mysql error output
$connection = @mysql_connect($config['host'], 
    $config['user'], $config['pass']);

if (!$connection) {
    trigger_error('Unable to connect to database: ' 
        . mysql_error(), E_USER_ERROR);
}

if (!mysql_select_db($config['db'])) {
    trigger_error('Unable to select db: ' . mysql_error(), 
        E_USER_ERROR);
}

if (!mysql_set_charset('utf8')) {
    trigger_error('Unable to set charset for db connection: ' 
        . mysql_error(), E_USER_ERROR);
}

$result = mysql_query(
    'UPDATE tablename SET name = "' 
    . mysql_real_escape_string($_POST['name']) 
    . '" WHERE id = "' 
    . mysql_real_escape_string($_POST['id']) . '"'
);

if ($result) {
    echo htmlentities($_POST['name'], ENT_COMPAT, 'utf-8') 
        . ' updated.';
} else {
    trigger_error('Unable to update db: ' 
        . mysql_error(), E_USER_ERROR);
}

Answer 2

I decided to jump the gun and just put something up. It's something to start with. Throws an exception on error.

function executeQuery($query, $args) {
    $cleaned = array_map('mysql_real_escape_string', $args);

    if($result = mysql_query(vsprintf($query, $cleaned))) {
        return $result;
    } else {
        throw new Exception('MySQL Query Error: ' . mysql_error());
    }
}

function updateTablenameName($id, $name) {
    $query = "UPDATE tablename SET name = '%s' WHERE id = %d";

    return executeQuery($query, array($name, $id));
}

try {
    updateTablenameName($_POST['id'], $_POST['name']);
} catch(Exception $e) {
    echo $e->getMessage();
    exit();
}

Answer 3

<?  
mysql_connect(); 
mysql_select_db("new"); 
$table = "test"; 
if($_SERVER['REQUEST_METHOD']=='POST') {
  $name = mysql_real_escape_string($_POST['name']); 
  if ($id = intval($_POST['id'])) { 
    $query="UPDATE $table SET name='$name' WHERE id=$id"; 
  } else { 
    $query="INSERT INTO $table SET name='$name'"; 
  } 
  mysql_query($query) or trigger_error(mysql_error()." in ".$query); 
  header("Location: http://".$_SERVER['HTTP_HOST'].$_SERVER['PHP_SELF']);  
  exit;  
}  
if (!isset($_GET['id'])) {
  $LIST=array(); 
  $query="SELECT * FROM $table";  
  $res=mysql_query($query); 
  while($row=mysql_fetch_assoc($res)) $LIST[]=$row; 
  include 'list.php'; 
} else {
  if ($id=intval($_GET['id'])) { 
    $query="SELECT * FROM $table WHERE id=$id";  
    $res=mysql_query($query); 
    $row=mysql_fetch_assoc($res); 
    foreach ($row as $k => $v) $row[$k]=htmlspecialchars($v); 
  } else { 
    $row['name']=''; 
    $row['id']=0; 
  } 
  include 'form.php'; 
}  
?>

form.php

<? include 'tpl_top.php' ?>
<form method="POST">
<input type="text" name="name" value="<?=$row['name']?>"><br>
<input type="hidden" name="id" value="<?=$row['id']?>">
<input type="submit"><br>
<a href="?">Return to the list</a>
</form>
<? include 'tpl_bottom.php' ?>

list.php

<? include 'tpl_top.php' ?>
<a href="?id=0">Add item</a>
<? foreach ($LIST as $row): ?>
<li><a href="?id=<?=$row['id']?>"><?=$row['name']?></a>
<? endforeach ?>
<? include 'tpl_bottom.php' ?>

Answer 4

Looks like my other answer missed the aim of the question.
(this one doesn't meet some requirements either, but as it can be seen, no safe solution can be achieved without implementing a function to process placeholders, which are being the cornerstone of the safe queries)

So, here is another attempt to post concise solution to make mysql queries safe yet handy.

A function I wrote long time ago and it served me well until I moved to the corporative standard OOP-based solution.
There was 2 goals to pursue for: security and ease of use.

First one achieved by implementing placeholders.
Second one achieved by implementing placeholders and different result types.

The function surely not ideal one. Some drawbacks are:

• no % chars have to be placed in the query directly as it's using printf syntax.
• no multiple connections supported.
• no placeholder for the identifiers (as well as many other handy placeholders).
• again, no identifier placeholder!. "ORDER BY $field" case have to be handled manually!
• of course an OOP implementation would be much more flexible, having neat distinct methods instead ugly "mode" variable as well other necessary methods.

Yet it is good, safe and concise, no need to install a whole library.

function dbget() {
  /*
  usage: dbget($mode, $query, $param1, $param2,...);
  $mode - "dimension" of result:
  0 - resource
  1 - scalar
  2 - row
  3 - array of rows
  */
  $args = func_get_args();
  if (count($args) < 2) {
    trigger_error("dbget: too few arguments");
    return false;
  }
  $mode  = array_shift($args);
  $query = array_shift($args);
  $query = str_replace("%s","'%s'",$query); 

  foreach ($args as $key => $val) {
    $args[$key] = mysql_real_escape_string($val);
  }

  $query = vsprintf($query, $args);
  if (!$query) return false;

  $res = mysql_query($query);
  if (!$res) {
    trigger_error("dbget: ".mysql_error()." in ".$query);
    return false;
  }

  if ($mode === 0) return $res;

  if ($mode === 1) {
    if ($row = mysql_fetch_row($res)) return $row[0];
    else return NULL;
  }

  $a = array();
  if ($mode === 2) {
    if ($row = mysql_fetch_assoc($res)) return $row;
  }
  if ($mode === 3) {
    while($row = mysql_fetch_assoc($res)) $a[]=$row;
  }
  return $a;
}
?>

usage examples

$name = dbget(1,"SELECT name FROM users WHERE id=%d",$_GET['id']);
$news = dbget(3,"SELECT * FROM news WHERE title LIKE %s LIMIT %d,%d",
              "%$_GET[search]%",$start,$per_page);

As it can be seen from the above examples, the main difference from all the codes ever posted in Stackoverflow, both safety and data retrieval routines are encapsulated in the function code. So, no manual binding, escaping/quoting or casting, as well as no manual data retrieval.

combined with other helper function

function dbSet($fields,$source=array()) {
  $set = '';
  if (!$source) $source = &$_POST;
  foreach ($fields as $field) {
    if (isset($source[$field])) {
      $set.="`$field`='".mysql_real_escape_string($source[$field])."', ";
    }
  }
  return substr($set, 0, -2); 
}

used like this

$fields = explode(" ","name surname lastname address zip phone regdate");
$_POST['regdate'] = $_POST['y']."-".$_POST['m']."-".$_POST['d'];
$sql = "UPDATE $table SET ".dbSet($fields).", stamp=NOW() WHERE id=%d";
$res = dbget(0,$sql, $_POST['id']);
if (!$res) {
  _503;//calling generic 503 error function
}

it may cover almost every need, including the example case from the OP.

Answer 5

/**
 * Rule #0: never trust users input!
 */

//sanitize integer value
$id = intval($_GET['id']);
//sanitize string value;
$name = mysql_real_escape_string($_POST['name']);
//1. using `dbname`. is better than using mysql_select_db()
//2. names of tables and columns should be quoted by "`" symbol
//3. each variable should be sanitized (even in LIMIT clause)
$q = mysql_query("UPDATE `dbname`.`tablename` SET `name`='".$name."' WHERE `id`='".$id."' LIMIT 0,1 ");
if ($q===false)
{
    trigger_error('Error in query: '.mysql_error(), E_USER_WARNING);
}
else
{
    //be careful! $name contains user's data, remember Rule #0
    //always use htmlspecialchars() to sanitize user's data in output
    print htmlspecialchars($name).' updated';
}

########################################################################
//Example, how easily is to use set_error_handler() and trigger_error()
//to control error reporting in production and dev-code
//Do NOT use error_reporting(0) or error_reporting(~E_ALL) - each error
//should be fixed, not muted
function err_handler($errno, $errstr, $errfile, $errline)
{
    $hanle_errors_print = E_ALL & ~E_NOTICE;

    //if we want to print this type of errors (other types we can just write in log-file)
    if ($errno & $hanle_errors_print)
    {
        //$errstr can contain user's data, so... Rule #0
        print PHP_EOL.'Error ['.$errno.'] in file '.$errfile.' in line '.$errline
              .': '.htmlspecialchars($errstr).PHP_EOL;
    }
    //here you can write error into log-file
}

set_error_handler('err_handler', E_ALL & ~E_NOTICE & E_USER_NOTICE & ~E_STRICT & ~E_DEPRECATED);

And some explanation of comments:

//1. using `dbname`. is better than using mysql_select_db()

With using mysql_select_db you can create errors, and it will be not so easy to find and fix them.
For example, in some script you will set db1 as database, but in some function you need to set db2 as database.
After calling this function, database will be switched, and all following queries in script will be broken or will broke some data in wrong database (if names of tables and columns will coincide).

//2. names of tables and columns should be quoted by "`" symbol 

Some names of columns can be also SQL-keywords, and using "`" symbol will help with that.
Also, all string-values, inserted to query, should be quoted by ' symbol.

//always use htmlspecialchars() to sanitize user's data in output
It will help you to prevent XSS-attacks.