UPDATE form not updating

No errors, just doesn't update - multiple 'if' statements to blame?

[HBird]

I'm a beginner with PHP but thought I understood the following code - until part of it decided not to work!

I'm building an all-purpose HTML form on a single page (input.php). By default it adds a new entry, but if the url is "input.php?id=2" then it populates the form with existing data for updating record 2.

All of that works great. But when I update the values and hit the Update Record button, I get the "success" message but the fields are not updated in SQL.

My guess is that something is conflicting with the script that populates the form. I've tried to prevent this by using unique variables in the update form (e.g. $ud_id instead of $id), but to no avail.

Any help anyone can offer would be greatly appreciated!

<?php

$id = $_GET['id']; // Pulls ID value from URL if there is one, for use with third "if" statement.

if(isset($_POST['add'])) // If the "Add Record" button has been pushed, insert data into database.

{
$con = mysql_connect("localhost","*****","*****");
mysql_select_db("*****", $con);
$sql="INSERT INTO Test (title, city) VALUES ('$_POST[title]','$_POST[city]')";
if (!mysql_query($sql,$con)) die('Error: ' . mysql_error());
echo "Record successfully added to database. <a href=\"input.php\">Return to form.</a>";
mysql_close();
}

else if(isset($_POST['update'])) // If the "Update Record" button has been pushed, update database with new values.

{
$con = mysql_connect("localhost","*****","*****");
mysql_select_db("*****", $con);
$sql = "UPDATE Test SET title='$ud_title', city='$ud_city' WHERE id=$ud_id";
$result = mysql_query($sql);
echo "Record successfully updated. <a href=\"input.php\">Return to form.</a>";
}

else if($id) // If ID value provided in URL, populate update form with existing values.

{
$con = mysql_connect("localhost","*****","*****");
mysql_select_db("*****", $con);
$sql = "SELECT * FROM Test WHERE id=$id";
$result = mysql_query($sql);
$id = mysql_result($result, $i, "id");
$city = mysql_result($result, $i, "city");
$title = mysql_result($result, $i, "title");
?>
<h2>Update Form</h2>
<form method="post" action="input.php">
<input type="hidden" name="ud_id" value="<?php echo $id; ?>">
<table width="30%" border="0" cellpadding="5">
<tr>
<td>Title:</td>
<td><input type="text" name="ud_title" value="<?php echo $title; ?>"></td>
</tr>
<tr>
<td>City:</td>
<td><input type="text" name="ud_city" value="<?php echo $city; ?>"></td>
</tr>
</table>
<input type="submit" name="update" value="Update Record"></form>
<?
}

else

{
?>
<h2>Insert Form</h2>
<form method="post" action="input.php">
<table width="30%" border="0" cellpadding="5">
<tr>
<td>Title:</td>
<td><input type="text" name="title"></td>
</tr>
<tr>
<td>City:</td>
<td><input type="text" name="city"></td>
</tr>
</table>
<input type="submit" name="add" value="Add Record"></form>
<?php
}
?>

[blang]

Ok, I see the problem you're talking about, it's right here:

$sql = "UPDATE Test SET title='$ud_title', city='$ud_city' WHERE id=$ud_id"; 

Where do those variables come from? They don't exist at all; you have to pull the values from the $_POST superglobal as you did in your INSERT statement. Since there is no value in $ud_id, the statement fails and you don't have any error handling to catch it, so it prints "success". You should mirror the behavior in your INSERT statement and follow up with a call to mysql_error(), or at the very least echo out your $sql variable. Good troubleshooting techniques. Of course when you 'go live' with the page be sure to handle the errors in another manner, such as logging to the server, and fail gracefully for the user's sake.

Now, another glaring problem I see is in the creation of the update form itself; do you actually retrieve any data and populate the form? Notice this code:

$id = mysql_result($result, $i, "id"); 
$city = mysql_result($result, $i, "city"); 
$title = mysql_result($result, $i, "title");  

Where is $i declared? What is it's purpose in the function call? It looks like you're getting lucky and the uninitialized variable is being passed as zero so the function call works because that parameter refers to the row index, and since there is only one record returned (we hope), it's referencing the first record (index zero). At any rate, that could be changed to a single call to mysql_fetch_assoc() and pull down an associative array of the data instead. I only recommend the use of mysql_result() if you have a single column to retrieve; otherwise use one of the mysql_fetch_* functions.

Also, please be conscience of the issue of SQL injection [google.com]. Be sure to properly validate and escape your incoming data! Even the $_GET['id'] value. Anything that you intend to even come near the database needs to be properly validate and escaped. The use of a simple call to mysql_real_escape_string() [php.net] on each string you pass to the database is a start (be sure to read that page, including the examples shown). Otherwise, type casting and using parameterized queries [google.com] or stored procedures [google.com] where you can are ideal.

Furthermore, you unnecessarily repeat code. You should have a single call to the database at the top of the script; you don't need to have 3 separate calls depending on your script logic. Granted, the initial display of the empty form doesn't need a connection, but some logic could dictate that. Also, you only need a single form, you don't have to have two forms based on whether or not one of them has update data. Simply do something like this:

<input name="test" id="test" value="<?php echo ( isset($test) ) ? $test : '';?>"> 

If the value $test exists (as it would if it was retrieved from the database), you'll have a value output there. That logic by the way is called a ternary operator; it's a shortcut if...else statement that I find very handy. Of course the submit button element value could be adjusted based on logic as well.. for example if the $_GET['id'] variable is set.

[HBird]

Thanks for your thoughtful reply, blang, I appreciate it.

The $ud_ variables come from the update form:

<input type="text" name="ud_title" value="<?php echo $title; ?>">

Or so I thought anyway. Is this not sufficient for defining $ud_title as a variable?

Excellent point on the error line - no idea why I left it out on that form. And I got this error: "Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1". I'm not seeing what this could be referring to - do you?

Yes, data is retrieved from the database and the update form is populated correctly.

Thanks for the great tips on making my code more efficient - I will definitely apply those once I get the form working. The reason for the inefficiencies is that I'm a beginner so I start out by using what I know (and what worked elsewhere). The $i comes from the script I use to view results from the database using loops - I can see that it has no use here, though.

As for security, the form is a tool for only me to update the database - I've learned a bit about SQL injection and it has me paranoid enough to not make a public form until I'm sure I've made it secure. Your links will be very helpful towards achieving that.

Thanks again - and I hope you or someone else can shed some light on the problem! I still can't figure out what's going wrong with the update form. Evidently the problem is that it doesn't "see" the $ud_title type variables, but I don't know why.

[blang]

HBird> You're welcome. The issue, as I pointed out, still lies in the fact that you are using uninitialized (empty) variables in your UPDATE statement. If the update form HTML has the correct variable data, that's fine. It's not being carried over to the receiving script, because you still need to access that data via the $_POST superglobal array, as you've done in the INSERT statement.

To test this, at the top of your script add this single line:

print_r($_POST);

This will display the POST data and let you see what's coming across in the HTTP POST to the server.

Once you see what's going on, you should realize that you need to access the $_POST superglobal array. Add these lines prior to the UPDATE statement, e.g.

// run each POST element through this loop 
foreach ($_POST AS $k => $v) { 
 // ..trim, slash, escape... explained below... 
 $$k= mysql_real_escape_string( stripslashes( trim( $v ) ) ); 
} 
// and then your original update 
$sql = "UPDATE Test SET title='$ud_title', city='$ud_city' WHERE id=$ud_id"; 

What those couple of lines do is, they take the $_POST data, trim any extraneous spaces off the end of it, strip off any slashes added by the dread magic quotes [php.net], and then run mysql_real_escape_string() on it. Then the entire result of that is reassigned to a variable variable with the name of the actual POST field. In other words, $_POST['ud_title'] becomes the local variable $ud_title, etc. This isn't the most efficient code, but it's good in a pinch. Good luck!

[HBird]

Hooray, it works! Thank you, thank you, thank you!