if clauses will not work as wanted

Question

So I have a forum where a user has several characters they can post with. Now on the Edit post page I want to have the character that posted the post selected in the select menu. If the post have been sent and there was an error and the player choose to edit with a another character other than the one that originally made the post I want to have that character selected.

The problem is right now it prints the character that made the post as many times as the users number of characters.

So let's say the character that made this post is called Bob. And the user that owns Bob has 6 characters it will print Bob six times. When it should be Bob selected and then all the other characters. And if they chose to edit with another character I want to have that one selected and then print all the others. I hope you understand.

$post = $db->query("SELECT c.subject, a.message, a.hide_smilies, a.posted, b.name, b.user_id, c.id AS topic_id, c.closed, d.id AS forum_id, d.name AS forum_name FROM posts a 
               INNER JOIN characters b ON a.poster = b.id 
               INNER JOIN topics c ON a.topic_id = c.id
               INNER JOIN forums d ON c.forum_id = d.id
               WHERE a.id = $id")->fetch();


Character to edit with:
foreach($db->query("SELECT id, name FROM characters WHERE user_id = $is_logged_in ORDER BY name ASC") as $row):
if (isset($_POST['character']) && ($row['id'] == $_POST['character'])) {
   echo '<option value="'.$row['id'].'" selected>'.$row['name'].'</option>';
} else if (isset($post['name'])) {
   echo '<option value="'.$row['id'].'" selected>'.$post['name'].'</option>';
} else  {
   echo '<option value="'.$row['id'].'">'.$row['name'].'</option>';
}
endforeach

Your query is a SQL injection attack waiting to happen. Please use bind variables, and save yourself and your users headaches in the future. http://stackoverflow.com/questions/1860130/how-to-bind-sql-variables-in-php — Palpatim, Nov 22 '13 at 23:54
Please learn how to use [**prepared statements**](https://www.youtube.com/watch?v=nLinqtCfhKY). — tereško, Nov 22 '13 at 23:56
Do all of the user's characters have the same `characters.id`? — Barmar, Nov 23 '13 at 00:00
@Palpatim $id = isset($_GET['postid']) ? intval($_GET['postid']) : 0; — Kaka, Nov 23 '13 at 00:00
@Barmar Every character has an unique ID. in The posts table it's stored in 'poster'. — Kaka, Nov 23 '13 at 00:00
Which query are you having a problem with? The one used to assign to `$post` or the one in the `foreach`? — Barmar, Nov 23 '13 at 00:03
The problem is here: `if (isset($post['name'])) {` will be true every time through the loop. So it will keep printing `$post['name']`. I can't figure out what you're trying to do there. — Barmar, Nov 23 '13 at 00:11

vogomatix · Answer 1 · 2013-11-23T00:24:09.810

This is not a complete answer but it is a start it's too big to fit in a comment!

Using bound variables to prevent SQL injections (as suggested by others in comments). Improve loop structure and other improvements

$sql = <<<EOF
SELECT c.subject, a.message, a.hide_smilies, a.posted, b.name, b.user_id, c.id AS topic_id, c.closed, d.id AS forum_id, d.name AS forum_name 
FROM posts a 
INNER JOIN characters b ON a.poster = b.id 
INNER JOIN topics c ON a.topic_id = c.id
INNER JOIN forums d ON c.forum_id = d.id
WHERE a.id = ?
EOF;

$stmt = $db->prepare($sql);

$stmt->bind_param("s", $is_logged_in); 
$result = $stmt->fetch();

$char_sql = <<<EOF
SELECT id, name 
FROM characters WHERE user_id = ? 
ORDER BY name ASC
EOF;

$stmt = $db->prepare($char_sql);
$stmt->bind_param( "s", $id);
$result = $stmt->execute();


foreach ($result->fetch_row($char_sql) as $row):
    $id = $row['id']; $name = $row['name'];
    if (isset($_POST['character']) && ($id == $_POST['character'])) {
       echo "<option value='{$id}' selected>{$name}</option>";
    } else if (isset($post['name'])) {
        echo "<option value='{$id}' selected>{$post['name']}</option>";
    } else  {
        echo "<option value='{$id}'>{$name}</option>";
    }
endforeach;

This is a good recommendation, but how will it solve the problem he's having? It should be a comment, not an answer. — Barmar, Nov 23 '13 at 00:06

score 0 · Answer 2 · answered Nov 23 '13 at 00:14

0

Change this line:

} else if (isset($post['name'])) {

to:

} else if ($post && $post['name'] == $row['name']) {

isset($post['name'] is the same every time through the loop, , so you were printing $post['name'] for each row fetched.

answered Nov 23 '13 at 00:14

Barmar

741,623
53
500
612

I'm not sure if this is what is needed as it effectively makes the first two if conditions print the same option statement – vogomatix Nov 23 '13 at 00:25
But he's using `else if`, so only one of them will be used. I don't know the relationship between `$_POST['character']` and `$post['name']`. – Barmar Nov 23 '13 at 00:31

if clauses will not work as wanted

2 Answers2