PHP Quiz, Attempt #2

[Irate]

So, after my last topic on this subject, I figured out how to validate my quiz with arrays, using only POST methods, and I have made some attempts to do so with MySQL.

I have a table named quiz in a database named 'DA' where each row has one primary key 'pid' as integer, simply numbered down from 1 to 35. Next I have the second column named 'answer' which contains the value attribute from my input buttons (the correct one out of 2-6 answer options).

How would I attempt to validate the quiz with the result I get from MySQL?

 

Code fragments I have so far...

 

<?php // quiz.php
// do something like headers here etc.
<form action="score.php" method="POST">
<input type="radio" name="1" value="a1">Question 1<br>
<input type="radio" name="1" value="b1">Question 2<br>
// and so on
?>

Now score.php...

 

<?php // score.php
$total = 0;
$validate = new mysqli('localhost',
'admin',
'password',
'DA');
if(!$validate){
die('A database connection problem occured.');
}
foreach($_POST as $answer){
$query_quiz = ('SELECT answer FROM quiz IF($answer = answer)');
if($validate->query($query_quiz)!=-1 AND $validate->$query_quiz>0){
$total++;
}
}
switch($total){
case 0: // now do the case check for the score and display some misc. messages
exit; break;
case 1: // and so on, until 35
exit; break;
}
?>

I cannot do a functionality check right now since I am on my mobile, but I would like to get some feedback on the code and where I could improve the code.

 

Remember, I'm still new to PHP

 

Anyway, thanks in advance!

[.josh]

Okay so firstly, let's talk about query syntax.  You are using a variable inside single quotes.  Variables do not get parsed inside single quotes, so it's comparing the literal string '$answer' to 'answer', not the value of $answer.  So you either need to use double quotes or else break out of the quotes and use string concatenation.  

 

2nd, your query syntax is wrong.  You should be using a WHERE clause.  Normally it would be "..WHERE answer='$answer'" but that brings me to the next point: you really don't need to be querying your db 35 times (once for each question).  Instead, you should make a single query that either selects all of the data, or else just the relevant data.  In general, only selecting the relevant data is more efficient, Also, you didn't do any kind of sanitation of the posted values, which makes your script vulnerable to sql injection attacks. I will include some basic sanitation in the script below, but you should change it to more explicitly check for expected values and reject them (do not use them in the query) if they do not match the expected format.  Better yet, look into prepared statements (not going to show below).

 

3rd, your condition to check if the query was successful is a bit funky but I guess technically works...basically for SELECT queries, you will either get an object returned, of FALSE.  You will never get -1 returned, but 0 does evaluate to false so technically this works, but you should take the time to understand what is returned and use proper conditions. 

 

4th, I'm not sure what your goal is with that switch statement, but it almost certainly could be refactored to be more efficient.  For example, if you simply wanted to display a custom message for how many questions you got right, you could just use an array and point to the array index that corresponds to how many questions were answered right.  If this is not quite what you're going for, then you'll have to provide more details on this, but taking all of these points into account, this is what the code should look like:

 
<?php // score.php
 
$validate = new mysqli('localhost',
'admin',
'password',
'DA');
if(!$validate){
die('A database connection problem occured.');
}
 
 
// here is where you should do some sanitation of the posted answers  
// just a basic escape function; ideally you should explicitly check for expected
// formats. Also, you should throw in some logic to only assign the answers
// to $answers array. The easy way to do this would be to namespace your answers
// form elements.  The harder way would be to add a condition in this loop to 
// evaluate $k, which is the name of the form element.  
$answers = array();
foreach ($_POST as $k => $v) {
  $answers[] = $validate->real_escape_string($v);
}
 
// implode the array into a string to insert into the query.  The format is 'a','b','c'
$answers = "'".implode("','",$answers)."'"; 
 
// single query to select all relevant answers. The IN filter will select any
// row where the value of answer is in the list.
$query_quiz = $validate->query("SELECT answer FROM quiz WHERE answer IN($answers)");
 
// A SELECT query will either return an object or FALSE, so all you have to do is this:
if($query_quiz) {
 
  // since you only selected relevant rows, the number of rows returned is 
  // how many questions you got right
  $total = $query_quiz->num_rows;
 
  // array of custom messages. Index 0 is 0 answers right, index 1 is 1 answer right, etc..
  $messages = array(
    'you got 0 right!',
    'you got 1 right!',
    'you got 2 right!',
    // etc..
  );
 
  // output the custom message 
  echo $messages[$total];
 
  // alternatively, if all the messages are the same except the number, you could
  // simply do
  echo "you got $total right!";
 
} // end if $query_quiz
?>

[Irate]

Okay so firstly, let's talk about query syntax. You are using a variable inside single quotes. Variables do not get parsed inside single quotes, so it's comparing the literal string '$answer' to 'answer', not the value of $answer. So you either need to use double quotes or else break out of the quotes and use string concatenation.

 

2nd, your query syntax is wrong. You should be using a WHERE clause. Normally it would be "..WHERE answer='$answer'" but that brings me to the next point: you really don't need to be querying your db 35 times (once for each question). Instead, you should make a single query that either selects all of the data, or else just the relevant data. In general, only selecting the relevant data is more efficient, Also, you didn't do any kind of sanitation of the posted values, which makes your script vulnerable to sql injection attacks. I will include some basic sanitation in the script below, but you should change it to more explicitly check for expected values and reject them (do not use them in the query) if they do not match the expected format. Better yet, look into prepared statements (not going to show below).

 

3rd, your condition to check if the query was successful is a bit funky but I guess technically works...basically for SELECT queries, you will either get an object returned, of FALSE. You will never get -1 returned, but 0 does evaluate to false so technically this works, but you should take the time to understand what is returned and use proper conditions.

 

4th, I'm not sure what your goal is with that switch statement, but it almost certainly could be refactored to be more efficient. For example, if you simply wanted to display a custom message for how many questions you got right, you could just use an array and point to the array index that corresponds to how many questions were answered right. If this is not quite what you're going for, then you'll have to provide more details on this, but taking all of these points into account, this is what the code should look like:

 
<?php // score.php
 
$validate = new mysqli('localhost',
'admin',
'password',
'DA');
if(!$validate){
die('A database connection problem occured.');
}
 
 
// here is where you should do some sanitation of the posted answers  
// just a basic escape function; ideally you should explicitly check for expected
// formats. Also, you should throw in some logic to only assign the answers
// to $answers array. The easy way to do this would be to namespace your answers
// form elements.  The harder way would be to add a condition in this loop to 
// evaluate $k, which is the name of the form element.  
$answers = array();
foreach ($_POST as $k => $v) {
  $answers[] = $validate->real_escape_string($v);
}
 
// implode the array into a string to insert into the query.  The format is 'a','b','c'
$answers = "'".implode("','",$answers)."'"; 
 
// single query to select all relevant answers. The IN filter will select any
// row where the value of answer is in the list.
$query_quiz = $validate->query("SELECT answer FROM quiz WHERE answer IN($answers)");
 
// A SELECT query will either return an object or FALSE, so all you have to do is this:
if($query_quiz) {
 
  // since you only selected relevant rows, the number of rows returned is 
  // how many questions you got right
  $total = $query_quiz->num_rows;
 
  // array of custom messages. Index 0 is 0 answers right, index 1 is 1 answer right, etc..
  $messages = array(
    'you got 0 right!',
    'you got 1 right!',
    'you got 2 right!',
    // etc..
  );
 
  // output the custom message 
  echo $messages[$total];
 
  // alternatively, if all the messages are the same except the number, you could
  // simply do
  echo "you got $total right!";
 
} // end if $query_quiz
?>

Hm, right, I guess I used single quotes because I didn't remember that vars aren't used as their values within single quotes. The . operator would've worked for it, too, but thanks anyway.

About SQL injections, is it really necessary to escape the preset input value I gave the user?

And I guess you're right anyway, I prefer to ask once again to be secure.

 

I'll also be sure to check prepared statements thoroughly, got it.

I think I'm still used to JavaScript where such search would have returned an indexOf-like result, but it could also that I didn't know what I did ;~;

I want to be sure that the query returns true every time executed, even when it is 0 because 0 answers correct is also possible (yet rare, I still want to be prepared for it).

As for the switch statement, I basically want it to echo out a new input button that contains the total amount to submit it to somewhere (I have that submission action handled).

But I guess an array would work for that, too.

 

I'll take some time to deeply review this when I'm on my PC, my mobile is not all too ideal for this.

Thanks anyway for answering, appreciate it.

Edited May 10, 2013 by Irate

[.josh]

About SQL injections, is it really necessary to escape the preset input value I gave the user?

And I guess you're right anyway, I prefer to ask once again to be secure.

 

Okay so yeah..you have the preset value in your form..but that doesn't stop the user from changing the value in the form so that it submits a different value.  It is ridiculously easy to do this sort of thing, and people do it all the time.  You should NEVER trust data coming from a form (or any request).  You should always validate and scrub the data you receive, and you should always do this server-side.  Do NOT do it client-side (javascript), because a user can just disable javascript or else alter it, same as the form. 

 

 

I want to be sure that the query returns true every time executed, even when it is 0 because 0 answers correct is also possible (yet rare, I still want to be prepared for it).

 

Right, and this is why I said you should take the time to look into and understand what is returned from a query.  Your original condition looked like you expected a return value of number of rows returned, but that is not what is returned from a query.  Well, it is ONE thing that is returned, but it is nested within a resource object.  A  SELECT query will always return either a resource object (if the query was successful, even if no rows were selected), or FALSE (the query failed - e.g. the query syntax was wrong).  In other words, a select query is still executed successfully even if no rows were actually selected, so you will get a resource object returned.  As shown in my example, $query_quiz->num_rows  is the property that contains the number of rows returned from the query.  If no rows were actually selected (user got 0 answers right), $query_quiz->num_rows will be 0.

 

 

As for the switch statement, I basically want it to echo out a new input button that contains the total amount to submit it to somewhere (I have that submission action handled).

But I guess an array would work for that, too.

 

Okay well if you are basically just wanting to echo out an input field and it's the input field no matter what the score is, only difference is the value, then yeah, you don't need a switch with 35 cases in it.. you don't even need the array.  You can just do like the 2nd output example I gave, 

 

Example of hidden input:

 

 

echo "<input type='hidden' name='someName' value='$total' />";