From 3f5b13cd4992158933969dd587e2eceafd559640 Mon Sep 17 00:00:00 2001 From: Philipp Memmel Date: Sun, 5 Oct 2025 17:39:25 +0000 Subject: [PATCH 1/2] MDL-79221 core_completion: Do not update state for disabled plugins --- public/lib/completionlib.php | 10 ++++++- public/lib/tests/completionlib_test.php | 39 ++++++++++++++++++++----- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/public/lib/completionlib.php b/public/lib/completionlib.php index 19df8dbccf4..d86e408ff07 100644 --- a/public/lib/completionlib.php +++ b/public/lib/completionlib.php @@ -573,7 +573,15 @@ class completion_info { */ public function update_state($cm, $possibleresult=COMPLETION_UNKNOWN, $userid=0, $override = false, $isbulkupdate = false) { - global $USER; + global $DB, $USER; + + // Do nothing if the mod plugin type is disabled. + $manager = \core_plugin_manager::resolve_plugininfo_class('mod'); + $modname = !empty($cm->modname) ? $cm->modname : $DB->get_field('modules', 'name', ['id' => $cm->module]); + $enabled = $manager::get_enabled_plugin($modname); + if (!$enabled) { + return; + } // Do nothing if completion is not enabled for that activity if (!$this->is_enabled($cm)) { diff --git a/public/lib/tests/completionlib_test.php b/public/lib/tests/completionlib_test.php index a2b84ab8fde..f276e173d34 100644 --- a/public/lib/tests/completionlib_test.php +++ b/public/lib/tests/completionlib_test.php @@ -137,20 +137,43 @@ final class completionlib_test extends advanced_testcase { * @covers ::update_state */ public function test_update_state(): void { + global $DB; $this->mock_setup(); $mockbuilder = $this->getMockBuilder('completion_info'); $mockbuilder->onlyMethods(array('is_enabled', 'get_data', 'internal_get_state', 'internal_set_data', 'user_can_override_completion')); $mockbuilder->setConstructorArgs(array((object)array('id' => 42))); - $cm = (object)array('id' => 13, 'course' => 42); + $cm = (object) ['id' => 13, 'modname' => 'forum', 'course' => 42]; + + // Mock check for enabled plugins. First simulate that no activity plugin type is enabled, especially not + // the forum plugin which we are using in this test. + $enabledplugins = []; + /** @var PHPUnit\Framework\MockObject\MockObject $DB */ + $DB->expects($this->any()) + ->method('get_records_menu') + ->with('modules', ['visible' => 1], 'name ASC', 'name, name AS val') + ->willReturnCallback(function () use (&$enabledplugins) { + return $enabledplugins; + }); + + // If the forum plugin is not enabled, the update_state method will not get to the point where the is_enabled + // method is being called, but will early exit before. + $c = $mockbuilder->getMock(); + $c->expects($this->never()) + ->method('is_enabled') + ->with($cm) + ->will($this->returnValue(false)); + $c->update_state($cm); + + // Enable forum plugin type for the rest of the test method. + $enabledplugins = ['forum' => 'forum']; - // Not enabled, should do nothing. $c = $mockbuilder->getMock(); $c->expects($this->once()) ->method('is_enabled') ->with($cm) - ->will($this->returnValue(false)); + ->willReturn(false); $c->update_state($cm); // Enabled, but current state is same as possible result, do nothing. @@ -181,7 +204,7 @@ final class completionlib_test extends advanced_testcase { // Manual, change state (no change). $c = $mockbuilder->getMock(); - $cm = (object)array('id' => 13, 'course' => 42, 'completion' => COMPLETION_TRACKING_MANUAL); + $cm = (object) ['id' => 13, 'modname' => 'forum', 'course' => 42, 'completion' => COMPLETION_TRACKING_MANUAL]; $current->completionstate = COMPLETION_COMPLETE; $c->expects($this->once()) ->method('is_enabled') @@ -213,7 +236,7 @@ final class completionlib_test extends advanced_testcase { // Auto, change state. $c = $mockbuilder->getMock(); - $cm = (object)array('id' => 13, 'course' => 42, 'completion' => COMPLETION_TRACKING_AUTOMATIC); + $cm = (object) ['id' => 13, 'modname' => 'forum', 'course' => 42, 'completion' => COMPLETION_TRACKING_AUTOMATIC]; $current = (object)array('completionstate' => COMPLETION_COMPLETE, 'overrideby' => null); $c->expects($this->once()) ->method('is_enabled') @@ -237,7 +260,7 @@ final class completionlib_test extends advanced_testcase { // Manual tracking, change state by overriding it manually. $c = $mockbuilder->getMock(); - $cm = (object)array('id' => 13, 'course' => 42, 'completion' => COMPLETION_TRACKING_MANUAL); + $cm = (object) ['id' => 13, 'modname' => 'forum', 'course' => 42, 'completion' => COMPLETION_TRACKING_MANUAL]; $current1 = (object)array('completionstate' => COMPLETION_INCOMPLETE, 'overrideby' => null); $current2 = (object)array('completionstate' => COMPLETION_COMPLETE, 'overrideby' => null); $c->expects($this->exactly(2)) @@ -298,7 +321,7 @@ final class completionlib_test extends advanced_testcase { // Auto, change state via override, incomplete to complete. $c = $mockbuilder->getMock(); - $cm = (object)array('id' => 13, 'course' => 42, 'completion' => COMPLETION_TRACKING_AUTOMATIC); + $cm = (object) ['id' => 13, 'modname' => 'forum', 'course' => 42, 'completion' => COMPLETION_TRACKING_AUTOMATIC]; $current = (object)array('completionstate' => COMPLETION_INCOMPLETE, 'overrideby' => null); $c->expects($this->once()) ->method('is_enabled') @@ -324,7 +347,7 @@ final class completionlib_test extends advanced_testcase { // Now confirm the status can be changed back from complete to incomplete using an override. $c = $mockbuilder->getMock(); - $cm = (object)array('id' => 13, 'course' => 42, 'completion' => COMPLETION_TRACKING_AUTOMATIC); + $cm = (object) ['id' => 13, 'modname' => 'forum', 'course' => 42, 'completion' => COMPLETION_TRACKING_AUTOMATIC]; $current = (object)array('completionstate' => COMPLETION_COMPLETE, 'overrideby' => 2); $c->expects($this->once()) ->method('is_enabled') From c629f23da85dc6728e3ff6e6dd4334375c95b8a8 Mon Sep 17 00:00:00 2001 From: Philipp Memmel Date: Tue, 18 Nov 2025 08:46:41 +0000 Subject: [PATCH 2/2] MDL-79221 core_grade: Fix fetching context for disabled mod plugins Activity plugins that are disabled on the site are not included in the modinfo cache. That's why their context must not be fetched via modinfo cache. --- public/lib/grade/grade_item.php | 17 +++++-- public/lib/grade/tests/fixtures/lib.php | 24 +++++++++ public/lib/grade/tests/grade_item_test.php | 59 +++++++++++++++++++++- 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/public/lib/grade/grade_item.php b/public/lib/grade/grade_item.php index cd7a70ce839..c5144f58697 100644 --- a/public/lib/grade/grade_item.php +++ b/public/lib/grade/grade_item.php @@ -2633,12 +2633,19 @@ class grade_item extends grade_object { rebuild_course_cache($this->courseid, true); $modinfo = get_fast_modinfo($this->courseid); } - // Even with a rebuilt cache the module does not exist. This means the - // database is in an invalid state - we will log an error and return - // the course context but the calling code should be updated. + + // Even with a rebuilt cache the module does not exist. This means we are dealing + // with a mod plugin type that is disabled on the site (which are not included in the + // modinfo cache) or the database is in an invalid state. In the latter case we will + // log an error and return the course context, but the calling code should be updated. if (!isset($modinfo->instances[$this->itemmodule][$this->iteminstance])) { - mtrace(get_string('moduleinstancedoesnotexist', 'error')); - $context = \context_course::instance($this->courseid); + if ($cm = get_coursemodule_from_instance($this->itemmodule, $this->iteminstance)) { + // Cache does not contain module plugins that are disabled. + $context = \context_module::instance($cm->id); + } else { + debugging(get_string('moduleinstancedoesnotexist', 'error')); + $context = \context_course::instance($this->courseid); + } } else { $cm = $modinfo->instances[$this->itemmodule][$this->iteminstance]; $context = \context_module::instance($cm->id); diff --git a/public/lib/grade/tests/fixtures/lib.php b/public/lib/grade/tests/fixtures/lib.php index 1aa0b0d2d6e..a33981bc407 100644 --- a/public/lib/grade/tests/fixtures/lib.php +++ b/public/lib/grade/tests/fixtures/lib.php @@ -109,6 +109,9 @@ abstract class grade_base_testcase extends advanced_testcase { $this->activities[7] = $this->getDataGenerator()->create_module('quiz', array('course'=>$this->course->id)); $this->course_module[7] = get_coursemodule_from_instance('quiz', $this->activities[7]->id); + + $this->activities[8] = $this->getDataGenerator()->create_module('forum', ['course' => $this->course->id]); + $this->course_module[8] = get_coursemodule_from_instance('forum', $this->activities[8]->id); } private function load_scales() { @@ -677,6 +680,27 @@ abstract class grade_base_testcase extends advanced_testcase { $this->grade_items[16] = $grade_item; // $this->grade_items[17] loaded in load_grade_outcomes() in order to use an outcome id. + + // Item with id = 18: Is being used to test what happens if the related mod plugin is disabled. + $grade_item = new stdClass(); + + $grade_item->courseid = $this->course->id; + $grade_item->categoryid = $this->grade_categories[3]->id; + $grade_item->itemname = 'singleparentitem1'; + $grade_item->itemtype = 'mod'; + $grade_item->itemmodule = $this->course_module[8]->modname; + $grade_item->iteminstance = $this->course_module[8]->instance; + $grade_item->gradetype = GRADE_TYPE_SCALE; + $grade_item->scaleid = $this->scale[0]->id; + $grade_item->grademin = 0; + $grade_item->grademax = $this->scalemax[0]; + $grade_item->iteminfo = 'Grade item 18 used for unit testing'; + $grade_item->timecreated = time(); + $grade_item->timemodified = time(); + $grade_item->sortorder = 18; + + $grade_item->id = $DB->insert_record('grade_items', $grade_item); + $this->grade_items[18] = $grade_item; } /** diff --git a/public/lib/grade/tests/grade_item_test.php b/public/lib/grade/tests/grade_item_test.php index 15048c4c26c..2d07a0c4bc5 100644 --- a/public/lib/grade/tests/grade_item_test.php +++ b/public/lib/grade/tests/grade_item_test.php @@ -33,6 +33,7 @@ final class grade_item_test extends \grade_base_testcase { $this->sub_test_grade_item_construct(); $this->sub_test_grade_item_insert(); $this->sub_test_grade_item_delete(); + $this->sub_test_grade_item_delete_disabled_modules(); $this->sub_test_grade_item_update(); $this->sub_test_grade_item_load_scale(); $this->sub_test_grade_item_load_outcome(); @@ -108,7 +109,7 @@ final class grade_item_test extends \grade_base_testcase { $last_grade_item = end($this->grade_items); $this->assertEquals($grade_item->id, $last_grade_item->id + 1); - $this->assertEquals(18, $grade_item->sortorder); + $this->assertEquals(19, $grade_item->sortorder); // Keep our reference collection the same as what is in the database. $this->grade_items[] = $grade_item; @@ -150,6 +151,62 @@ final class grade_item_test extends \grade_base_testcase { unset($this->grade_items[7]); } + /** + * Tests the deletion of a grade item of an activity instance that has been globally disabled. + * + * @covers \grade_item::delete + */ + protected function sub_test_grade_item_delete_disabled_modules(): void { + global $CFG, $DB; + require_once($CFG->dirroot . '/course/lib.php'); + $grade_item = new \grade_item($this->grade_items[18], false); // Use a grade item not touched by previous (or future) tests. + $this->assertTrue(method_exists($grade_item, 'delete')); + + // Add two files. + $dummy = [ + 'contextid' => $grade_item->get_context()->id, + 'component' => GRADE_FILE_COMPONENT, + 'filearea' => GRADE_HISTORY_FEEDBACK_FILEAREA, + 'itemid' => 1, + 'filepath' => '/', + 'filename' => 'feedback1.txt', + ]; + + $fs = get_file_storage(); + $fs->create_file_from_string($dummy, ''); + + $dummy['itemid'] = 2; + $fs->create_file_from_string($dummy, ''); + + $files = $fs->get_area_files($grade_item->get_context()->id, GRADE_FILE_COMPONENT, GRADE_HISTORY_FEEDBACK_FILEAREA); + // Includes directories. + $this->assertCount(4, $files); + + // Now disable the mod plugin. + $class = \core_plugin_manager::resolve_plugininfo_class('mod'); + $class::enable_plugin($this->course_module[8]->modname, false); + $this->assertFalse(in_array($this->course_module[8]->modname, \core\plugininfo\mod::get_enabled_plugins())); + rebuild_course_cache($this->course->id); + + ob_start(); + $this->assertTrue($grade_item->delete()); + $result = ob_get_contents(); + ob_end_clean(); + // No mtrace warning about missing module when fetching the context should appear. + $this->assertEmpty($result); + + $this->assertFalse($DB->get_record('grade_items', ['id' => $grade_item->id])); + + $files = $fs->get_area_files($grade_item->get_context()->id, GRADE_FILE_COMPONENT, GRADE_HISTORY_FEEDBACK_FILEAREA); + $this->assertEmpty($files); + + // Keep our reference collection the same as the database. + unset($this->grade_items[18]); + $class::enable_plugin($this->course_module[8]->modname, true); + $this->assertTrue(in_array($this->course_module[8]->modname, \core\plugininfo\mod::get_enabled_plugins())); + rebuild_course_cache($this->course->id); + } + protected function sub_test_grade_item_update() { global $DB; $grade_item = new \grade_item($this->grade_items[0], false);