[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:
?
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
h1Edit / Delete Fleet/h1
table
tr
tdFleet Code/td
tdDownload URL/td
tdHistory URL/td
tdDelete/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
tdinput type=text name=fleet_code[]
value=?=$row['fleet_code']?input type=hidden name=fleet_id[]
value=?=$row['fleet_id']?/td
tdinput type=text name=download_url[]
value=?=$row['download_url']?/td
tdinput type=text name=history_url[]
value=?=$row['history_url']?/td
tdinput type=checkbox name=delete[] value=?=$row['fleet_id']?/td
/tr
?
}
}
?
tr
td colsapn=4
table
tr
tdinput type=hidden name=action value=updateinput type=reset
value=cancel/td
td colspan=2input type=submit value=submit/td
/tr
/table
/td
/tr
/table
/form
/body
/html
Thanks for your time and feedback.
Matt
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
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
h1Edit / Delete Fleet/h1
table
tr
tdFleet Code/td
tdDownload URL/td
tdHistory URL/td
tdDelete/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
tdinput type=text name=fleet_code[] value=?=$row['fleet_code']?input type=hidden name=fleet_id[] value=?=$row['fleet_id']?/td
tdinput type=text name=download_url[] value=?=$row['download_url']?/td
tdinput type=text name=history_url[] value=?=$row['history_url']?/td
tdinput type=checkbox name=delete[] value=?=$row['fleet_id']?/td
/tr
?
}
}
?
tr
td colsapn=4
table
tr
tdinput type=hidden name=action value=updateinput type=reset value=cancel/td
td colspan=2input 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
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);
}
}
?
html
head
title/title
/head
body
form name=edit method=post
h1Edit / Delete Fleet/h1
table
tr
tdFleet Code/td
tdDownload URL/td
tdHistory URL/td
tdDelete/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
tdinput type=text name=fleet_code[]
value=?=$row['fleet_code']?input type=hidden name=fleet_id[]
value=?=$row['fleet_id']?/td
tdinput type=text name=download_url[]
value=?=$row['download_url']?/td
tdinput type=text name=history_url[]
value=?=$row['history_url']?/td
tdinput type=checkbox name=delete[]
value=?=$row['fleet_id']?/td
/tr
?
}
}
?
tr
td colsapn=4
table
tr
tdinput type=hidden name=action value=updateinput
type=reset value=cancel/td
td colspan=2input 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
--
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 !!!
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 !!!
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);
}
}
?
html
head
title/title
/head
body
form name=edit method=post
h1Edit / Delete Fleet/h1
table
tr
tdFleet Code/td
tdDownload URL/td
tdHistory URL/td
tdDelete/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
tdinput type=text name=fleet_code[]
value=?=$row['fleet_code']?input type=hidden name=fleet_id[]
value=?=$row['fleet_id']?/td
tdinput type=text name=download_url[]
value=?=$row['download_url']?/td
tdinput type=text name=history_url[]
value=?=$row['history_url']?/td
tdinput type=checkbox name=delete[]
value=?=$row['fleet_id']?/td
/tr
?
}
}
?
tr
td colsapn=4
table
tr
tdinput type=hidden name=action value=updateinput
type=reset value=cancel/td
td colspan=2input 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
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?
?
use full php tags: ?php
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)) {
?
tr
tdinput type=text name=fleet_code[]
value=?=$row['fleet_code']?input type=hidden name=fleet_id[]
value=?=$row['fleet_id']?/td
tdinput type=text name=download_url[]
value=?=$row['download_url']?/td
tdinput type=text name=history_url[]
value=?=$row['history_url']?/td
tdinput type=checkbox name=delete[] value=?=$row['fleet_id']?/td
// set up a variable for the input form names for easier reading.
$var_name = 'fleets['.$row['fleet_id'].']';
?
td
input name=?php echo $var_name?[code]
input name=?php echo $var_name?[urls][history]
input name=?php echo $var_name?[urls][download]
...
I'd suggest not using the ?= shorthand also.
HTH,
Curt
--
I used to think I was indecisive, but now I'm not so sure.
--
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 !!!
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
