views:

55

answers:

3

I have a a php page which updates a mySql database it works fine on my mac (localhost using mamp)

I made a check if its the connection but it appears to be that there is a connection

 <?php require_once('connection.php'); ?>

 <?php  
  $id = $_GET['id'];
  $collumn = $_GET['collumn'];
  $val = $_GET['val'];
 // checking if there is a connection
 if(!$connection){
     echo "connectioned failed";
   }
  ?>


 <?php 
    $sqlUpdate = 'UPDATE plProducts.allPens SET '. "{$collumn}".' = '."'{$val}'".' WHERE allPens.prodId = '."'{$id}'".' LIMIT 1';
    mysql_query($sqlUpdate);
    // testing for errors
   if ($sqlUpdate === false) {
      // Checked this and echos NO errors.
     echo "Query failed: " . mysql_error();
    }

if (mysql_affected_rows() == 1) {
  echo "updated";
} else {
  echo "failed";
}?>

In the URL i pass in parameters and it looks like this: http://pathToSite.com/updateDB.php?id=17&amp;collumn=prodid&amp;val=4

Maybe this has to do with the hosting? isn' t this simple PHP mySql database updating? what can be wrong here?

Why on localhost it does work?

Why on live server it doesn't?

+1  A: 

Most mysql functions return FALSE if they encounter an error. You should check for error conditions and if one occurs, output the error message. That will give you a better idea of where the problem occurred and what the nature of the problem is.

It's amazing how many programmers never check for error states, despite many examples in the PHP docs.

$link = mysql_connect(...);
if ($link === false) {
    die(mysql_error());
}

$selected = mysql_select_db(...);
if ($selected === false) {
    die(mysql_error());
}

$result = mysql_query(...);
if ($result === false) {
    die(mysql_error());
}
Bill Karwin
Neglecting to check for errors is an antipattern I call *See No Evil*.
Bill Karwin
Thanks, Look like the error is only visible in the error logs? how can i echo them? to see them in the browser.
adardesign
You can `echo` in addition to passing the value to `die()`. But FYI I think it's good practice to send error diagnostics to the log so you can read it but the public users of your web app cannot. Revealing too much information about your code or your db can make it easier for malicious users to exploit your system.
Bill Karwin
@Bill thanks, Please see updated code. I don't see any error (no Error is echoed to the browser.
adardesign
@adardesign: No, you want to check the value returned by the function, not the original SQL query you passed to it.
Bill Karwin
+1  A: 

Let's start with troubleshooting your exact problem. Your query is failing for some reason. We can find out what that problem is by checking what comes back from mysql_query, and if it's boolean false, asking mysql_error what went wrong:

$sh = mysql_query($sqlUpdate);
if($sh === false) {
    echo "Query failed: " . mysql_error();
    exit;
}

You have other problems here. The largest is that your code suffers from an SQL Injection vulnerability. Let's say your script is called foo.php. If I request:

foo.php?collumn=prodId = NULL -- 

then your SQL will come out looking like:

UPDATE plProducts.allPens SET prodId = NULL -- = "" WHERE allPens.prodId = "" LIMIT 1

-- is an SQL comment.

I just managed to nuke all of the product IDs in your table.

The most effective way to stop SQL injection is to use prepared statements and placeholders. The "mysql" extension in PHP doesn't support them, so you'd also need to switch to either the must better mysqli extension, or the PDO extension.

Let's use a PDO prepared statement to make your query safe.

// Placeholders only work for *data*.  We'll need to validate 
// the column name another way.  A list of columns that can be
// updated is very safe.
$safe_columns = array('a', 'b', 'c', 'd');
if(!in_array($collumn, $safe_columns))
    die "Invalid column";
// Those question marks are the placeholders.
$sqlUpdate = "UPDATE plProducts.allPens SET $column = ? WHERE allPens.prodId = ? LIMIT 1";
$sh = $db->prepare($sqlUpdate);
// The entries in the array you pass to execute() are substituted
// into the query, replacing the placeholders.
$success = $sh->execute(array( $val, $id ));
// If PDO is configured to use warnings instead of exceptions, this will work.
// Otherwise, you'll need to worry about handling the exception...
if(!$success)
    die "Oh no, it failed!  MySQL says: " . join(' ', $db->errorInfo());
Charles
+1  A: 

Your call to mysql_query() is faulty; you're checking the contents of the variable you're passing in but the function call doesn't work that way. It returns a value which is what you should check. If the query failed, it returned false. If it returns data (like from a SELECT) it returns a resource handle. If it succeeds but doesn't return data (like from an INSERT) it returns true.

You also have some problems constructing your SQL. @Charles mentions SQL injection and suggests prepared statements. If you still want to construct a query string, then you need to use mysql_real_escape_string(). (But I would recommend you read up on the mysqli extension and use those functions instead.)

Secondly, you're concatenating strings with embedded substitution. This is silly. Do it this way instead:

 $sqlUpdate = 'UPDATE plProducts.allPens SET '.$collumn.' = \''.$val.'\' 
        WHERE allPens.prodId = '.intval($id).' LIMIT 1';

If you must accept it in the querystring, you should also check that $collumn is set to a valid value before you use it. And emit and error page if it's not. Likewise, check that $id will turn into a number (use is_numeric()). All this is called defensive programming.

staticsan