Re: Code Review PLEASE !!!

Yes I agree I need some validation, dunno whether to do server or client side validation. I don't think the fleet_id example will be a problem though as this is retrieved from the database where the field is an int. Thanks for your feedback Matt ----- Original Message ----- From: "Jordan S. Jones" <list@racistnames.com> To: "Matthew Oatham" <matthewoatham@hotmail.com>; <php-general@lists.php.net> Sent: Monday, April 05, 2004 11:56 PM Subject: Re: [PHP] Code Review PLEASE !!! > Wells first of all, you are going to want better form input validation. > For Example: > > foreach ($_POST['fleet_id'] as $key => $value) { > $fleetCode = $_POST['fleet_code'][$key]; > $historyUrl = $_POST['history_url'][$key]; > $downloadUrl = $_POST['download_url'][$key]; > mysql_query("UPDATE imp_fleet SET fleet_code = '$fleetCode', history_url = '$historyUrl', download_url = '$downloadUrl' WHERE fleet_id = $value") or die (mysql_error()); > } > > > Are you sure that $_POST['fleet_id'] is valid? or even a number? > > What happens with $_POST['fleet_id'] == '1 = 1'?? Well, long story > short, imp_fleet has no more records. > > Just a simple example of a huge problem. > > Jordan S. Jones > > Matthew Oatham wrote: > > >Hi, > > > >I am a newbie PHP programmer, I have some code that works but I want some tips on how I an Improve my code, i.e. should I be doing my updates / deletes on same php page as the display page, am I using transactions correctly, am I capturing SQL errors correctly am I handling form data as efficient as possible? > > > >My code displays some information from a database and gives users the chance to delete or edit any field and is as follows: > > > ><? > > > >include ("../db.php"); > > > >$acton = $_POST['action']; > > > >if ($action == "update") { > > if (isset($_POST['delete'])) { > > $deleteList = join(', ', $_POST['delete']); > > } > > > > //Enter info into the database > > mysql_query("begin"); > > foreach ($_POST['fleet_id'] as $key => $value) { > > $fleetCode = $_POST['fleet_code'][$key]; > > $historyUrl = $_POST['history_url'][$key]; > > $downloadUrl = $_POST['download_url'][$key]; > > mysql_query("UPDATE imp_fleet SET fleet_code = '$fleetCode', history_url = '$historyUrl', download_url = '$downloadUrl' WHERE fleet_id = $value") or die (mysql_error()); > > } > > if ($deleteList) { > > mysql_query("DELETE FROM imp_fleet WHERE fleet_id IN($deleteList)") or die (mysql_error()); > > } > > if (mysql_error()) { > > echo ("There has been an error with your edit / delete request. Please contact the webmaster"); > > mysql_query("rollback"); > > } else { > > mysql_query("commit"); > > } > >} > > > >?> > ><html> > ><head> > > <title></title> > ></head> > ><body> > ><form name="edit" method="post"> > ><h1>Edit / Delete Fleet</h1> > > <table> > > <tr> > > <td>Fleet Code</td> > > <td>Download URL</td> > > <td>History URL</td> > > <td>Delete</td> > > </tr> > ><? > >$sql = mysql_query("SELECT fleet_id, fleet_code, download_url, history_url FROM > > imp_fleet"); > > > >if (mysql_num_rows($sql) > 0) { > >while ($row = mysql_fetch_array($sql)) { > >?> > > <tr> > > <td><input type="text" name="fleet_code[]" value="<?=$row['fleet_code']?>"><input type="hidden" name="fleet_id[]" value="<?=$row['fleet_id']?>"></td> > > <td><input type="text" name="download_url[]" value="<?=$row['download_url']?>"></td> > > <td><input type="text" name="history_url[]" value="<?=$row['history_url']?>"></td> > > <td><input type="checkbox" name="delete[]" value="<?=$row['fleet_id']?>"></td> > > </tr> > ><? > >} > >} > >?> > > <tr> > > <td colsapn="4"> > > <table> > > <tr> > > <td><input type="hidden" name="action" > > value="update"><input type="reset" value="cancel"></td> > > <td colspan="2"><input type="submit" > > value="submit"></td> > > </tr> > > </table> > > </td> > > </tr> > > </table> > ></form> > ></body> > ></html> > > > >Thanks for your time and feedback. > > > >Matt > > > > > > -- > PHP General Mailing List (http://www.php.net/) > To unsubscribe, visit: http://www.php.net/unsub.php > >