diff --git a/lib/questionlib.php b/lib/questionlib.php index 61764ed59ea..22fa4f711e2 100644 --- a/lib/questionlib.php +++ b/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. - * @return boolean this user has the capability $cap for this question $question? + * @param int $notused no longer used. + * @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) { - $question = question_bank::load_question_data($questionorid->id); + // At this point, either $question or $questionid is expected to be set. + 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.'); } diff --git a/lib/tests/questionlib_test.php b/lib/tests/questionlib_test.php index ced468bbfb2..ae1cd80c23c 100644 --- a/lib/tests/questionlib_test.php +++ b/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. * diff --git a/question/engine/bank.php b/question/engine/bank.php index 6721cbfc05b..68c6aaf1357 100644 --- a/question/engine/bank.php +++ b/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); }