MDL-63809 question: handling bad questions in question_has_capability_on

lib/questionlib.php

@@ -921,8 +921,6 @@ function question_load_questions($questionids, $extrafields = '', $join = '') {
  * @param stdClass[]|null $filtercourses The courses to filter the course tags by.
  */
 function _tidy_question($question, $category, array $tagobjects = null, array $filtercourses = null) {
-    global $CFG;
-
     // Load question-type specific fields.
     if (!question_bank::is_qtype_installed($question->qtype)) {
         $question->questiontext = html_writer::tag('p', get_string('warningmissingtype',
@@ -1651,25 +1649,44 @@ class context_to_string_translator{
 /**
  * Check capability on category
  *
- * @param mixed $questionorid object or id. If an object is passed, it should include ->contextid and ->createdby.
+ * @param int|stdClass $questionorid object or id. If an object is passed, it should include ->contextid and ->createdby.
  * @param string $cap 'add', 'edit', 'view', 'use', 'move' or 'tag'.
- * @param integer $notused no longer used.
+ * @param int $notused no longer used.
- * @return boolean this user has the capability $cap for this question $question?
+ * @return bool this user has the capability $cap for this question $question?
+ * @throws coding_exception
  */
 function question_has_capability_on($questionorid, $cap, $notused = -1) {
-    global $USER;
+    global $USER, $DB;
 
     if (is_numeric($questionorid)) {
-        $question = question_bank::load_question_data((int)$questionorid);
+        $questionid = (int)$questionorid;
     } else if (is_object($questionorid)) {
+        // All we really need in this function is the contextid and author of the question.
+        // We won't bother fetching other details of the question if these 2 fields are provided.
         if (isset($questionorid->contextid) && isset($questionorid->createdby)) {
             $question = $questionorid;
+        } else if (!empty($questionorid->id)) {
+            $questionid = $questionorid->id;
         }
+    }
 
-        if (!isset($question) && isset($questionorid->id) && $questionorid->id != 0) {
+    // At this point, either $question or $questionid is expected to be set.
-            $question = question_bank::load_question_data($questionorid->id);
+    if (isset($questionid)) {
+        try {
+            $question = question_bank::load_question_data($questionid);
+        } catch (Exception $e) {
+            // Let's log the exception for future debugging.
+            debugging($e->getMessage(), DEBUG_NORMAL, $e->getTrace());
+
+            // Well, at least we tried. Seems that we really have to read from DB.
+            $question = $DB->get_record_sql('SELECT q.id, q.createdby, qc.contextid
+                                               FROM {question} q
+                                               JOIN {question_categories} qc ON q.category = qc.id
+                                              WHERE q.id = :id', ['id' => $questionid]);
         }
-    } else {
+    }
+
+    if (!isset($question)) {
         throw new coding_exception('$questionorid parameter needs to be an integer or an object.');
     }
 

lib/tests/questionlib_test.php

@@ -1643,6 +1643,37 @@ class core_questionlib_testcase extends advanced_testcase {
         ];
     }
 
+    /**
+     * Tests that question_has_capability_on does not throw exception on broken questions.
+     */
+    public function test_question_has_capability_on_broken_question() {
+        global $DB;
+
+        // Create the test data.
+        $generator = $this->getDataGenerator();
+        $questiongenerator = $generator->get_plugin_generator('core_question');
+
+        $category = $generator->create_category();
+        $context = context_coursecat::instance($category->id);
+        $questioncat = $questiongenerator->create_question_category([
+            'contextid' => $context->id,
+        ]);
+
+        // Create a cloze question.
+        $question = $questiongenerator->create_question('multianswer', null, [
+            'category' => $questioncat->id,
+        ]);
+        // Now, break the question.
+        $DB->delete_records('question_multianswer', ['question' => $question->id]);
+
+        $this->setAdminUser();
+
+        $result = question_has_capability_on($question->id, 'tag');
+        $this->assertTrue($result);
+
+        $this->assertDebuggingCalled();
+    }
+
     /**
      * Tests for the deprecated question_has_capability_on function when passing a stdClass as parameter.
      *

question/engine/bank.php

@@ -267,7 +267,7 @@ abstract class question_bank {
         global $DB;
 
         if (self::$testmode) {
-            // Evil, test code in production, but now way round it.
+            // Evil, test code in production, but no way round it.
             return self::return_test_question_data($questionid);
         }