Update Personal Details Script

[chibineku]

I have created a form that displays a user's personal information and lets them change their details. My handler cycles through the fields of the form and sorts them based on various conditions. Any thoughts on why it doesn't actually do anything would be appreciated I've looked at it to the point of physical nausea now. Here is the script with notes where necessary:

<?phpsession_start();include_once("db_include.php5");doDB();if(!$_POST) {  //come directly via address bar  header("Location: index.hmtl");  exit;}//loop through all the post variablesforeach ($_POST as $k => $v) {    if(eregi("confirm",$k) || eregi("old",$k)) {//the field in question is a duplicate one or there for authentication purposes and shouldn't be added to a table	continue;  }    if($k == "address" || $k == "town" || $k == "city" || $k == "postcode") {		//use aromaAddress table			$v = trim(htmlspecialchars(check_chars_mailto(mysqli_real_escape_string($mysqli,$v))));				if(empty($v)) {//the field is empty...do nothing		  continue; 		}  //create query  $update_sql = "UPDATE aromaAddress SET ".$k." = '".$v."' WHERE userid = '".$_SESSION["userid"]."'";  $update_res = mysqli_query($mysqli, $update_sql) or die(mysqli_error($mysqli));    //add to session for the sake of having the form fields filled in next time  $_SESSION["$k"] = $v;  session_write_close();		  } else {  //sanitize them    $v = trim(htmlspecialchars(mysqli_real_escape_string($mysqli,check_chars_mailto($v))));  		  if(empty($v)) {		  continue;		}  if(eregi("email",$k)) {		if($_POST["email"] != $_POST["confirmEmail"]) {	  header("Location: account_management.php5?error=ef");	  exit();	}		$_SESSION["$k"] = $v;	  session_write_close();  //if email address/username being changed, check for pre-existing account with new address/username  $check_sql = "SELECT id FROM aromaMaster WHERE email='".$v."'";  $check_res = mysqli_query($mysqli, $check_sql) or die(mysqli_error($mysqli));  if(mysqli_num_rows($check_res) >= 1) {	//duplicate entry	mysqli_free_result($check_res);	header("Location: account_management.php5?error=email");	exit;  }  } else if(eregi("username",$k)) {			if($_POST["username"] != $_POST["confirmUsername"]) {	  header("Location: account_management.php5?error=ef");	  exit();	}  $v = trim(htmlspecialchars(mysqli_real_escape_string($mysqli,check_chars_mailto($v))));	//check for pre-existing account with same username	  $check_sql = "SELECT id FROM aromaMaster WHERE username='".$v."'";  $check_res = mysqli_query($mysqli, $check_sql) or die(mysqli_error($mysqli));  if(mysqli_num_rows($check_res) >=1 ) {	//duplicate entry	mysqli_free_result($check_res);	header("Location: account_management.php5?error=username");	exit;  }  	} else if(eregi("newPassword",$k)) {			if(($_POST["newPassword"] != $_POST["confirmNewUsername"]) || ($_POST["oldPassword"] != $_POST["confirmOldPassword"])) {	  header("Location: account_management.php5?error=ef");	  exit();	}  $v = trim(htmlspecialchars(mysqli_real_escape_string($mysqli,check_chars_mailto($v))));	//check for pre-existing account with same username	  $check_sql = "SELECT id FROM aromaMaster WHERE id='".$_SESSION["userid"]."'";  $check_res = mysqli_query($mysqli, $check_sql) or die(mysqli_error($mysqli));  if(mysqli_num_rows($check_res) >=1 ) {	//duplicate entry	mysqli_free_result($check_res);	header("Location: account_management.php5?error=username");	exit;  }} else {		$v = trim(htmlspecialchars(check_chars_mailto(mysqli_real_escape_string($mysqli,$v))));  //create query  $update_sql = "UPDATE aromaMaster SET ".$k." = '".$v."' WHERE id = '".$_SESSION["userid"]."'";  $update_res = mysqli_query($mysqli, $update_sql) or die(mysqli_error($mysqli));$_SESSION["$k"] = $v;	  session_write_close();	  header("Location: account_management.php5?res=suc");	  exit();}  }  }  mysqli_close($mysqli);  ?>

[justsomeguy]

One thing is that you're using session_write_close in the loop, so the first time you write to the session you immediately close it, the others aren't going to be written. Another issue is that if the field name does not contain "confirm", "old", "address", "town", "city", "postcode", "email", "username", or "newPassword", then it gets to the last else, tries to update the table and then immediately redirects without looping through the rest of them. So if you have a named submit button, and the name isn't one of those other values, then it's just going to redirect away when it gets to the submit button.I'm not sure about this approach in general, you're updating each individual field with a query. You could just use one query to update the entire table, but you're sending one query per field. I'm not sure about the idea of redirecting in the loop either, if a few fields are correct you'll update those and then if you find a problem you'll redirect away, so it might update a few fields but not all of them. I typically do an all or none thing, if there are errors I don't update anything. I guess that's preference though. But with this it also might be the case that several of them are correct and able to be updated, but it redirects before it gets to them because it finds a problem sooner.Other than that stuff, add some debugging statements to figure out where the execution is going. Since you're redirecting it will probably be easier to use the error log for debugging instead of just printing stuff, but add some debugging statements just so that you can figure out where the execution flow is going and exactly what the script is doing.

[chibineku]

Hm, I guess it would be easier just doing it at once...then the handler is almost identical to my registration handler. I liked the idea of the loop because it seemed like it'd be efficient, but it's the same amount of code, if not more. How would you handle the fact that someone might not want to change their password, but I have to allow that as a possibility. Would you make a separate form and handler for changing password, rather than test if the new password field is set, if it matches the confirm new password field, then check if the old password is right and that the old password confirm field matches it?edit: yeah, putting session_write_close in a loop... Just when I'm starting to think I'm getting a grasp of this stuff I go and do something stupid like that.

[justsomeguy]

I liked the idea of the loop because it seemed like it'd be efficient

You can have as much code as you want, the main inefficiency comes with database accesses. The code will be a lot more efficient in terms of time and resource usage if you only send one database query. It's the same thing with file access, something like opening a file or connecting to a database server and sending a query is going to be a bottleneck in performance way before inefficient code. You have to have *really* inefficient code before it would even approach the delay you get from sending only a single query. Traditionally, the three main areas programmers look at for finding bottlenecks are memory, CPU, and I/O. You'll hit a memory bottleneck any time your program is so large that it runs out of memory, so it's always good to try to use memory efficiently. You'll hit a CPU bottleneck any time you've got your code doing a lot of calculations in a loop or something like that. These days, memory is so large and CPUs are so fast that it's pretty difficult to hit either of those (although give yourself time, you'll manage to do it, everyone does). The I/O bottleneck is the killer. When you use any I/O device, like accessing a disk to read a file, or connecting to a database server, you're adding a lot of extra time (relatively speaking). Consider that it might take 10ms or so to connect to a database and send a query, or open a file. Now 10ms isn't a lot of time, you might think that's not such a big deal, that you would have to send 100 queries or so to even hit one second. But consider how fast a CPU works - if you have a 3GHz CPU then that thing is able to do 3 billion calculations every second. In those 10ms that it takes to work with an I/O device, you could have done 30 million CPU operations. And that's only with a single processor with a single core, not a 4-core or 8-core CPU like you would find in most dedicated web servers. So even though in absolute terms 10ms doesn't sound like a lot of time to expend on I/O, in reality you're wasting CPU time when you're waiting for I/O to come back. Instead of sending one query each time through the loop, you could just build your query in a string and then send it all at the end of the loop.

How would you handle the fact that someone might not want to change their password, but I have to allow that as a possibility.

Like I said, I typically build a query in a string variable and then send it at the end. So I'll check on the password stuff and if everything is good I'll just add that field to the query to update. Another issue is checking the current database values. If the user needs to enter their current password in order to change it then you'll need to send a query to check the password, so if you ever need to send a query to check it's probably going to be more efficient to send one query for all of the fields you may need to check, and then check each field from the result if necessary instead of sending another query for each field to check. That's not always possible, but it's just another little efficiency you may be able to do. It's even better if you can check to see if you even need to check existing fields and only send the query if you do.About the queries though, in my old style of just using the mysql extension like you're doing I would usually just keep a variable for all the fields to update. e.g.:

$update = '';if ($email != ''){  //validate, sanitize  if ($valid)	$update .= ($update != '' ? ',' /* add comma if necessary */ : '') . "email='{$email}'";}if ($password != ''){  //validate, sanitize  if ($valid)	$update .= ($update != '' ? ',' /* add comma if necessary */ : '') . "password='{$password}'";}

etc, at the end you get a string like "email='test@test.com', password='1234'" etc, so you can add that to the end of an update query to only update the fields you want. e.g.:$sql = 'UPDATE table SET ' . $update . ' WHERE id=123';The way I do it now is with a database class, which is even easier. I have the update method accept a table name and an array of field/value pairs, and the WHERE condition, so if I want to update a certain field I just add it to the array, the update method will take care of sanitizing everything. e.g.:

$update = array();if ($email != ''){  //validate  if ($valid)	$update['email'] = $email;}if ($password != ''){  //validate  if ($valid)	$update['password'] = $password;}if (count($update) > 0)  $db->update('table', $update, 'id=123');

The database class makes it a lot easier. This is the actual code I use when a user updates their own information on one of my applications. This is called through ajax so it gets included by another file that defines the database class and everything, but this is the actual code it runs. This will update a user's email address, first and last name, password, and up to 30 custom fields that the admin would have added to user profiles, plus it even checks for various system options that the admin would have set (emails optional, locking users from changing their name, etc), and it does all the validation and sends back error messages, etc. All that in about 70 lines of code, with plenty of whitespace:

$update = array();$update['email'] = form_var('email');$update['fname'] = form_var('fname');$update['lname'] = form_var('lname');$password = form_var('password');$conf_password = form_var('conf_password');$email_req = !$opts->get_opt('email_optional');if (!$opts->get_opt('lock_standard_fields')){  if ($update['email'] != '' && $db->num_rows("SELECT id FROM users WHERE email='" . $db->escape($update['email']) . "' AND id!={$sess->userid}"))  {	$response['errors'][] = array('id' => 'email', 'msg' => 'That email address belongs to another user.');	return;  }  if (($update['email'] != '' || $email_req) && !validate_email($update['email']))  {	$response['errors'][] = array('id' => 'email', 'msg' => 'That email address was not recognized as a valid format.');	return;  }}else{  // don't update standard fields if they're locked  unset($update['email']);  unset($update['fname']);  unset($update['lname']);}if ($password != ""){  if (strlen($password) < 4)  {	$response['errors'][] = array('id' => 'password', 'msg' => 'The password needs to be at least 4 characters long.');	return;  }  if ($password != $conf_password)  {	$response['errors'][] = array('id' => 'conf_password', 'msg' => 'The confirmation password did not match.');	return;  }  if (sha1($password) == $sess->userdata['password'])  {	$response['errors'][] = array('id' => 'password', 'msg' => 'You cannot use the same password you currently have.');	return;  }}$ufields = $uf->get_fields();for ($i = 0; $i < count($ufields); $i++){  if ($ufields[$i]->shown)  {	$update[$ufields[$i]->id] = form_var($ufields[$i]->id);  }}// TODO: need to validate user fieldsif ($password != ""){  $update['password'] = sha1($password);  $update['pw_last_change'] = time();}$db->update('users', $update, "id={$sess->userid}"); // update DB$sess->update($update); // update session$update['fullname'] = $sess->fullname;$update['fname'] = $sess->userdata['fname'];$update['lname'] = $sess->userdata['lname'];$update['email'] = $sess->userdata['email'];$response['update'] = $update;$response['success'] = true;

[chibineku]

Hey. Thanks for the really in depth reply. It's nice seeing how someone who has plenty experience does something. There are a lot of things in there that I would never have thought of - even building the query as you filter conditions, etc. A very simple and good idea. There are more that I don't know anything about. I haven't even started with OOPHP, and while I understand the principle of creating classes and methods, I don't know any of the ins and outs. It is definately on my to-do list, but for the current project time is of the essence. It would be enlightening to see your update, valid and escape methods (is $valid a method or just pseudo-code for string-valid?). Does form_var do any sort of escaping/validation or is that just to fetch the values without having to mush your fingers up typing $_POST[ ] over and over? I guess the feeling of loss and anxiety as I fail to grasp all of that is all right since I've only been doing PHP for 2 months.

[justsomeguy]

I was just using $valid is a boolean to say whether or not it's valid. e.g.:

<?phpclass tc_lms_database{  private $host = '';  private $user = '';  private $pass = '';  private $db = '';  private $conn = false;  public $sql_stmt = '';  public $params = false;  private $result = false;  private $rowset = false;  private $last_stmt = '';  /*  Constructor    Set the host, username, password, and database, then connect  */  function __construct($h = '', $u = '', $p = '', $d = '')  {    if ($h != '')    {      $this->host = $h;      $this->user = $u;      $this->pass = $p;      $this->db = $d;      $this->connect();    }  }  /*  connect    Connect to the database using the supplied information, trigger an error if there was a problem  */  function connect($h = '', $u = '', $p = '', $d = '')  {    if ($h != '')    {      $this->host = $h;      $this->user = $u;      $this->pass = $p;      $this->db = $d;    }    $this->conn = mysql_connect($this->host, $this->user, $this->pass) or trigger_error(mysql_error(), E_USER_ERROR);    if ($this->db != '')      mysql_select_db($this->db, $this->conn) or trigger_error($this->error(), E_USER_ERROR);  }  /*  reset    Reset the SQL statement and all parameters  */  function reset()  {    $this->sql_stmt = '';    $this->params = false;  }  /*  sql    Set the SQL statement  */  function sql($str)  {    $this->sql_stmt = $str;  }  /*  add_param    Add a parameter for the SQL statement, and optionally escape it.  Parameters should be added  in the same order that they appear in the SQL  */  function add_param($val, $escape = true)  {    if ($escape)      $val = "'" . $this->escape($val) . "'";    if (!$this->params)      $this->params = array();    $this->params[] = $val;  }  /*  exec  Execute a SQL statement and return the result.  If the argument is true, do not die on an error.  This returns a mysql result resource.  */  function exec($passthrough_errors = false)  {    $this->check_db();    if ($this->sql_stmt == '')      trigger_error('SQL statement not set', E_USER_ERROR);    if (is_array($this->params) && count($this->params))    {      $this->sql_stmt = vsprintf($this->sql_stmt, $this->params);      $this->params = false;    }    $this->last_stmt = $this->sql_stmt;    if ($passthrough_errors)      return mysql_query($this->sql_stmt);    else      return mysql_query($this->sql_stmt) or trigger_error($this->error(), E_USER_ERROR);  }  /*  get_row  Get a single row from the given result set  */  function get_row($result)  {    return mysql_fetch_assoc($result);  }  /*  select  Execute a SELECT statement, with an optional limit and page number.  This returns the  results as an array of rows, not as a mysql result resource.  e.g.:  $db->select(10, 3); // get the 10 items on page 3  */  function select($limit = 0, $page = 0)  {    $this->check_db();    if ($this->sql_stmt == '')      trigger_error ('SQL statement not set', E_USER_ERROR);    if (is_array($this->params) && count($this->params))    {      $this->sql_stmt = vsprintf($this->sql_stmt, $this->params);      $this->params = false;    }    if ($limit)    {      $this->sql_stmt .= ' LIMIT ';      if ($page)        $this->sql_stmt .= (($page - 1) * $limit) . ',';      $this->sql_stmt .= $limit;    }    $this->last_stmt = $this->sql_stmt;    $this->result = mysql_query($this->sql_stmt) or trigger_error($this->error(), E_USER_ERROR);    $this->rowset = array();    while ($row = mysql_fetch_assoc($this->result))    {      // convert null to empty string      foreach ($row as $k => $v)      {        if (is_null($v))          $row[$k] = '';      }      $this->rowset[] = $row;    }    mysql_free_result($this->result);    return $this->rowset;  }  /*  delete    Execute a DELETE statement with an optional limit  */  function delete($limit = 0)  {    $this->check_db();    if ($this->sql_stmt == '')      trigger_error('SQL statement not set', E_USER_ERROR);    if (is_array($this->params) && count($this->params))    {      $this->sql_stmt = vsprintf($this->sql_stmt, $this->params);      $this->params = false;    }    if ($limit)      $this->sql_stmt .= " LIMIT {$limit}";    $this->last_stmt = $this->sql_stmt;    return mysql_query($this->sql_stmt) or trigger_error($this->error(), E_USER_ERROR);  }  /*  insert  Insert a record into the given table.  The $params variable should be an associative  array of column names and values.  e.g.;  $db->insert('table_name', array('id' => 10, 'username' => 'Joe'));  */  function insert($table, $params)  {    $this->check_db();    if (!is_array($params) || count($params) == 0)      return false;    $names = '';    $values = '';    foreach ($params as $k => $v)    {      $names .= "{$k},";      $values .= "'" . $this->escape($v) . "',";    }    $names = substr($names, 0, -1);    $values = substr($values, 0, -1);    $this->last_stmt = "INSERT INTO {$table} ({$names}) VALUES ({$values})";    return mysql_query("INSERT INTO {$table} ({$names}) VALUES ({$values})", $this->conn) or trigger_error($this->error(), E_USER_ERROR);  }  /*  update  Update a record in the given table.  Similar to insert, but the third parameter is the expression  to use in the WHERE clause.  e.g.:  $db->update('table_name', array('username' => 'John'), 'id=10');  */  function update($table, $params, $where = '')  {    $this->check_db();    if (!is_array($params) || count($params) == 0)      return false;    $cols = '';    foreach ($params as $k => $v)      $cols .= "{$k}='" . $this->escape($v) . "',";    $cols = substr($cols, 0, -1);    if ($where != '')      $where = " WHERE {$where}";    $sql = "UPDATE {$table} SET {$cols} {$where}";    $this->last_stmt = $sql;    return mysql_query($sql, $this->conn) or trigger_error($this->error(), E_USER_ERROR);  }  /*  escape    Escape a string for use in a query  */  function escape($str)  {    return mysql_real_escape_string($str, $this->conn);  }  /*  strip    Remove selected characters from a string  */  function strip($str)  {    $replace = array(      "'",      '"',      '-',      ';'    );    return str_replace($replace, '', $str);  }  /*  insert_id    Get the last autoassigned ID  */  function insert_id()  {    return mysql_insert_id($this->conn);  }  /*  num_rows    Get the number of rows produced by the given SQL statement  */  function num_rows($sql)  {    return mysql_num_rows(mysql_query($sql, $this->conn));  }  /*  check_db    Make sure the database has been connected to  */  function check_db()  {    if ($this->conn == false)      die('Database not initialized');  }  /*  error    Generate the last error message  */  function error()  {    return 'Database Error #' . mysql_errno($this->conn) . ': ' . mysql_error($this->conn) . '.  SQL Query: ' . $this->last_stmt;  }  /*  get_last_sql    Get the last SQL statement executed, after parameter replacement  */  function get_last_sql()  {    return $this->last_stmt;  }}?>

Here are a couple examples creating and using it:

require_once 'class.db.php';$db = new tc_lms_database($config['db_host'], $config['db_user'], $config['db_pass'], $config['db_name']);

register user (insert, num_rows, insert_id)

$insert = array();$insert['username'] = form_var('username');$insert['email'] = form_var('email');$insert['fname'] = form_var('fname');$insert['lname'] = form_var('lname');$password = form_var('password');$conf_password = form_var('conf_password');if ($db->num_rows("SELECT id FROM users WHERE username='" . $db->escape($insert['username']) . "'")){  $response['errors'][] = array('id' => 'reg_username', 'msg' => 'That username belongs to another user.');  return;}if (strlen($insert['username']) < 3){  $response['errors'][] = array('id' => 'reg_username', 'msg' => 'The username needs to be at least 3 characters long.');  return;}if (strlen($password) < 4){  $response['errors'][] = array('id' => 'reg_password', 'msg' => 'The password needs to be at least 4 characters long.');  return;}if ($password != $conf_password){  $response['errors'][] = array('id' => 'conf_password', 'msg' => 'The confirmation password did not match.');  return;}$insert['password'] = sha1($password);$insert['active'] = 1;$insert['date_registered'] = time();$db->insert('users', $insert);$new_uid = $db->insert_id();

search (select with paging)

if ($limit == 0)  $limit = $opts->get_opt('recs_per_page');if ($sort == '')  $sort = 'queue_time';if ($dir != 'DESC')  $dir = 'ASC';$sql = 'SELECT h.queue_time, h.send_email, h.subject, h.body, u1.username AS send_username, u2.username AS sender FROM mail_history AS h ';$sql .= 'LEFT JOIN users AS u1 ON u1.id=h.send_to ';$sql .= 'LEFT JOIN users AS u2 ON u2.id=h.sender ';$sql .= " ORDER BY {$sort} {$dir}";$response['totalCount'] = $db->num_rows($sql);$db->sql($sql);$response['data'] = $db->select($limit, floor($start / $limit) + 1);

delete, query parameters

// check for instructor classes$db->sql('SELECT COUNT(id) AS num FROM standup_instance WHERE instructor=%d');$db->add_param($id, false);$check = $db->select();if ($check[0]['num'] > 0){  $response['errors']['reason'] = 'This user is marked as the instructor for ' . $check[0]['num'] . ' classes.  You cannot delete an instructor assigned to classes.';  return;}$db->sql('DELETE FROM users WHERE id=%d');$db->add_param($id, false);$db->delete();

more select and parameters

$response['data'] = array();$db->sql('SELECT id, title FROM content_categories ORDER BY disp_order, title');$cats = $db->select();foreach ($cats as $cat){  $cur_cat = array('id' => 'cat_' . $cat['id'], 'title' => '<span style="font-weight: bold;">' . $cat['title'] . '</span>');  $response['data'][] = $cur_cat;  $db->sql('SELECT id, title FROM content WHERE category=%d ORDER BY disp_order, title');  $db->add_param($cat['id'], false);  $content = $db->select();  foreach ($content as $c)  {	$cur = array('id' => 'cid_' . $c['id'], 'title' => '  ' . $c['title']);	$response['data'][] = $cur;  }}$response['success'] = true;

The parameters and add_param method are for auto-sanitizing everything. Instead of doing something like this:$result = mysql_query("SELECT * FROM users WHERE username='" . mysql_real_escape_string($username) . "' AND email='" . mysql_real_escape_string($email) . "'");I would do this:$db->sql('SELECT * FROM users WHERE username=%s AND email=%s');$db->add_param($username);$db->add_param($email);$user = $db->select();The string format is common in C-style programming, %s means a string and %d means a number. add_param also takes false as a second parameter if you don't want to sanitize it (if it's a number and you already did):$db->sql('SELECT * FROM users WHERE id=%d');$db->add_param(intval($id), false);$user = $db->select();By default add_param also wraps the value in quotes, that's why I didn't put quotes around the %s. More information about that string style here:http://www.php.net/manual/en/function.sprintf.phpNote that the select method returns an array, not a result resource. For something like this:$db->sql('SELECT * FROM users WHERE id=%d');$db->add_param(intval($id), false);$user = $db->select();If it finds a user, the fields will be in $user[0], e.g. $user[0]['username'], $user[0]['email'], etc. If it finds a second user, it will be in $user[1], etc. When I'm checking for a single record I usually do something like this:

$db->sql('SELECT * FROM users WHERE id=%d');$db->add_param($id, false);$curr = $db->select();if (!$curr){  $response['errors']['reason'] = 'The user record to update was not found.';  return;}$curr = $curr[0];// now $curr is the single record instead of $curr[0]

If you're returning multiple records, having them already in an array makes it easier to loop through them or count them without using the other mysql functions.The exec method in the db class will not return an array, it will return a mysql resource, it can also be set not to quit on an error (it will trigger an error and quit if select, insert, update, or delete fails). I've had to use exec for some more complex things that didn't neatly fit into one of the other methods. That's the basics though, feel free to ask if you have questions.

I guess the feeling of loss and anxiety as I fail to grasp all of that is all right since I've only been doing PHP for 2 months.

Don't worry about it, it comes with experience. I've been doing this stuff professionally for years and have a degree in computer science, this isn't stuff I picked up after 2 months.