Re: [PHP] Code Review PLEASE !!!
always do serverside - that the only secure option - use clientside (in addition) as a favour to the user to avoid repeated page requested to fill the form in correctly. (or in order to rearrange data before submitting to server) Matthew Oatham wrote: 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 -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Review PLEASE !!!
Matthew Oatham wrote: Hi, only use double quotes (") if you want to have variables interpolated e.g. $myVal = 'its amazing'; $x = "wow $myVal"; think out about the way you layout your code - it helps when you come back to it 12 months later ;-) sanitize all incoming variables (POST/GET/COOKIE) by changing their type or performing some other function on them to make them safe. writing: (int) $someVar; means make $someVar an integer; which has the nice side affect of turn ALL strings into 0 (apart from those beginning with a number in which case it keeps the numbers but that is quite sane I think) limit your use of temporary variables e.g: don't create $action if you are only going to use it on the next line - its a performance hit and looks unelegant - a good place for temp vars is to increase readability in your code, more often than not, I feel, readability is king. DO: if ($_POST['action'] == 'update') { NOT: $action = $_POST['action']; if ($action == 'update') { DO: $action = perform_check_on_var( $_POST['action'], array($arg1, $arg3), $arg2 ); if ($action == 'update') { speaking of readability - comment you code; especially the bit the make your eyes water! google around for stuff like phpDoc or phpDocumentor; that will at the very least give you good tips on what to stuff put in your comments and how to define them (getting into this habit pays dividends later when documentation generation come into play - for which tools like phpDocumentor are written ;-) ... all kinda based on javaDoc (something to google ;-) as far as I can see. if you are just starting out and its feasable why not give php5 a go - its feature complete and quite stable (RC1 currently); alot of the new features are really quite cool! besides as you learn you will not have to step over to a new version (which in itself can be a learning experience!). BTW: there is nothing wrong with having one page handle all updates/deletes etc - although becareful of filesize, longfile are a pain to edit/maintain - in such case you maybe want to create functions of the update and delete code and call them from your main script. At the end of the day its a matter of preference and application design choices which lead to one or the other scenario. try googling for 'Model View Controller PHP' - hopefully you'll get some useful code/examples of request processing control which uses single entry points (i.e. a single page) to perform all actions. lastly I made a few slight alterations to your example script: include ('./db.php'); if ($_POST['action'] == 'update') { //Enter info into the database mysql_query("begin"); if (isset($_POST['delete']) && @count($_POST['delete'])) { foreach ($_POST['delete'] as $k => $val) { (int) $_POST['delete'][ $k ]; if (! $_POST['delete'][ $k ]) { unset($_POST['delete']); } } if ($deleteList = join(', ', $_POST['delete'])) { mysql_query("DELETE FROM imp_fleet WHERE fleet_id IN($deleteList)") or die (mysql_error()); } } else { foreach ($_POST['fleet_id'] as $k => $val) { $fleetCode = (int) $_POST['fleet_code'][ $k ]; if (! $fleetcode) { continue; } $historyUrl = str_replace("'", "''", $_POST['history_url'][ $k ]); $downloadUrl = str_replace("'", "''", $_POST['download_url'][ $k ]); mysql_query("UPDATE imp_fleet SET fleet_code = '$fleetCode', history_url = '$historyUrl', download_url = '$downloadUrl' WHERE fleet_id = $val") 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'); } } ?> -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Review PLEASE !!!
* Thus wrote Matthew Oatham ([EMAIL PROTECTED]): > 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? > > >if (isset($_POST['delete'])) { > $deleteList = join(', ', $_POST['delete']); be careful here! if I add to the _POST data something like &delete%5B%5D=%29%20OR%281 then I just deleted all your records. > //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]; I would set up your array's so they are a little more readable and dont rely on autoincrementing array key's. So your loop would look something like this: foreach($_POST['fleets'] as $fleet_id => $fleet) { echo $fleet_id; // the fleet_id echo $fleet['code']; echo $fleet['urls']['history']; echo $fleet['ulrs']['download']; ... > mysql_query("UPDATE imp_fleet SET fleet_code = '$fleetCode', history_url = > '$historyUrl', download_url = '$downloadUrl' WHERE fleet_id = $value") or die > (mysql_error()); escape your data that is untrusted, see http://php.net/mysql_real_escape_string > } > if ($deleteList) { > mysql_query("DELETE FROM imp_fleet WHERE fleet_id IN($deleteList)") or die > (mysql_error()); > } > if (mysql_error()) { this is never reached if there was an error since you die'd() on errors befor. > } > } > ... > > while ($row = mysql_fetch_array($sql)) { > ?> > >value=""> value=""> >value=""> >value=""> > > // set up a variable for the input form names for easier reading. $var_name = 'fleets['.$row['fleet_id'].']'; ?> ... I'd suggest not using the http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Review PLEASE !!!
If it were me I would do both Client and Server side validation. The majority of the time the client side will suffice, but, simply put, because you don't/may not look at the HTML source of a web page, doesn't mean that nobody else does. The fact of the matter is, you should not trust any data that comes from a form. Even if the ids come from the database, you still want to ensure that they really are a valid numerical value or whatever your ids happen to be based upon. Jordan S. Jones Matthew Oatham wrote: 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" <[EMAIL PROTECTED]> To: "Matthew Oatham" <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]> 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"); } } ?> Edit / Delete Fleet Fleet Code Download URL History URL Delete $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)) { ?> value=""> value=""> value=""> value=""> value=""> } } ?> type="reset" value="cancel"> Thanks for your time and feedback. Matt -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Review PLEASE !!!
On Monday 05 April 2004 04:00 pm, Matthew Oatham wrote: > 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. google for "SQL injection" and you will see why what you currently have may cause you some problems. -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Review PLEASE !!!
On Monday 05 April 2004 04:00 pm, Matthew Oatham wrote: > Yes I agree I need some validation, dunno whether to do server or client > side validation. *both* :) you should always do server side validation on any data, especially if you are going to be putting it into your database. Client side validation, can be helpful to the user, but is easy to bypass. -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] 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" <[EMAIL PROTECTED]> To: "Matthew Oatham" <[EMAIL PROTECTED]>; <[EMAIL PROTECTED]> 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"); > > } > >} > > > >?> > > > > > > > > > > > > > >Edit / Delete Fleet > > > > > > Fleet Code > > Download URL > > History URL > > Delete > > > > >$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)) { > >?> > > > > > > > > > > > > > > >} > >} > >?> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >Thanks for your time and feedback. > > > >Matt > > > > > > -- > PHP General Mailing List (http://www.php.net/) > To unsubscribe, visit: http://www.php.net/unsub.php > > -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
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"); } } ?> Edit / Delete Fleet Fleet Code Download URL History URL Delete if (mysql_num_rows($sql) > 0) { while ($row = mysql_fetch_array($sql)) { ?> } ?> Thanks for your time and feedback. Matt -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Review PLEASE !!!
> 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 / > include ("../db.php"); Some things I do is use single quotes include '../db.php' ; (they are slightly faster, no replacments looking for $ variables) -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
[PHP] Code Review PLEASE !!!
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: $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"); } } ?> Edit / Delete Fleet Fleet Code Download URL History URL Delete 0) { while ($row = mysql_fetch_array($sql)) { ?> Thanks for your time and feedback. Matt