Merge branch 'MDL-80127' of https://github.com/timhunt/moodle

question/engine/datalib.php

@@ -149,6 +149,7 @@ public function insert_question_attempt(question_attempt $qa, $context) {
 
     /**
      * Helper method used by insert_question_attempt_step and update_question_attempt_step
+     *
      * @param question_attempt_step $step the step to store.
      * @param int $questionattemptid the question attept id this step belongs to.
      * @param int $seq the sequence number of this stop.
@@ -158,7 +159,7 @@ protected function make_step_record(question_attempt_step $step, $questionattemp
         $record = new stdClass();
         $record->questionattemptid = $questionattemptid;
         $record->sequencenumber = $seq;
-        $record->state = (string) $step->get_state();
+        $record->state = $step->get_state()?->__toString();
         $record->fraction = $step->get_fraction();
         $record->timecreated = $step->get_timecreated();
         $record->userid = $step->get_user_id();

question/engine/states.php

@@ -111,10 +111,23 @@ public function __toString() {
     }
 
     /**
+     * Get the instance of this class for a given state name.
+     *
      * @param string $name a state name.
-     * @return question_state the state with that name.
+     * @return question_state|null the state with that name. (Null only in an exceptional case.)
      */
-    public static function get($name) {
+    public static function get(string $name): ?question_state {
+        // In the past, there was a bug where null states got stored
+        // in the database as an empty string, which was wrong because
+        // the state column should be NOT NULL.
+        // That is no longer possible, but we need to avoid exceptions
+        // for people with old bad data in their database.
+        if ($name === '') {
+            debugging('Attempt to create a state from an empty string. ' .
+                'This is probably a sign of bad data in your database. See MDL-80127.');
+            return null;
+        }
+
         return self::$$name;
     }
 

question/engine/tests/datalib_test.php

@@ -40,6 +40,7 @@
  * @category  test
  * @copyright 2014 The Open University
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @covers \question_engine_data_mapper
  */
 class datalib_test extends \qbehaviour_walkthrough_test_base {
 
@@ -251,6 +252,34 @@ public function test_save_and_load_an_empty_usage() {
         question_engine::delete_questions_usage_by_activity($quba->get_id());
     }
 
+    public function test_cannot_save_a_step_with_a_missing_state(): void {
+        global $DB;
+
+        $this->resetAfterTest();
+
+        // Create a question.
+        $generator = $this->getDataGenerator()->get_plugin_generator('core_question');
+        $cat = $generator->create_question_category();
+        $questiondata = $generator->create_question('shortanswer', null, ['category' => $cat->id]);
+
+        // Create a usage.
+        $quba = question_engine::make_questions_usage_by_activity('test', \context_system::instance());
+        $quba->set_preferred_behaviour('deferredfeedback');
+        $slot = $quba->add_question(question_bank::load_question($questiondata->id));
+        $quba->start_all_questions();
+
+        // Add a step with a bad state.
+        $newstep = new \question_attempt_step();
+        $newstep->set_state(null);
+        $addstepmethod = new \ReflectionMethod('question_attempt', 'add_step');
+        $addstepmethod->setAccessible(true);
+        $addstepmethod->invoke($quba->get_question_attempt($slot), $newstep);
+
+        // Verify that trying to save this throws an exception.
+        $this->expectException(\dml_write_exception::class);
+        question_engine::save_questions_usage_by_activity($quba);
+    }
+
     /**
      * Test cases for {@see test_get_file_area_name()}.
      *

question/engine/tests/questionstate_test.php

@@ -31,6 +31,7 @@
  * @category   test
  * @copyright  2009 The Open University
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @covers \question_state
  */
 class questionstate_test extends \advanced_testcase {
     public function test_is_active() {
@@ -154,4 +155,14 @@ public function test_manually_graded_state_for_other_state() {
         $this->assertEquals(question_state::$mangrright,
                 question_state::$gradedpartial->corresponding_commented_state(1));
     }
+
+    public function test_get(): void {
+        $this->assertEquals(question_state::$todo, question_state::get('todo'));
+    }
+
+    public function test_get_bad_data(): void {
+        question_state::get('');
+        $this->assertDebuggingCalled('Attempt to create a state from an empty string. ' .
+                'This is probably a sign of bad data in your database. See MDL-80127.');
+    }
 }