The security aspect of things looks fine, but there are some areas for improvement,
I have provided inline comments where possible.
The whole quiz could be coded as a class, and would be a lot nicer, however I didn't want to go too far from what you started with so you can see what changes I have done.
Overall, I have split the code into functions, and tried to make each function do 1 thing only, and renamed variables for readability. This will become more important when you go to maintain the code.
The first bit of code is what I used to test all the functions and catches exceptions that occur. You could improve by adding more error checks after each prepare and execute, but I will leave that to you, as they are only really necessary if the database structure doesn't match the sql.
try {
$conn = connectToDatabase();
list($questions, $answers) = getQuestionsAndAnswers($conn);
// lets handle user data, separate of database calls
$answers = extractPostAnswers($_POST);
// not sure this is the best idea, if we have 2 simultaneous calls, both will get the same $maxId
// i am unsure of your database design, but the way I am assuming it should work is
// have an autoincrement column on the quiz administration table
// insert into the quiz administration table and get the last_insert_id back
// then use that id value to store in the answer table
$maxId = selectMaxId($conn);
$QuizAdministrationid = $maxId + 1;
saveAnswers($conn, $QuizAdministrationid, $answers);
$quizNumber = 1;
saveQuizAdministration($conn, $quizNumber);
$conn->close(); // close database connection here
header('Location: http://www.myWebsite.com/quizResults.php');
} catch (Exception $ex) {
// you should log the messages, and display a nice error for the user, this is just to give you idea of exception handling
die($ex->getMessage());
}
exit;
function extractPostAnswers($post) {
$answers = array();
for ($questnum=1; $questnum<21; $questnum++) {
// what if the $_POST doesn't exist? what is your default value
//$questansw = $_POST['qora'.$yty];
$key = "qora{$questnum}";
$answers[$questnum] = isset($_POST[$key]) ? $_POST[$key] : NULL ; // assuming null is ok as a default?
}
return $answers;
}
function connectToDatabase() {
$connect = new mysqli('localhost', 'root', '', 'test');
if ($connect->connect_error) {
// exit doesn't help anyone, at least let the user know there is a database error,
// and perhaps log the error for your own use
// exit;
throw new Exception('Database Connection Error: ' . $connect->connect_error);
}
return $connect;
}
function getQuestionsAndAnswers($connect) {
$questionArray = array();
$answersArray = array();
// is getStuff a really good description of what the variable is?
// $getStuff = $connect->prepare('SELECT `question`, `answer` FROM `Quizzes` WHERE `questionNumber`<? ORDER BY `questionNumber` ASC');
$stmt = $connect->prepare('SELECT `question`, `answer` FROM `Quizzes` WHERE `questionNumber`<? ORDER BY `questionNumber` ASC');
// what no error checking?
if (!$stmt) {
throw new Exception("Database Error: {$connect->error}");
}
// not quite sure why $questnum is a constant value, if so why bother having is as a bind param, it could go directly into the stmt
$questnum = 21;
$stmt->bind_param('i', $questnum);
$stmt->execute();
$stmt->bind_result($singleQuestion, $singleAnswer);
while ($stmt->fetch()) {
// array_push is great adding whole arrays, but a little overkill for single items, this is personal preference, not essential
//array_push($questionArray, $singleQuestion);
//array_push($answersArray, $singleAnswer);
$questionArray[] = $singleQuestion;
$answersArray[] = $singleAnswer;
}
$stmt->close();
// are you sure you want to close the connection here, it is passed in as a param, what if it something else needs to use it after this function?
// $connect->close();
// i am not a big fan of returning multiple values like this, but I have no suggestion as I am not sure how you are using the values
return array($questionArray, $answersArray);
}
function saveQuizAdministration($conn, $quizNumber) {
$cookie = session_id();
$ip = getUserIp();
$score = 90;
$stmt = $conn->prepare("INSERT INTO QuizAdministration (`quizNumber`, `cookie`, `ip`, `score`) VALUES (?, ?, ?, ?)");
// lets make things easier to read, name the variables the same as the columns
// $stmt->bind_param('issi', $quizNumber, $playerCookie, $playerIP, $playersPercent);
$stmt->bind_param('issi', $quizNumber, $cookie, $ip, $score);
$stmt->execute();
}
function saveAnswers($connection, $quizAdministrationId, $answers) {
$stmt = $connection->prepare("INSERT INTO PlayerAnswers (`QuizAdministrationId`, `questionNumber`, `playerAnswer`) VALUES (?, ?, ?)");
$questionNumber = NULL;
$playerAnswer = NULL;
// why not use the col names for variable names, then it would be a whole lot easier for me when I read the code for the first time
$stmt->bind_param('iii', $quizAdministrationId, $questionNumber, $playerAnswer);
// $yty is a cryptic variable name, what does it mean?
// i have moved the loop into the function extractPostAnswers as it shouldn't be mixed with database code
// for ($yty = 1; $yty < 21; $yty += 1) {
foreach ($answers as $questionNumber => $playerAnswer) {
// bind outside the for loop, that is whole point of binding to variables, the variables can change without needing to be re-binded
// $stmt->bind_param('iii', $QuizAdministrationid, $questnum, $questansw);
// we are re-assigning the same value over and over, put it outside the loop, outside the function even
// $QuizAdministrationid = $maxId + 1;
// assignment not required, use $questnum as the loop variable
// $questnum = $yty;
// if $questansw is null do we even want to execute the stmt? why store a null value
$stmt->execute();
}
$stmt->close();
}
function selectMaxId($connection) {
// whats the point of using prepared statements for a static statement that is being called once???
$stmt = $connection->prepare("SELECT MAX(`id`) FROM QuizAdministration");
$stmt->execute();
$stmt->bind_result($maxId);
$stmt->fetch();
$stmt->close();
// if no row exists, $maxId = null, is that what you are expecting?
return $maxId;
}
