I was going to say that "there's nothing in the posted code to print the line twice, so the problem lies elsewhere". Instead, some feedback. For one thing, there's a security hole.
Just want to make sure you know about
include_once. In some cases, it's better to make sure a file isn't included multiple times (which can happen when e.g. A includes B and C, and B includes C).
PHP:
function getAllVotes($id) {
/**
Returns an array whose first element is votes_up and the second one is votes_down
Indexing the array by number requires remembering or looking up what's at each index. Better to use 'votes_up' and 'votes_down' as the indices. This also simplifies the function, as you can return the row resulting from the MySQL query.
If a row isn't found, this results in an empty array being returned, which would be fine if you checked for an empty array before using the result, which you don't. The options are to check the return value or return an array with the expected indices defined and all values set to zero ("
array(0,0)" in your version or "
array('votes_up' => 0, 'votes_down' => 0)" in mine).
PHP:
$q = "SELECT * FROM 4wheelmadness WHERE id = $id";
The cases where a "SELECT *" in code is a good idea are few and far between, and all involve writing DB administration apps, such as phpMyAdmin. "SELECT *" results in wasted bandwidth, as you transfer fields you never use, and potentially makes it harder to alter tables (not a problem here, as you don't reference fields by number). Better to select only the fields you care about ("votes_up" and "votes_down").
There is also an
SQL Injection vulnerability via
$id here, which I'll get back to later.
PHP:
function getEffectiveVotes($id) {
...
function getEffectiveVotes2($id) {
Not very descriptive names. Why not "getVotesUp" and "getVotesDown"?
Try this:
PHP:
function getAllVotes($id, $fields='votes_up, votes_down') {
$id = (int) $id;
$q = "SELECT $fields FROM 4wheelmadness WHERE id = $id";
$r = mysql_query($q);
$votes = array();
if(mysql_num_rows($r)==1) { //id found in the table
$votes = mysql_fetch_assoc($r);
}
return $votes;
}
function getVotesUp($id) {
$votes = getAllVotes($id, 'votes_up');
return $votes['votes_up'];
}
function getVotesDown($id) {
$votes = getAllVotes($id, 'votes_down');
return $votes['votes_down'];
}
Actually, by the end of this post we'll have gotten rid of the get*Votes* functions entirely. I'm discussing them for educational purposes and because you might need them for other scripts (they should be in a separate "votes.php" script, in any case).
Here's the first part of the SQL injection (see also: "
SQL Injection Walkthrough"
, the
PHP manual and "
Little Bobby Tables"). You're taking user input and later placing it directly in an SQL query. What if the visitor sets the 'id' input to
1 OR 1=1 LIMIT 1 or
1; drop table 4wheelmadness? Always sanitize user input, though there's still the question of when to do it: when you get the user input (when you access
$_POST['id']) or use the input (in
getAllVotes). A better solution (the modern solution) is to use
prepared statements, which the
PDO driver offers. I can't stress this enough, since few people seem to pay attention to this advice: use the PDO driver. It's not harder to use than the old MySQL driver, and is more secure. In DB.php, put something like:
PHP:
class DB {
static $db = array();
protected function __construct() {}
static function connect($dbName='') {
if (! isset(self::$db[$dbName]) ) {
if ($dbName) {
$dbOpt = ';dbname=' . $dbName;
} else {
$dbOpt = '';
}
self::$db[$dbName] = new PHP("mysql:host=localhost$dbOpt", 'username', 'password');
self::$db[$dbName]->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
return self::$db[$dbName];
}
}
Whenever you want a DB connection, include "DB.php" and call
DB::connect(). Homework: expand class DB to properly support a
close() method. You'll need to keep track of the number of calls to
DB::connect() for each database name, and only close a connection when this count reaches 0. Googling "reference counting" might help.
For error handling, wrap most everything up in a try/catch block (including the
include('config.php');):
PHP:
try {
...
} catch (PDOException $exc) {
error_log($exc);
echo "There was a database error processing your vote. It's been logged, and we'll look into it.";
} catch (Exception $exc) {
error_log($exc);
echo "There was an error processing your vote. It's been logged, and we'll look into it.";
}
Personally, I only put in config.php statements that won't error and don't call functions (well, basic operations and simple string functions are allowed). I'll also create an "init.php" script that performs any tasks requiring more complex processing, such as adding to the include path and defining and
registering an
autoload function (init.php will also include config.php). The idea is that an admin will edit config.php to change options, but init.php should only need to be edited during development.
Things like DB connections go in a set of database access scripts, along with classes/functions that access the database. This way, all the code that touches the database isn't scattered everywhere, which vastly simplifies
code maintenance.
For reference, user input can be sanitized using
filter functions or database-native function, such as
mysql_escape_string. That said, prepared queries are the way to go.
Another way of reducing damage from SQL injection is to limit the privileges of the DB user. This is of limited utility since the DB user needs the UPDATE privilege.
PHP:
if($action=='vote_up') //voting up
{
$votes_up = $cur_votes['votes_up']+1;
$q = "UPDATE 4wheelmadness SET votes_up = $votes_up WHERE id = $id";
}
elseif($action=='vote_down') //voting down
{
$votes_down = $cur_votes['votes_down']+1;
$q = "UPDATE 4wheelmadness SET votes_down = $votes_down WHERE id = $id";
}
There's no need to fetch the current value of a field before updating it. You can do it solely in SQL:
Code:
UPDATE 4wheelmadness SET votes_down = votes_down+1 WHERE id = ?
For sequences of 'if' statements, a
switch statement is more natural:
PHP:
switch ($action) {
case 'vote_up':
$q = "UPDATE 4wheelmadness SET votes_up = votes_up+1 WHERE id = ?";
break;
case 'vote_down':
$q = "UPDATE 4wheelmadness SET votes_down = votes_down+1 WHERE id = ?";
break;
default:
throw new Exception("You asked me to perform an action ($action) I don't know how to do.");
break;
}
Associative arrays can also often take the place of a sequence of if blocks and switches:
PHP:
$actions = array('vote_up' => 'votes_up', 'vote_down' => 'votes_down');
...
if (!isset($actions[$action])) {
echo "You asked me to perform an action ($action) I don't know how to do."
} else {
$voteQuery = "UPDATE 4wheelmadness SET {$actions[$action]} = {$actions[$action]}+1 WHERE id = ?";
$stmt = $db->prepare($voteQuery);
$stmt->execute(array($id)); // if this fails, it will throw an exception.
echo 'Thank you for your vote.';
}
Neither your code nor the above properly handles the case where a row with the given id doesn't exist.
This post is much longer than I intended. C'est programmation.
I'm going to read through your whole post and hopefully i'l take all your advice into consideration.