I also understand that mysql_connect() et. al are deprecated in PHP since 5.5, but I assume better equivalents work the same way.
Sort of. MySQLi can work a lot like the the mysql extension, but personally I prefer PDO. Whereas MySQLi has both an OO and a procedural API, PDO has only an OO one. PDO can interact with more than just MySQL though.
Due to differences in SQL dialects, it's rare for it to be possible to just change out the driver and have everything work. It is a lot closer to cross compatibility than a specific RDBMS's API. Also, you can use your knowledge of the PDO API across multiple database systems. Whether it's MySQL, sqlite, SQL Server, etc, the general API is the same.
Anyway, whether you choose MySQLi or PDO, you should definitely stop using ext/mysql.
As for SQL injection: your code is basically a poster child for it.
$choice = isset($_GET['status']) ? $_GET['status'] : '7';
$result = mysql_query("SELECT id, subject FROM issues WHERE status_id = " . $choice, $conn);
What if I go to page.php?status=0 OR 1=1? The query becomes:
SELECT id, subject FROM issues WHERE status_id = 0 OR 1=1
This is a fairly benevolent example since all this will do is ignore the status filter. I've seen some very clever exploits though. Given enough time and determination, this small exploit could be ripped open into horrible, horrible things.
The easiest solution to SQL injection is to always stay aware that SQL is a language like any other (well, not on a technical level, but on a superficial level).
In particular, just remember to either escape content if it's being put into the query as a string, or cast it if it's being put in as something else.
For example:
$choice = isset($_GET['status']) ? (int) $_GET['status'] : 7;
$choice is now safe to put into an SQL statement.
This StackOverflow thread gives tons of options for properly handling SQL queries.
if (!$options || !$result || !$chosen_status)
{
echo("<p>Error performing query: " . mysql_error() . "</p>");
exit();
}
If more than one query fails, this will only output the error for the last one. Additionally, using exit for rendering errors is bad practice. In a file this simple, there's really not much of a better option without going way out of your way. What I tend to do in larger applications though is let uncaught exceptions (in other words, either the ones that I expect to never happen or the ones that are fatal) bubble up to the top at which point the request is transparently piped to an error controller (basically, I've copied the approach of most PHP MVC frameworks). If you wanted to use a similar approach, you could basically wrap exceptions over the MySQL API. That get's ugly fast though.
Oh, also, the mysql_error should be passed through htmlspecialchars. I'll add more to this note in a minute.
I'm not sure if it's technically valid to close a MySQL connection and then read the result sets. I suspect that this is not valid, and that if it works, it's just by luck.
Even when not going full-blown MVC, it can be very beneficial to separate logic or manipulation from the view. In particular, all of the mysql calls in the HTML parts, I would put in the PHP parts. If your view is only looping over an array, the source of the array can be changed without the view having to know about it. (Side note: depending on how strictly we're using terminology, the HTML part is probably better not called a view.)
while ($row = mysql_fetch_array($options, MYSQL_ASSOC))
You might as well us mysql_fetch_assoc.
echo('<option value="' . $row['id'] . '">' . $row['name'] . '</option>');
Anything being put into HTML needs to be run through htmlspecialchars. This comes back to being aware of context. It's just the rules of HTML. Certain things must be escaped in the same way that things in SQL or PHP must be escaped.
(I'm also not a fan of the echo() form of echo, but that's mostly opinion.)
echo '<option value="' . htmlspecialchars($row['id']) . '">' . htmlspecialchars($row['name']) . '</option>';
A lot of people will say that the htmlspecialchars on the id is pointless since it's going to be an int. While that's true, that's only for now. With a primary key column it's not likely to happen (in fact it probably shouldn't happen), but I've been in situations before where an int column has been changed to a varchar and we've had problems with the HTML escaping after that. Considering the overhead of htmlspecialchars is tiny, might as well call it on everything being put into HTML.
There's a few more comments and suggestions I have. I was originally going to add them as comments to the code, but I got a bit carried away and decided to rewrite it with PDO.
<?php
$db = new PDO('mysql:host=localhost;dbname=somedatabase', 'username', 'password', array(
PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
));
//The error mode being exception means that exceptions will be thrown if a query fails
//(or some other kind of error happens). This tends to be more convenient since you don't
//have to explicitly check for unexpected errors -- either everything executes fine, or the script
//crashes and burns (which might not be desired behavior in a lot of cases).
//Anyway, there is one little catch. PDOException instances will sometimes include with them
//the DSN (the 'mysql:host=...' string), the username, and the password. This means that you must be
//very careful about never letting an end user see a PDOException. In particular, you can either have
//a just-in-case top level try {} catch {} that can catch any unexpected errors, log them, and render
//them in a pretty page (what I like to do in any kind of sizable application), or at the very least,
//you can ensure that display_errors of Off on production servers (which it should be anyway). That way
//the exception will still be logged, but it won't be displayed.
//$options is a bad name. What kind of options?
$issueStatusesStmt = $db->query("SELECT name, id FROM issue_statuses");
//This is done in a weird way, but it has a reason.
//In particular, you can now check if a provided status id exists (isset($issueStatuses[$idToCheck])).
//And, you no longer need your name query since you can pull the name with $issueStatuses[$id]['name'].
$issueStatuses = array();
while ($is = $issueStatusesStmt->fetch(PDO::FETCH_ASSOC)) {
$issueStatuses[$is['id']] = $is;
}
//This handles the isset check and validates that the provided value is an integer.
//If the value fails the integer check, $statusId will be false. If no status param is
//provided in the URL, $statusId will be null. There is of course a lot more
//information about this at http://php.net/filter_input
$statusId = filter_input(INPUT_GET, 'status', FILTER_VALIDATE_INT);
if (!isset($issueStatuses[$statusId]) {
//This is what's called a magic constant. They tend to lead to problems.
//In particular, what is this magical status 7? Someone not intimately familiar
//with your DB data will have no idea.
$statusId = 7;
}
//This prepares a statement with a placeholder for status_id.
$issuesStmt = $db->prepare("SELECT id, subject FROM issues WHERE status_id = :statusId");
$issuesStmt->execute(array('statusId' => $statusId));
/*
* You could also do it like:
$issuesStmt = $db->prepare("SELECT id, subject FROM issues WHERE status_id = ?");
$issuesStmt->execute(array($statusId));
* I prefer to stick with named fillers though. It's significantly less confusing if you have
* to revisit a complicated query a month later.
*/
//You could make this an associative array like earlier, but really there's no reason to at this point.
$issues = $issuesStmt->fetchAll(PDO::FETCH_ASSOC);
//Note: if $statusId default to 7 and 7 somehow doesn't exist in the database,
//this will issue a notice and $statusName will be null.
$statusName = $issueStatuses[$statusId]['name'];
//PHP will only eat one new line. The doctype is supposed to be the very top line (unless I've imagined that requirement).
//This means it need to be on the line directly after the closing tag.
?>
<!DOCTYPE html>
<html>
<head>
<title>RM Rep</title>
<style>
body { font-family: Arial, sans-serif; }
td { padding: 0px 5px; }
</style>
</head>
<body>
<form name="status_choice" action="index.php" method="get">
<select name="status">
<?php foreach ($issueStatuses as $is) { ?>
<option value="<?php echo htmlspecialchars($is['id']); ?>"><?php echo htmlspecialchars($is['name']); ?></option>
<?php } ?>
</select>
<input type="submit" value="Submit">
</form>
<h1><?php echo htmlspecialchars($statusName); ?></h1>
<span><?php echo count($issues) . ' results'; ?></span>
<hr>
<?php
/*
If no entries are found, you will emit broken HTML in your current code.
(At least I think an empty table is invalid HTML -- can't remember.)
(Oh and also, I'm abusing PHP comments right now...)
*/
?>
<?php if (!empty($issues)) { ?>
<table>
<?php foreach ($issues as $issue) { ?>
<tr>
<td><?php echo htmlspecialchars($row['id']); ?></td>
<td><?php echo htmlspecialchars($row['subject']); ?></td>
</tr>
<?php } ?>
</table>
<?php } else { ?>
<div>
No entries matched the given status.
</div>
<?php } ?>
</body>
</html>
mysql_everywhere, consider writing wrapper functions. Implementdb_querythat simply callsmysql_query. Switching to Oracle or PostgreSQL then requires changing only a single file. Better is to use PDO, which avoids the issue altogether. \$\endgroup\$