[RESOLVED] Script Works... Can It Be Better?

[dispratt]

I've only written a handful of PHP scripts... this works fine, but I want it to be correct. So I'm looking for any feedback on the code, security issues, etc.

It's a basic MySQL database of work for our clients. The tables "clients", "industries" and "projects" get merged into the table "client_roster" (which has some additional info, not shown here).

I created a PHP page with links to display clients by our four project categories. So when you click on "Reports" you get back all the client names we did Reports for.

Thanks for any pointers or suggestions for improvement.

Page: projects.php
----------------------

<a href="<?php echo "projects.php?rep=1"; ?>">Reports</a>
<a href="<?php echo "projects.php?aud=2"; ?>">Audits</a>
<a href="<?php echo "projects.php?ass=3"; ?>">Assessments</a>
<a href="<?php echo "projects.php?rev=4"; ?>">Reviews</a>

<?php
if ($_GET['rep'] !=NULL) {
    $project = "AND projects.project_id = '1'";
}
else if ($_GET['aud'] !=NULL) {
    $project = "AND projects.project_id = '2'";
}
else if ($_GET['ass'] !=NULL) {
    $project = "AND projects.project_id = '3'";
}
else {
    $project = "AND projects.project_id = '4'";
}

include 'config-file.php';

$query = "SELECT * FROM client_roster, clients, industries, projects 
					WHERE client_roster.client_id = clients.client_id
					AND client_roster.project_id = projects.project_id
					AND client_roster.industry_id = industries.industry_id
					$project
					ORDER BY client_name ASC"; 

$result = mysql_query($query) or die('Error : ' . mysql_error());

while($row = mysql_fetch_assoc($result)) {

?>

<table >
<tr>
<tr>
<td>Project Type</td>
<td><?php echo $row['project_type'];?></td>
</tr>
<td>Name</td>
<td><?php echo $row['client_name'];?></td>
</tr>
<tr>
<td>Industry</td>
<td><?php echo $row['industry_segment'];?></td>
</tr>

</table>

<br />

<?php } mysql_close($conn); ?>

[bradgrafelman]

Thread moved to Code Critique forum.

Also, welcome to PHPBuilder dispratt! Here are a few critiques:

1. <a href="<?php echo "projects.php?rep=1"; ?>">Reports</a>
   <a href="<?php echo "projects.php?aud=2"; ?>">Audits</a>
   <a href="<?php echo "projects.php?ass=3"; ?>">Assessments</a>
   <a href="<?php echo "projects.php?rev=4"; ?>">Reviews</a> 

   Why are you using PHP to echo out plain strings, adding superfluous code into the mix?

2. if ($_GET['rep'] !=NULL) {
       $project = "AND projects.project_id = '1'";
   }
   else if ($_GET['aud'] !=NULL) {
       $project = "AND projects.project_id = '2'";
   }
   else if ($_GET['ass'] !=NULL) {
       $project = "AND projects.project_id = '3'";
   }
   else {
       $project = "AND projects.project_id = '4'";
   } 

   I would suggest using [man]isset/man or [man]empty/man instead; otherwise, you're generating E_NOTICE error messages about undefined indexes.

3. SELECT *

   Do you really need to SELECT the entire table? Since you don't use all of the data, I'm going to guess you don't. You should always SELECT only the data you actually need to use (even if that happens to be all of the columns - a list of columns will be more scalable than 'SELECT ').
   

4. or die('Error : ' . mysql_error()); 

   Once this script goes live, you should get rid of this code; displaying MySQL error messages to users is never desirable (that type of information wasn't meant for them). Instead, you should implement some other method of tracking errors (be it your own error handler or at least enabling log_errors and disabling display_errors in PHP).

[Weedpacket]

Just adding a couple of subpoint to bradgrafelman's points. (What's a subpoint? Geometrically that's just bizarre.)

bradgrafelman wrote:

3. Do you really need to SELECT the entire table? Since you don't use all of the data, I'm going to guess you don't.

Especially since this would include duplicate columns from all the join criteria - that's at least three redundant columns right there.

4. Once this script goes live, you should get rid of this code; displaying MySQL error messages to users is never desirable

Also, you should provide a more graceful way of letting the user down than simply slamming the door in their face with die(). Your error handling should generate a proper page for them to look at, preferably one with any suggestions on how they might recover and navigation so that they can remain on your site.

[dispratt]

Thanks very much for the pointers, very helpful... I'll update the code and repost in a few days.

[dispratt]

I took the feedback (thanks bradgrafelman and Weedpacket), did some research and developed this new version.

I've changed it so all clients are listed when you first land on the page. Then if you click on a project category link, you'll see clients in that category along with their industry. I still need to work on the error handling end of things.

Page: projects.php
---------------------- 

<a href="projects.php">Home</a>

<a href="projects.php?id=1">Reports</a>
<a href="projects.php?id=2">Audits</a>
<a href="projects.php?id=3">Assessments</a>
<a href="projects.php?id=4">Reviews</a>

<?php 

include 'config-file.php';

// if a link id is selected
if (isset($_GET['id'])) {

// then get all clients in that project category
$query = "SELECT   client_roster.client_id, client_roster.project_id, client_roster.industry_id, 
								   clients.client_name, 
								   industries.industry_segment,
								   projects.project_type
				  FROM     client_roster, clients, industries, projects 
					WHERE    client_roster.client_id = clients.client_id
					AND      client_roster.industry_id = industries.industry_id
					AND      client_roster.project_id = projects.project_id
					AND			 projects.project_id = {$_GET['id']}
					ORDER BY client_name ASC"; 

// but nothing is selected when you first land on page
} else {

// so then get all the client names and list in alpha order
$query = "SELECT client_name FROM clients ORDER BY client_name ASC"; 
}

// whatever the query was, here is the result
$result = mysql_query($query);

while($row = mysql_fetch_array($result)) {

// if a link id is not empty, echo a table with project category, client name and industry
if ($_GET['id'] !=NULL) { 
echo '<table width="500" border="1" cellspacing="1" cellpadding="1">
	 			<tr>
					<td width="200">Project Type</td>
					<td>'.$row['project_type'].'</td>
				</tr>
				<tr>
					<td width="200">Name</td>
					<td>'.$row['client_name'].'</td>
				</tr>
				<tr>
					<td width="200">Industry</td>
					<td>'.$row['industry_segment'].'</td>
				 </tr>
			</table><br />'; 

// but when you first land on page with no link id selected, just echo a table of all client names 
} else { 
?>
<table width="500" border="1" cellspacing="1" cellpadding="1">
<?	echo '<tr>
					<td width="200">Name</td>
					<td>'.$row['client_name'].'</td>
				</tr>'; 
} 
?>
</table>

<?
} mysql_close($conn); 

?>

[foyer]

Tabbing if statements and loops can make code a bit more readable. I've also made some other changes to make it a little easier to follow I think.

Page: projects.php
----------------------

<a href="projects.php">Home</a>

<a href="projects.php?id=1">Reports</a>
<a href="projects.php?id=2">Audits</a>
<a href="projects.php?id=3">Assessments</a>
<a href="projects.php?id=4">Reviews</a>

<?php

include 'config-file.php';

// if a link id is selected
if (isset($_GET['id']))
{
	// then get all clients in that project category
	$query = "SELECT   client_roster.client_id, client_roster.project_id, client_roster.industry_id,
									   clients.client_name,
									   industries.industry_segment,
									   projects.project_type
					  FROM     client_roster, clients, industries, projects
						WHERE    client_roster.client_id = clients.client_id
						AND      client_roster.industry_id = industries.industry_id
						AND      client_roster.project_id = projects.project_id
						AND             projects.project_id = {$_GET['id']}
						ORDER BY client_name ASC";

// but nothing is selected when you first land on page
} else
{
	// so then get all the client names and list in alpha order
	$query = "SELECT client_name FROM clients ORDER BY client_name ASC";
}

// whatever the query was, here is the result
$result = mysql_query($query);

while($row = mysql_fetch_array($result)) {
	// if a link id is not empty, echo a table with project category, client name and industry
	if (isset($_GET['id']))
	{
		?><table width="500" border="1" cellspacing="1" cellpadding="1">
						 <tr>
							<td width="200">Project Type</td>
							<td><?=$row['project_type']?></td>
						</tr>
						<tr>
							<td width="200">Name</td>
							<td><?=$row['client_name']?></td>
						</tr>
						<tr>
							<td width="200">Industry</td>
							<td><?=$row['industry_segment']?></td>
						 </tr>
					</table><br />
		<?		
		// but when you first land on page with no link id selected, just echo a table of all client names
	} else
	{
		?>
		<table width="500" border="1" cellspacing="1" cellpadding="1">
		<tr>
							<td width="200">Name</td>
							<td><?=$row['client_name']?></td>
						</tr></table>
		<?
	}
}

mysql_close($conn);
?>

[bradgrafelman]

Now you're vulnerable to SQL injection attaks. User-supplied data should never be placed directly into a query. Instead, you must first sanitize it with a function such as [man]mysql_real_escape_string/man. Note that in this case since $_GET['id'] is assumed to be numeric, you could cast it to an (int) instead.

[dispratt]

Thanks for the feedback... why, yes, an SQL injection attack is less than desirable.

The $_GET['id'] is indeed an integer, so this works if I use the cast approach:

...
// if a link id is selected
$id = (int)$_GET['id'];

if ($id > 0) {

// then get all clients in that project category
$query = "SELECT   client_roster.client_id, client_roster.project_id, client_roster.industry_id, 
									 clients.client_name, 
									 industries.industry_segment,
									 projects.project_type
					FROM     client_roster, clients, industries, projects 
					WHERE    client_roster.client_id = clients.client_id
					AND      client_roster.industry_id = industries.industry_id
					AND      client_roster.project_id = projects.project_id
					AND			 projects.project_id = '$id'
					ORDER BY client_name ASC"; 

...

Alternatively, this is also working:

...
// if a link id is selected
$id= mysql_real_escape_string($_GET['id']);
...

[bradgrafelman]

Note that the second method is good enough as long as you keep the data surrounded by single quotes in the SQL query. Since the data is numeric, however, I usually omit the quotes and instead cast the data to an integer. Either way will protect you.

[dispratt]

Thanks very much bradgrafelman... this has been a big help and motivated me to keep learning more.