I've never really understood if it's ok to do mysql inserts / updates by chunking through an array and using variable variables - I seem to remember Laserlight advised against it but I cant find the post

so here's my code - please comment!

// build array that I'll use for other things so ignore explicit titles
$variables = array(
'adm_fname' => 'First name',
'adm_lname' => 'Last name',
'adm_uname' => 'Username',

'adm_co' => 'Company',

'adm_priv' => 'Access level',
'adm_tel' => 'Telephone',
'adm_mobile' => 'Mobile',
'adm_email' => 'Email'
);

// now build sql strings

if ($adm_id) {
    $sql = "UPDATE admin_usr SET ";
    $v=0;
    foreach ($variables as $variable => $title) {
    if($v) $sql.= ', ';
     $sql.= "$variable = '".mysql_escape_string(trim($_REQUEST[$variable]))."' ";
    $v++;
    }
    $sql.=" WHERE adm_id=$adm_id";


  } else {

$sql = "INSERT INTO admin_usr ( ";

$v=0;
foreach ($variables as $variable => $title) {
    if($v) $sql.= ', ';
     $sql.= $variable;
    $v++;
  }
  $sql.= " ) VALUES ( ";

  $v=0;
  foreach ($variables as $variable => $title) {
    if($v) $sql.= ', ';
     $sql.= "'".mysql_escape_string(trim($_REQUEST[$variable]))."'";
    $v++;
  }
$sql.= " )";
          }

// then do the mysql query..

    its like working, but not safe.

    and you get that $adm_id value ?

    lets test if the user fill all the forms.

    if it is an update form, there might have fields that are unique.

      I don't see any variable variables.

      Incidentally, since you only ever use the array keys when you're looping

      foreach(array_keys($variables) as $variable)

      would save the redundant $title variable from floating around. And since you're using an array, there might be some array functions that would simplify what you're doing. E.g.,

      $v=0;
      foreach ($variables as $variable => $title) {
          if($v) $sql.= ', ';
           $sql.= $variable;
          $v++;
        } 

      could be replaced by

      $sql .= join(", ", array_keys($variables));

      and

      $v=0;
      foreach ($variables as $variable => $title) {
          if($v) $sql.= ', ';
           $sql.= "'".mysql_escape_string(trim($_REQUEST[$variable]))."'";
          $v++;
        } 

      by

      $request_values = array_intersect_key($_REQUEST, $variables));
      $request_values = array_map('mysql_escape_string',(array_map('trim', $request_values));
      $request_values = array_map(create_function('$a','return "\'$a\'";'), $request_values);
      $sql .= join(", ", $request_values);
      

      although it would be tidier to have a distinct function to do that trimming, escaping, and quoting, and write

      $sql .= join(', ', array_map('prep_variable', array_intersect_key($_REQUEST, $variables)));

      And then of course there's PDO: then the escaping and quoting steps wouldn't be needed.

        many thanks for replies

        sorry djjjozi I was only doing a kind of pseudocode to show the structure - I fully intend to do user entry checking and in-depth validation with another function

        Weed (if I may call you that ; ) ) - firstly I stupidly managed to edit out the bit with the variable variable

        foreach ($variables as $variable => $title) {
        $$curvar=  mysql_escape_string(trim($_REQUEST[$variable]));
        }

        that was to set each value explicitly but maybe I dont need that..

        your other comments are worth their weight in gold - really appreciate it

        I'm sick of typing lengthy strings of variables numerous times for updates/inserts and these are the steps I want to go through to make it into a function (and even work towards making it into a class - if I can just get my brain around writing those)

          Write a Reply...