MDL-43749 normalise quiz database structure.

The sequence of questions that made up a quiz used to be stored as a
comma-separated list in quiz.questions. Now the same information is
stored in the rows in the quiz_slots table. This is not just 'better' in
a database design sense, but it allows for the future changes we will
need as we enhance the quiz in the MDL-40987 epic.

Having changed the database structure, all the rest of the code needs to
be changed to account for it, and that is done here.

Note that there are not many unit tests for the changed bit. That is
because as part of MDL-40987 we will be changing the code further, and
we will add unit tests then.

backup/moodle2/restore_stepslib.php

@@ -4236,7 +4236,7 @@ protected function get_attempt_upgrader() {
 
     /**
      * Process the attempt data defined by {@link add_legacy_question_attempt_data()}.
-     * @param object $data contains all the grouped attempt data ot process.
+     * @param object $data contains all the grouped attempt data to process.
      * @param pbject $quiz data about the activity the attempts belong to. Required
      * fields are (basically this only works for the quiz module):
      *      oldquestions => list of question ids in this activity - using old ids.
@@ -4298,7 +4298,8 @@ protected function process_legacy_quiz_attempt_data($data, $quiz) {
         $this->inform_new_usage_id($usage->id);
 
         $data->uniqueid = $usage->id;
-        $upgrader->save_usage($quiz->preferredbehaviour, $data, $qas, $quiz->questions);
+        $upgrader->save_usage($quiz->preferredbehaviour, $data, $qas,
+                 $this->questions_recode_layout($quiz->oldquestions));
     }
 
     protected function find_question_session_and_states($data, $questionid) {

lib/questionlib.php

@@ -718,51 +718,59 @@ function question_preview_popup_params() {
  * to pull in extra data. See, for example, the usage in mod/quiz/attemptlib.php, and
  * read the code below to see how the SQL is assembled. Throws exceptions on error.
  *
- * @global object
- * @global object
- * @param array $questionids array of question ids.
+ * @param array $questionids array of question ids to load. If null, then all
+ * questions matched by $join will be loaded.
  * @param string $extrafields extra SQL code to be added to the query.
  * @param string $join extra SQL code to be added to the query.
  * @param array $extraparams values for any placeholders in $join.
- * You are strongly recommended to use named placeholder.
+ * You must use named placeholders.
+ * @param string $orderby what to order the results by. Optional, default is unspecified order.
  *
  * @return array partially complete question objects. You need to call get_question_options
  * on them before they can be properly used.
  */
-function question_preload_questions($questionids, $extrafields = '', $join = '',
-        $extraparams = array()) {
+function question_preload_questions($questionids = null, $extrafields = '', $join = '',
+        $extraparams = array(), $orderby = '') {
     global $DB;
-    if (empty($questionids)) {
-        return array();
+
+    if ($questionids === null) {
+        $where = '';
+        $params = array();
+    } else {
+        if (empty($questionids)) {
+            return array();
+        }
+
+        list($questionidcondition, $params) = $DB->get_in_or_equal(
+                $questionids, SQL_PARAMS_NAMED, 'qid0000');
+        $where = 'WHERE q.id ' . $questionidcondition;
     }
+
     if ($join) {
-        $join = ' JOIN '.$join;
+        $join = 'JOIN ' . $join;
     }
+
     if ($extrafields) {
         $extrafields = ', ' . $extrafields;
     }
-    list($questionidcondition, $params) = $DB->get_in_or_equal(
-            $questionids, SQL_PARAMS_NAMED, 'qid0000');
-    $sql = 'SELECT q.*, qc.contextid' . $extrafields . ' FROM {question} q
-            JOIN {question_categories} qc ON q.category = qc.id' .
-            $join .
-          ' WHERE q.id ' . $questionidcondition;
 
-    // Load the questions
-    if (!$questions = $DB->get_records_sql($sql, $extraparams + $params)) {
-        return array();
+    if ($orderby) {
+        $orderby = 'ORDER BY ' . $orderby;
     }
 
+    $sql = "SELECT q.*, qc.contextid{$extrafields}
+              FROM {question} q
+              JOIN {question_categories} qc ON q.category = qc.id
+              {$join}
+             {$where}
+          {$orderby}";
+
+    // Load the questions.
+    $questions = $DB->get_records_sql($sql, $extraparams + $params);
     foreach ($questions as $question) {
         $question->_partiallyloaded = true;
     }
 
-    // Note, a possible optimisation here would be to not load the TEXT fields
-    // (that is, questiontext and generalfeedback) here, and instead load them in
-    // question_load_questions. That would add one DB query, but reduce the amount
-    // of data transferred most of the time. I am not going to do this optimisation
-    // until it is shown to be worthwhile.
-
     return $questions;
 }
 

mod/quiz/accessrule/delaybetweenattempts/tests/rule_test.php

@@ -44,7 +44,6 @@ public function test_just_first_delay() {
         $quiz->delay1 = 1000;
         $quiz->delay2 = 0;
         $quiz->timeclose = 0;
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);
@@ -79,7 +78,6 @@ public function test_just_second_delay() {
         $quiz->delay1 = 0;
         $quiz->delay2 = 1000;
         $quiz->timeclose = 0;
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);
@@ -119,7 +117,6 @@ public function test_just_both_delays() {
         $quiz->delay1 = 2000;
         $quiz->delay2 = 1000;
         $quiz->timeclose = 0;
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);
@@ -171,7 +168,6 @@ public function test_with_close_date() {
         $quiz->delay1 = 2000;
         $quiz->delay2 = 1000;
         $quiz->timeclose = 15000;
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);
@@ -228,7 +224,6 @@ public function test_time_limit_and_overdue() {
         $quiz->delay1 = 2000;
         $quiz->delay2 = 1000;
         $quiz->timeclose = 0;
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);

mod/quiz/accessrule/ipaddress/tests/rule_test.php

@@ -48,7 +48,6 @@ public function test_ipaddress_access_rule() {
         // does not always work, for example using the mac install package on my laptop.
         $quiz->subnet = getremoteaddr(null);
         if (!empty($quiz->subnet)) {
-            $quiz->questions = '';
             $quizobj = new quiz($quiz, $cm, null);
             $rule = new quizaccess_ipaddress($quizobj, 0);
 
@@ -61,7 +60,6 @@ public function test_ipaddress_access_rule() {
         }
 
         $quiz->subnet = '0.0.0.0';
-        $quiz->questions = '';
         $quizobj = new quiz($quiz, $cm, null);
         $rule = new quizaccess_ipaddress($quizobj, 0);
 

mod/quiz/accessrule/numattempts/tests/rule_test.php

@@ -41,7 +41,6 @@ class quizaccess_numattempts_testcase extends basic_testcase {
     public function test_num_attempts_access_rule() {
         $quiz = new stdClass();
         $quiz->attempts = 3;
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);

mod/quiz/accessrule/openclosedate/tests/rule_test.php

@@ -43,7 +43,6 @@ public function test_no_dates() {
         $quiz->timeopen = 0;
         $quiz->timeclose = 0;
         $quiz->overduehandling = 'autosubmit';
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);
@@ -73,7 +72,6 @@ public function test_start_date() {
         $quiz->timeopen = 10000;
         $quiz->timeclose = 0;
         $quiz->overduehandling = 'autosubmit';
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);
@@ -105,7 +103,6 @@ public function test_close_date() {
         $quiz->timeopen = 0;
         $quiz->timeclose = 20000;
         $quiz->overduehandling = 'autosubmit';
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);
@@ -144,7 +141,6 @@ public function test_both_dates() {
         $quiz->timeopen = 10000;
         $quiz->timeclose = 20000;
         $quiz->overduehandling = 'autosubmit';
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);
@@ -197,7 +193,6 @@ public function test_close_date_with_overdue() {
         $quiz->timeclose = 20000;
         $quiz->overduehandling = 'graceperiod';
         $quiz->graceperiod = 1000;
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);

mod/quiz/accessrule/password/tests/rule_test.php

@@ -41,7 +41,6 @@ class quizaccess_password_testcase extends basic_testcase {
     public function test_password_access_rule() {
         $quiz = new stdClass();
         $quiz->password = 'frog';
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);

mod/quiz/accessrule/safebrowser/tests/rule_test.php

@@ -42,7 +42,6 @@ class quizaccess_safebrowser_testcase extends basic_testcase {
     public function test_safebrowser_access_rule() {
         $quiz = new stdClass();
         $quiz->browsersecurity = 'safebrowser';
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);

mod/quiz/accessrule/securewindow/tests/rule_test.php

@@ -43,7 +43,6 @@ class quizaccess_securewindow_testcase extends basic_testcase {
     public function test_securewindow_access_rule() {
         $quiz = new stdClass();
         $quiz->browsersecurity = 'securewindow';
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);

mod/quiz/accessrule/timelimit/tests/rule_test.php

@@ -40,7 +40,6 @@ class quizaccess_timelimit_testcase extends basic_testcase {
     public function test_time_limit_access_rule() {
         $quiz = new stdClass();
         $quiz->timelimit = 3600;
-        $quiz->questions = '';
         $cm = new stdClass();
         $cm->id = 0;
         $quizobj = new quiz($quiz, $cm, null);

mod/quiz/attemptlib.php

@@ -65,7 +65,6 @@ class quiz {
     protected $cm;
     protected $quiz;
     protected $context;
-    protected $questionids;
 
     // Fields set later if that data is needed.
     protected $questions = null;
@@ -89,12 +88,6 @@ public function __construct($quiz, $cm, $course, $getcontext = true) {
         if ($getcontext && !empty($cm->id)) {
             $this->context = context_module::instance($cm->id);
         }
-        $questionids = quiz_questions_in_quiz($this->quiz->questions);
-        if ($questionids) {
-            $this->questionids = explode(',', quiz_questions_in_quiz($this->quiz->questions));
-        } else {
-            $this->questionids = array(); // Which idiot made explode(',', '') = array('')?
-        }
     }
 
     /**
@@ -132,13 +125,10 @@ public function create_attempt_object($attemptdata) {
      * Load just basic information about all the questions in this quiz.
      */
     public function preload_questions() {
-        if (empty($this->questionids)) {
-            throw new moodle_quiz_exception($this, 'noquestions', $this->edit_url());
-        }
-        $this->questions = question_preload_questions($this->questionids,
-                'qqi.maxmark, qqi.id AS instance',
-                '{quiz_question_instances} qqi ON qqi.quizid = :quizid AND q.id = qqi.questionid',
-                array('quizid' => $this->quiz->id));
+        $this->questions = question_preload_questions(null,
+                'slot.maxmark, slot.id AS slotid, slot.slot, slot.page',
+                '{quiz_slots} slot ON slot.quizid = :quizid AND q.id = slot.questionid',
+                array('quizid' => $this->quiz->id), 'slot.slot');
     }
 
     /**
@@ -148,8 +138,11 @@ public function preload_questions() {
      * @param array $questionids question ids of the questions to load. null for all.
      */
     public function load_questions($questionids = null) {
+        if ($this->questions === null) {
+            throw new coding_exception('You must call preload_questions before calling load_questions.');
+        }
         if (is_null($questionids)) {
-            $questionids = $this->questionids;
+            $questionids = array_keys($this->questions);
         }
         $questionstoprocess = array();
         foreach ($questionids as $id) {
@@ -226,7 +219,10 @@ public function is_preview_user() {
      * @return whether any questions have been added to this quiz.
      */
     public function has_questions() {
-        return !empty($this->questionids);
+        if ($this->questions === null) {
+            $this->preload_questions();
+        }
+        return !empty($this->questions);
     }
 
     /**
@@ -242,7 +238,7 @@ public function get_question($id) {
      */
     public function get_questions($questionids = null) {
         if (is_null($questionids)) {
-            $questionids = $this->questionids;
+            $questionids = array_keys($this->questions);
         }
         $questions = array();
         foreach ($questionids as $id) {
@@ -530,7 +526,7 @@ private function determine_layout() {
         $this->pagelayout = array();
 
         // Break up the layout string into pages.
-        $pagelayouts = explode(',0', quiz_clean_layout($this->attempt->layout, true));
+        $pagelayouts = explode(',0', $this->attempt->layout);
 
         // Strip off any empty last page (normally there is one).
         if (end($pagelayouts) == '') {

mod/quiz/backup/moodle2/backup_quiz_stepslib.php

@@ -47,7 +47,7 @@ protected function define_structure() {
             'reviewspecificfeedback', 'reviewgeneralfeedback',
             'reviewrightanswer', 'reviewoverallfeedback',
             'questionsperpage', 'navmethod', 'shufflequestions', 'shuffleanswers',
-            'questions', 'sumgrades', 'grade', 'timecreated',
+            'sumgrades', 'grade', 'timecreated',
             'timemodified', 'password', 'subnet', 'browsersecurity',
             'delay1', 'delay2', 'showuserpicture', 'showblocks'));
 
@@ -57,7 +57,7 @@ protected function define_structure() {
         $qinstances = new backup_nested_element('question_instances');
 
         $qinstance = new backup_nested_element('question_instance', array('id'), array(
-            'questionid', 'maxmark'));
+            'slot', 'page', 'questionid', 'maxmark'));
 
         $feedbacks = new backup_nested_element('feedbacks');
 
@@ -107,7 +107,7 @@ protected function define_structure() {
         // Define sources.
         $quiz->set_source_table('quiz', array('id' => backup::VAR_ACTIVITYID));
 
-        $qinstance->set_source_table('quiz_question_instances',
+        $qinstance->set_source_table('quiz_slots',
                 array('quizid' => backup::VAR_PARENTID));
 
         $feedback->set_source_table('quiz_feedback',

mod/quiz/backup/moodle2/restore_quiz_stepslib.php

@@ -92,9 +92,10 @@ protected function process_quiz($data) {
         $data->timecreated = $this->apply_date_offset($data->timecreated);
         $data->timemodified = $this->apply_date_offset($data->timemodified);
 
-        // Needed by {@link process_quiz_attempt_legacy}.
-        $this->oldquizlayout = $data->questions;
-        $data->questions = $this->questions_recode_layout($data->questions);
+        if (property_exists($data, 'questions')) {
+            // Needed by {@link process_quiz_attempt_legacy}, in which case it will be present.
+            $this->oldquizlayout = $data->questions;
+        }
 
         // The setting quiz->attempts can come both in data->attempts and
         // data->attempts_number, handle both. MDL-26229.
@@ -243,10 +244,36 @@ protected function process_quiz_question_instance($data) {
             $data->maxmark = $data->grade;
         }
 
+        if (!property_exists($data, 'slot')) {
+            $page = 1;
+            $slot = 1;
+            foreach (explode(',', $this->oldquizlayout) as $item) {
+                if ($item == 0) {
+                    $page += 1;
+                    continue;
+                }
+                if ($item == $data->questionid) {
+                    $data->slot = $slot;
+                    $data->page = $page;
+                    break;
+                }
+                $slot += 1;
+            }
+        }
+
+        if (!property_exists($data, 'slot')) {
+            // There was a question_instance in the backup file for a question
+            // that was not acutally in the quiz. Drop it.
+            $this->log('question ' . $data->questionid . ' was associated with quiz ' .
+                    $quiz->id . ' but not actually used. ' .
+                    'The instance has been ignored.', backup::LOG_INFO);
+            return;
+        }
+
         $data->quizid = $this->get_new_parentid('quiz');
         $data->questionid = $this->get_mappingid('question', $data->questionid);
 
-        $DB->insert_record('quiz_question_instances', $data);
+        $DB->insert_record('quiz_slots', $data);
     }
 
     protected function process_quiz_feedback($data) {