From cefc1f0166fc73dd99a7ef82b28785a271d22081 Mon Sep 17 00:00:00 2001 From: Laurent David Date: Wed, 15 Oct 2025 12:47:28 +0200 Subject: [PATCH] MDL-86854 core_course: refactor moveto_module other usages --- .upgradenotes/MDL-86854-2025101706145612.yml | 8 +++ public/availability/tests/info_test.php | 8 ++- .../format/classes/local/sectionactions.php | 5 +- public/course/format/classes/stateactions.php | 11 +++- .../format/tests/local/baseactions_test.php | 4 +- public/course/lib.php | 65 ++++++++++--------- public/course/mod.php | 9 ++- public/course/rest.php | 10 ++- public/course/tests/courselib_test.php | 9 ++- 9 files changed, 86 insertions(+), 43 deletions(-) create mode 100644 .upgradenotes/MDL-86854-2025101706145612.yml diff --git a/.upgradenotes/MDL-86854-2025101706145612.yml b/.upgradenotes/MDL-86854-2025101706145612.yml new file mode 100644 index 00000000000..58ba1a9d4ef --- /dev/null +++ b/.upgradenotes/MDL-86854-2025101706145612.yml @@ -0,0 +1,8 @@ +issueNumber: MDL-86854 +notes: + core_course: + - message: >- + Deprecates moveto_module (core_course) in favor of + cmactions::move_before or cmactions::move_end_section + (core_courseformat\local\cmactions). + type: deprecated diff --git a/public/availability/tests/info_test.php b/public/availability/tests/info_test.php index 456cfa63776..afd94934e4d 100644 --- a/public/availability/tests/info_test.php +++ b/public/availability/tests/info_test.php @@ -16,6 +16,8 @@ namespace core_availability; +use core_courseformat\formatactions; + /** * Unit tests for info and subclasses. * @@ -170,7 +172,8 @@ final class info_test extends \advanced_testcase { $modinfo = get_fast_modinfo($course); $section = $modinfo->get_section_info(1); $cm = $modinfo->get_cm($pages[2]->cmid); - moveto_module($cm, $section); + $cmactions = formatactions::cm($course); + $cmactions->move_end_section($cm->id, $section->id); // Set the availability restrictions in database. The enableavailability // setting is off so these do not take effect yet. @@ -417,7 +420,8 @@ final class info_test extends \advanced_testcase { $DB->set_field('course_sections', 'availability', '{"op":"|","show":true,"c":[{"type":"mock","filter":[' . $u1->id . ',' . $u2->id .']}]}', array('id' => $section2->id)); - moveto_module($modinfo->get_cm($page2->cmid), $section2); + $cmactions = formatactions::cm($course); + $cmactions->move_end_section($page2->cmid, $section2->id); // With no restrictions, returns full list. $info = new info_module($modinfo->get_cm($page->cmid)); diff --git a/public/course/format/classes/local/sectionactions.php b/public/course/format/classes/local/sectionactions.php index efd8b8b5410..a9d4c4a3bf0 100644 --- a/public/course/format/classes/local/sectionactions.php +++ b/public/course/format/classes/local/sectionactions.php @@ -16,6 +16,7 @@ namespace core_courseformat\local; +use core_courseformat\formatactions; use section_info; use stdClass; use core\event\course_module_updated; @@ -335,9 +336,11 @@ class sectionactions extends baseactions { // Move all modules to section 0. $modinfo = get_fast_modinfo($this->course->id); + $action = formatactions::cm($this->course); + $section0 = $modinfo->get_section_info(0); foreach ($modinfo->get_cms() as $cm) { if ($cm->sectionnum == $sectioninfo->section) { - moveto_module($cm, $modinfo->get_section_info(0)); + $action->move_end_section($cm->id, $section0->id); } } diff --git a/public/course/format/classes/stateactions.php b/public/course/format/classes/stateactions.php index e350cb9536f..14a1cdffd0c 100644 --- a/public/course/format/classes/stateactions.php +++ b/public/course/format/classes/stateactions.php @@ -594,10 +594,19 @@ class stateactions { // Duplicate course modules. $affectedcmids = []; + $action = formatactions::cm($course); foreach ($cms as $cm) { if ($newcm = duplicate_module($course, $cm)) { if ($targetsection) { - moveto_module($newcm, $targetsection, $beforecm); + if ($beforecm) { + $action->move_before($newcm->id, $beforecm->id); + } else { + // We retrieve the target section directly from the cache to avoid stale information in the section info. + $action->move_end_section( + $newcm->id, + $targetsection->id, + ); + } } else { $affectedcmids[] = $newcm->id; } diff --git a/public/course/format/tests/local/baseactions_test.php b/public/course/format/tests/local/baseactions_test.php index 0c3057910ba..3c8dc850016 100644 --- a/public/course/format/tests/local/baseactions_test.php +++ b/public/course/format/tests/local/baseactions_test.php @@ -15,6 +15,7 @@ // along with Moodle. If not, see . namespace core_courseformat\local; +use core_courseformat\formatactions; use ReflectionMethod; use section_info; use cm_info; @@ -136,7 +137,8 @@ final class baseactions_test extends \advanced_testcase { $this->assertEquals($originalcm->name, $cm->name); // CM info should be always the most updated one. - moveto_module($originalcm, $destinationsection); + $formatactions = formatactions::cm($course); + $formatactions->move_end_section($originalcm->id, $destinationsection->id); $cm = $method->invoke($baseactions, $originalcm->id); $this->assertInstanceOf(cm_info::class, $cm); diff --git a/public/course/lib.php b/public/course/lib.php index 344d53eade0..150ebe0f736 100644 --- a/public/course/lib.php +++ b/public/course/lib.php @@ -1176,42 +1176,41 @@ function reorder_sections($sections, $origin_position, $target_position) { * before which the module needs to be included. Null for inserting in the * end of the section * @return int new value for module visibility (0 or 1) + * @todo Remove this method in Moodle 6.0 (MDL-87465). */ +#[\core\attribute\deprecated( + replacement: 'core_courseformat\local\cmactions', + since: '5.2', + mdl: 'MDL-86854', + reason: 'Replaced by an cmactions::move_before or cmactions::move_end_section.', +)] function moveto_module($mod, $section, $beforemod=NULL) { - global $OUTPUT, $DB; + \core\deprecation::emit_deprecation(__FUNCTION__); if ($section->section != 0 && !course_modinfo::is_mod_type_visible_on_course($mod->modname)) { throw new coding_exception("Modules with FEATURE_CAN_DISPLAY set to false can not be moved from section 0"); } - - // Current module visibility state - return value of this function. - $modvisible = $mod->visible; - - // Remove original module from original section. - if (! delete_mod_from_section($mod->id, $mod->section)) { - echo $OUTPUT->notification("Could not delete module from existing section"); + [$course, $cm] = get_course_and_cm_from_cmid($mod->id); + $action = \core_courseformat\formatactions::cm($course); + if ($beforemod) { + $action->move_before($cm->id, $beforemod->id); + } else { + // We retrieve the target section directly from the cache to avoid stale information in the section info. + $action->move_end_section( + $cm->id, + $section->id, + ); } - - // Add the module into the new section. - course_add_cm_to_section($section->course, $mod->id, $section->section, $beforemod, $mod->modname); - - // If moving to a hidden section then hide module. - if ($mod->section != $section->id) { - if (!$section->visible && $mod->visible) { - // Module was visible but must become hidden after moving to hidden section. - $modvisible = 0; - set_coursemodule_visible($mod->id, 0); - // Set visibleold to 1 so module will be visible when section is made visible. - $DB->set_field('course_modules', 'visibleold', 1, array('id' => $mod->id)); - } - if ($section->visible && !$mod->visible) { - // Hidden module was moved to the visible section, restore the module visibility from visibleold. - set_coursemodule_visible($mod->id, $mod->visibleold); - $modvisible = $mod->visibleold; - } - } - - return $modvisible; + $modinfo = get_fast_modinfo($course); + $cm = $modinfo->get_cm($mod->id); + $modvisibility = $cm->visible; + // Purge the cm cache to ensure visibility changes are reflected. + // This was done last in the original method so we need to keep this here for backward compatibility. + // The explanation is that get_fast_modinfo was sometimes called with the last parameter to true in order to purge the cache. + // But this is not working well, so removing the following line will lead to a unit test failure for + // info_test::test_is_user_visible as the course module visibility is not refreshed properly. + \course_modinfo::purge_course_module_cache($cm->course, $cm->id); + return $modvisibility; } /** @@ -2657,14 +2656,16 @@ function duplicate_module($course, $cm, ?int $sectionid = null, bool $changename set_coursemodule_name($newcm->id, $newname); } - $section = $DB->get_record('course_sections', ['id' => $sectionid ?? $cm->section, 'course' => $cm->course]); + $section = get_fast_modinfo($course)->get_section_info_by_id($sectionid ?? $cm->section); + $action = formatactions::cm($course); if (isset($sectionid)) { - moveto_module($newcm, $section); + $action->move_end_section($newcm->id, $section->id); } else { $modarray = explode(",", trim($section->sequence)); $cmindex = array_search($cm->id, $modarray); if ($cmindex !== false && $cmindex < count($modarray) - 1) { - moveto_module($newcm, $section, $modarray[$cmindex + 1]); + $beforecmid = $modarray[$cmindex + 1]; + $action->move_before($newcm->id, $beforecmid); } } diff --git a/public/course/mod.php b/public/course/mod.php index d19195d9564..ddf5f775dec 100644 --- a/public/course/mod.php +++ b/public/course/mod.php @@ -232,7 +232,14 @@ if ((!empty($movetosection) or !empty($moveto)) and confirm_sesskey()) { throw new \moodle_exception('needcopy', '', "view.php?id=$section->course"); } - moveto_module($cm, $section, $beforecm); + $formatactions = formatactions::cm($course->id); + if (!empty($section)) { + $formatactions->move_end_section($cm->id, $section->id); + } else if (!empty($beforecm)) { + $formatactions->move_before($cm->id, $beforecm->id); + } else { + throw new \moodle_exception('invalidmovetarget'); + } $sectionreturn = $USER->activitycopysectionreturn; unset($USER->activitycopy); diff --git a/public/course/rest.php b/public/course/rest.php index b48be7cc763..65aeb2a5028 100644 --- a/public/course/rest.php +++ b/public/course/rest.php @@ -81,7 +81,13 @@ if ($class === 'section' && $field === 'move') { } else { $beforemod = null; } - - $isvisible = moveto_module($cm, $section, $beforemod); + $action = \core_courseformat\formatactions::cm($course); + if (!$beforemod) { + $action->move_end_section($cm, $section->id); + } else { + $action->move_before($cm->id, $beforemod->id); + } + $modinfo = get_fast_modinfo($course); + $isvisible = $modinfo->get_cm($cm->id)->is_visible(); echo json_encode(array('visible' => (bool) $isvisible)); } diff --git a/public/course/tests/courselib_test.php b/public/course/tests/courselib_test.php index 9858c56c6a1..c3eed43562d 100644 --- a/public/course/tests/courselib_test.php +++ b/public/course/tests/courselib_test.php @@ -31,6 +31,7 @@ use context_system; use context_coursecat; use core\event\section_viewed; use core_completion_external; +use core_courseformat\formatactions; use core_external; use core_tag_index_builder; use core_tag_tag; @@ -1303,7 +1304,8 @@ final class courselib_test extends advanced_testcase { $oldsectionid = $cm->section; // Perform the move - moveto_module($cm, $newsection); + $cmactions = formatactions::cm($course); + $cmactions->move_end_section($cm->id, $newsection->id); $cms = get_fast_modinfo($course)->get_cms(); $cm = reset($cms); @@ -1330,7 +1332,7 @@ final class courselib_test extends advanced_testcase { // Perform a second move as some issues were only seen on the second move $newsection = get_fast_modinfo($course)->get_section_info(2); $oldsectionid = $cm->section; - moveto_module($cm, $newsection); + $cmactions->move_end_section($cm->id, $newsection->id); $cms = get_fast_modinfo($course)->get_cms(); $cm = reset($cms); @@ -1372,8 +1374,9 @@ final class courselib_test extends advanced_testcase { // Try to perform the move. $this->expectExceptionMessageMatches($codingerror); + $cmactions = formatactions::cm($course); try { - moveto_module($qbankcm, $newsection); + $cmactions->move_end_section($qbankcm->id, $newsection->id); } finally { $qbankcms = get_fast_modinfo($course)->get_instances_of('qbank'); $qbankcm = reset($qbankcms);