From 53dd76348c025f9270744602cb9d9aab60d1e2ce Mon Sep 17 00:00:00 2001 From: Cameron Ball Date: Wed, 11 May 2022 15:10:14 +0800 Subject: [PATCH] MDL-74548 backup: Refactor course copies This patch modifies the way copy data is shared in order to mitigate potential race conditions and ensure that the serialised controller stored in the DB is always in a valid state. The restore controller is now considered the "source of truth" for all information about the copy operation. Backup controllers can no longer contain information about course copies. As copy creation is not atomic, it is still possible for copy controllers to become orphaned or exist in an invalid state. To mitigate this the backup cleanup task has been modified to call a new helper method copy_helper::cleanup_orphaned_copy_controllers. Summary of changes in this patch: - Copy data must now be passed through the restore controller's constructor - base_controller::get_copy has been deprecated in favour of restore_controller::get_copy - base_controller::set_copy has been deprecated without replacement - core_backup\copy\copy has been deprecated, use copy_helper.class.php's copy_helper instead - backup_cleanup_task will now clean up orphaned controllers from copy operations that went awry Thanks to Peter Burnett for assiting with testing this patch. --- .../controller/restore_controller.class.php | 29 +- backup/controller/tests/controller_test.php | 16 +- backup/copy.php | 5 +- backup/externallib.php | 4 +- backup/tests/course_copy_test.php | 57 ++-- backup/tests/externallib_test.php | 4 +- backup/util/helper/copy_helper.class.php | 310 ++++++++++++++++++ backup/util/includes/backup_includes.php | 2 +- backup/util/includes/restore_includes.php | 1 + backup/util/ui/classes/copy/copy.php | 71 +--- backup/util/ui/classes/output/copy_form.php | 2 +- backup/util/ui/renderer.php | 4 +- lang/en/backup.php | 2 +- lib/classes/task/asynchronous_copy_task.php | 5 +- lib/classes/task/backup_cleanup_task.php | 6 +- 15 files changed, 398 insertions(+), 120 deletions(-) create mode 100644 backup/util/helper/copy_helper.class.php diff --git a/backup/controller/restore_controller.class.php b/backup/controller/restore_controller.class.php index 1c79c860f0c..0e60d08d53a 100644 --- a/backup/controller/restore_controller.class.php +++ b/backup/controller/restore_controller.class.php @@ -65,6 +65,13 @@ class restore_controller extends base_controller { /** @var int Number of restore_controllers that are currently executing */ protected static $executing = 0; + /** + * Holds the relevant destination information for course copy operations. + * + * @var \stdClass. + */ + protected $copy; + /** * Constructor. * @@ -79,10 +86,17 @@ class restore_controller extends base_controller { * @param int $userid * @param int $target backup::TARGET_[ NEW_COURSE | CURRENT_ADDING | CURRENT_DELETING | EXISTING_ADDING | EXISTING_DELETING ] * @param \core\progress\base $progress Optional progress monitor + * @param \stdClass $copydata Course copy data, required when in MODE_COPY * @param bool $releasesession Should release the session? backup::RELEASESESSION_YES or backup::RELEASESESSION_NO */ public function __construct($tempdir, $courseid, $interactive, $mode, $userid, $target, - \core\progress\base $progress = null, $releasesession = backup::RELEASESESSION_NO) { + \core\progress\base $progress = null, $releasesession = backup::RELEASESESSION_NO, ?\stdClass $copydata = null) { + + if ($mode == backup::MODE_COPY && is_null($copydata)) { + throw new restore_controller_exception('cannot_instantiate_missing_copydata'); + } + + $this->copy = $copydata; $this->tempdir = $tempdir; $this->courseid = $courseid; $this->interactive = $interactive; @@ -563,6 +577,19 @@ class restore_controller extends base_controller { $this->progress->end_progress(); } + /** + * Get the course copy data. + * + * @return \stdClass + */ + public function get_copy(): \stdClass { + if ($this->mode != backup::MODE_COPY) { + throw new restore_controller_exception('cannot_get_copy_wrong_mode'); + } + + return $this->copy; + } + // Protected API starts here protected function calculate_restoreid() { diff --git a/backup/controller/tests/controller_test.php b/backup/controller/tests/controller_test.php index cf289501a15..f1271aca32a 100644 --- a/backup/controller/tests/controller_test.php +++ b/backup/controller/tests/controller_test.php @@ -72,17 +72,15 @@ class controller_test extends \advanced_testcase { } /** - * Test set copy method. + * Test instantiating a restore controller for a course copy without providing copy data. + * + * @covers \restore_controller::__construct */ - public function test_base_controller_set_copy() { - $this->expectException(\backup_controller_exception::class); - $copy = new \stdClass(); + public function test_restore_controller_copy_without_copydata() { + $this->expectException(\restore_controller_exception::class); - // Set up controller as a non-copy operation. - $bc = new \backup_controller(backup::TYPE_1COURSE, $this->courseid, backup::FORMAT_MOODLE, - backup::INTERACTIVE_NO, backup::MODE_GENERAL, $this->userid, backup::RELEASESESSION_YES); - - $bc->set_copy($copy); + new \restore_controller(1729, $this->courseid, backup::INTERACTIVE_NO, backup::MODE_COPY, + $this->userid, backup::TARGET_NEW_COURSE); } /* diff --git a/backup/copy.php b/backup/copy.php index a095229d7ae..8c7465dafed 100644 --- a/backup/copy.php +++ b/backup/copy.php @@ -24,6 +24,7 @@ */ require_once('../config.php'); +require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); defined('MOODLE_INTERNAL') || die(); @@ -71,8 +72,8 @@ if ($mform->is_cancelled()) { } else if ($mdata = $mform->get_data()) { // Process the form and create the copy task. - $backupcopy = new \core_backup\copy\copy($mdata); - $backupcopy->create_copy(); + $copydata = \copy_helper::process_formdata($mdata); + \copy_helper::create_copy($copydata); if (!empty($mdata->submitdisplay)) { // Redirect to the copy progress overview. diff --git a/backup/externallib.php b/backup/externallib.php index e05b9db3d98..7f902cffca2 100644 --- a/backup/externallib.php +++ b/backup/externallib.php @@ -388,8 +388,8 @@ class core_backup_external extends external_api { if ($mdata) { // Create the copy task. - $backupcopy = new \core_backup\copy\copy($mdata); - $copyids = $backupcopy->create_copy(); + $copydata = \copy_helper::process_formdata($mdata); + $copyids = \copy_helper::create_copy($copydata); } else { throw new moodle_exception('copyformfail', 'backup'); } diff --git a/backup/tests/course_copy_test.php b/backup/tests/course_copy_test.php index 59e8efa2fa4..3280f8e2b9f 100644 --- a/backup/tests/course_copy_test.php +++ b/backup/tests/course_copy_test.php @@ -163,15 +163,14 @@ class course_copy_test extends \advanced_testcase { $formdata->role_3 = 3; $formdata->role_5 = 5; - $coursecopy = new \core_backup\copy\copy($formdata); - $result = $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + $result = \copy_helper::create_copy($copydata); // Load the controllers, to extract the data we need. $bc = \backup_controller::load_controller($result['backupid']); $rc = \restore_controller::load_controller($result['restoreid']); // Check the backup controller. - $this->assertEquals($result, $bc->get_copy()->copyids); $this->assertEquals(backup::MODE_COPY, $bc->get_mode()); $this->assertEquals($this->course->id, $bc->get_courseid()); $this->assertEquals(backup::TYPE_1COURSE, $bc->get_type()); @@ -180,7 +179,6 @@ class course_copy_test extends \advanced_testcase { $newcourseid = $rc->get_courseid(); $newcourse = get_course($newcourseid); - $this->assertEquals($result, $rc->get_copy()->copyids); $this->assertEquals(get_string('copyingcourse', 'backup'), $newcourse->fullname); $this->assertEquals(get_string('copyingcourseshortname', 'backup'), $newcourse->shortname); $this->assertEquals(backup::MODE_COPY, $rc->get_mode()); @@ -222,11 +220,11 @@ class course_copy_test extends \advanced_testcase { $formdata2->shortname = 'tree'; // Create some copies. - $coursecopy = new \core_backup\copy\copy($formdata); - $result = $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + $result = \copy_helper::create_copy($copydata); // Backup, awaiting. - $copies = \core_backup\copy\copy::get_copies($USER->id); + $copies = \copy_helper::get_copies($USER->id); $this->assertEquals($result['backupid'], $copies[0]->backupid); $this->assertEquals($result['restoreid'], $copies[0]->restoreid); $this->assertEquals(\backup::STATUS_AWAITING, $copies[0]->status); @@ -236,7 +234,7 @@ class course_copy_test extends \advanced_testcase { // Backup, in progress. $bc->set_status(\backup::STATUS_EXECUTING); - $copies = \core_backup\copy\copy::get_copies($USER->id); + $copies = \copy_helper::get_copies($USER->id); $this->assertEquals($result['backupid'], $copies[0]->backupid); $this->assertEquals($result['restoreid'], $copies[0]->restoreid); $this->assertEquals(\backup::STATUS_EXECUTING, $copies[0]->status); @@ -244,19 +242,19 @@ class course_copy_test extends \advanced_testcase { // Restore, ready to process. $bc->set_status(\backup::STATUS_FINISHED_OK); - $copies = \core_backup\copy\copy::get_copies($USER->id); - $this->assertEquals($result['backupid'], $copies[0]->backupid); + $copies = \copy_helper::get_copies($USER->id); + $this->assertEquals(null, $copies[0]->backupid); $this->assertEquals($result['restoreid'], $copies[0]->restoreid); $this->assertEquals(\backup::STATUS_REQUIRE_CONV, $copies[0]->status); $this->assertEquals(\backup::OPERATION_RESTORE, $copies[0]->operation); // No records. $bc->set_status(\backup::STATUS_FINISHED_ERR); - $copies = \core_backup\copy\copy::get_copies($USER->id); + $copies = \copy_helper::get_copies($USER->id); $this->assertEmpty($copies); - $coursecopy2 = new \core_backup\copy\copy($formdata2); - $result2 = $coursecopy2->create_copy(); + $copydata2 = \copy_helper::process_formdata($formdata2); + $result2 = \copy_helper::create_copy($copydata2); // Set the second copy to be complete. $bc = \backup_controller::load_controller($result2['backupid']); $bc->set_status(\backup::STATUS_FINISHED_OK); @@ -265,7 +263,7 @@ class course_copy_test extends \advanced_testcase { $rc->set_status(\backup::STATUS_FINISHED_OK); // No records. - $copies = \core_backup\copy\copy::get_copies($USER->id); + $copies = \copy_helper::get_copies($USER->id); $this->assertEmpty($copies); } @@ -291,11 +289,11 @@ class course_copy_test extends \advanced_testcase { $formdata->role_5 = 5; // Create some copies. - $coursecopy = new \core_backup\copy\copy($formdata); - $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + \copy_helper::create_copy($copydata); // No copies match this course id. - $copies = \core_backup\copy\copy::get_copies($USER->id, ($this->course->id + 1)); + $copies = \copy_helper::get_copies($USER->id, ($this->course->id + 1)); $this->assertEmpty($copies); } @@ -321,13 +319,13 @@ class course_copy_test extends \advanced_testcase { $formdata->role_5 = 5; // Create some copies. - $coursecopy = new \core_backup\copy\copy($formdata); - $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + \copy_helper::create_copy($copydata); delete_course($this->course->id, false); // No copies match this course id as it has been deleted. - $copies = \core_backup\copy\copy::get_copies($USER->id, ($this->course->id)); + $copies = \copy_helper::get_copies($USER->id, ($this->course->id)); $this->assertEmpty($copies); } @@ -353,8 +351,8 @@ class course_copy_test extends \advanced_testcase { $formdata->role_5 = 5; // Create the course copy records and associated ad-hoc task. - $coursecopy = new \core_backup\copy\copy($formdata); - $copyids = $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + $copyids = \copy_helper::create_copy($copydata); $courseid = $this->course->id; @@ -430,8 +428,8 @@ class course_copy_test extends \advanced_testcase { $formdata->role_5 = 0; // Create the course copy records and associated ad-hoc task. - $coursecopy = new \core_backup\copy\copy($formdata); - $copyids = $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + $copyids = \copy_helper::create_copy($copydata); $courseid = $this->course->id; @@ -499,8 +497,8 @@ class course_copy_test extends \advanced_testcase { $formdata->role_5 = 5; // Create the course copy records and associated ad-hoc task. - $coursecopy = new \core_backup\copy\copy($formdata); - $copyids = $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + $copyids = \copy_helper::create_copy($copydata); $courseid = $this->course->id; @@ -568,8 +566,8 @@ class course_copy_test extends \advanced_testcase { $formdata->role_5 = 5; // Create the course copy records and associated ad-hoc task. - $coursecopy = new \core_backup\copy\copy($formdata); - $copyids = $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + $copyids = \copy_helper::create_copy($copydata); $courseid = $this->course->id; @@ -627,6 +625,7 @@ class course_copy_test extends \advanced_testcase { // Expect and exception as form data is incomplete. $this->expectException(\moodle_exception::class); - new \core_backup\copy\copy($formdata); + $copydata = \copy_helper::process_formdata($formdata); + \copy_helper::create_copy($copydata); } } diff --git a/backup/tests/externallib_test.php b/backup/tests/externallib_test.php index d3e9d342650..ee39842a302 100644 --- a/backup/tests/externallib_test.php +++ b/backup/tests/externallib_test.php @@ -82,8 +82,8 @@ class externallib_test extends externallib_advanced_testcase { $formdata->role_3 = 3; $formdata->role_5 = 5; - $coursecopy = new \core_backup\copy\copy($formdata); - $copydetails = $coursecopy->create_copy(); + $copydata = \copy_helper::process_formdata($formdata); + $copydetails = \copy_helper::create_copy($copydata); $copydetails['operation'] = \backup::OPERATION_BACKUP; $params = array('copies' => $copydetails); diff --git a/backup/util/helper/copy_helper.class.php b/backup/util/helper/copy_helper.class.php new file mode 100644 index 00000000000..0bbd042424f --- /dev/null +++ b/backup/util/helper/copy_helper.class.php @@ -0,0 +1,310 @@ +. + +defined('MOODLE_INTERNAL') || die(); +require_once($CFG->dirroot . '/backup/util/includes/restore_includes.php'); + +/** + * Copy helper class. + * + * @package core_backup + * @copyright 2022 Catalyst IT Australia Pty Ltd + * @author Cameron Ball + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +final class copy_helper { + + /** + * Process raw form data from copy_form. + * + * @param \stdClass $formdata Raw formdata + * @return \stdClass Processed data for use with create_copy + */ + public static function process_formdata(\stdClass $formdata): \stdClass { + $requiredfields = [ + 'courseid', // Course id integer. + 'fullname', // Fullname of the destination course. + 'shortname', // Shortname of the destination course. + 'category', // Category integer ID that contains the destination course. + 'visible', // Integer to detrmine of the copied course will be visible. + 'startdate', // Integer timestamp of the start of the destination course. + 'enddate', // Integer timestamp of the end of the destination course. + 'idnumber', // ID of the destination course. + 'userdata', // Integer to determine if the copied course will contain user data. + ]; + + $missingfields = array_diff($requiredfields, array_keys((array)$formdata)); + if ($missingfields) { + throw new \moodle_exception('copyfieldnotfound', 'backup', '', null, implode(", ", $missingfields)); + } + + // Remove any extra stuff in the form data. + $processed = (object)array_intersect_key((array)$formdata, array_flip($requiredfields)); + $processed->keptroles = []; + + // Extract roles from the form data and add to keptroles. + foreach ($formdata as $key => $value) { + if ((substr($key, 0, 5) === 'role_') && ($value != 0)) { + $processed->keptroles[] = $value; + } + } + + return $processed; + } + + /** + * Creates a course copy. + * Sets up relevant controllers and adhoc task. + * + * @param \stdClass $copydata Course copy data from process_formdata + * @return array $copyids The backup and restore controller ids + */ + public static function create_copy(\stdClass $copydata): array { + global $USER; + $copyids = []; + + // Create the initial backupcontoller. + $bc = new \backup_controller(\backup::TYPE_1COURSE, $copydata->courseid, \backup::FORMAT_MOODLE, + \backup::INTERACTIVE_NO, \backup::MODE_COPY, $USER->id, \backup::RELEASESESSION_YES); + $copyids['backupid'] = $bc->get_backupid(); + + // Create the initial restore contoller. + list($fullname, $shortname) = \restore_dbops::calculate_course_names( + 0, get_string('copyingcourse', 'backup'), get_string('copyingcourseshortname', 'backup')); + $newcourseid = \restore_dbops::create_new_course($fullname, $shortname, $copydata->category); + $rc = new \restore_controller($copyids['backupid'], $newcourseid, \backup::INTERACTIVE_NO, + \backup::MODE_COPY, $USER->id, \backup::TARGET_NEW_COURSE, null, + \backup::RELEASESESSION_NO, $copydata); + $copyids['restoreid'] = $rc->get_restoreid(); + + $bc->set_status(\backup::STATUS_AWAITING); + $bc->get_status(); + $rc->save_controller(); + + // Create the ad-hoc task to perform the course copy. + $asynctask = new \core\task\asynchronous_copy_task(); + $asynctask->set_blocking(false); + $asynctask->set_custom_data($copyids); + \core\task\manager::queue_adhoc_task($asynctask); + + // Clean up the controller. + $bc->destroy(); + + return $copyids; + } + + /** + * Get the in progress course copy operations for a user. + * + * @param int $userid User id to get the course copies for. + * @param int|null $courseid The optional source course id to get copies for. + * @return array $copies Details of the inprogress copies. + */ + public static function get_copies(int $userid, ?int $courseid = null): array { + global $DB; + $copies = []; + [$insql, $inparams] = $DB->get_in_or_equal([\backup::STATUS_FINISHED_OK, \backup::STATUS_FINISHED_ERR]); + $params = [ + $userid, + \backup::EXECUTION_DELAYED, + \backup::MODE_COPY, + \backup::OPERATION_BACKUP, + \backup::STATUS_FINISHED_OK, + \backup::OPERATION_RESTORE + ]; + + // We exclude backups that finished with OK. Therefore if a backup is missing, + // we can assume it finished properly. + // + // We exclude both failed and successful restores because both of those indicate that the whole + // operation has completed. + $sql = 'SELECT backupid, itemid, operation, status, timecreated, purpose + FROM {backup_controllers} + WHERE userid = ? + AND execution = ? + AND purpose = ? + AND ((operation = ? AND status <> ?) OR (operation = ? AND status NOT ' . $insql .')) + ORDER BY timecreated DESC'; + + $copyrecords = $DB->get_records_sql($sql, array_merge($params, $inparams)); + $idtorc = self::map_backupids_to_restore_controller($copyrecords); + + // Our SQL only gets controllers that have not finished successfully. + // So, no restores => all restores have finished (either failed or OK) => all backups have too + // Therefore there are no in progress copy operations, return early. + if (empty($idtorc)) { + return []; + } + + foreach ($copyrecords as $copyrecord) { + try { + $isbackup = $copyrecord->operation == \backup::OPERATION_BACKUP; + + // The mapping is guaranteed to exist for restore controllers, but not + // backup controllers. + // + // When processing backups we don't actually need it, so we just coalesce + // to null. + $rc = $idtorc[$copyrecord->backupid] ?? null; + + $cid = $isbackup ? $copyrecord->itemid : $rc->get_copy()->courseid; + $course = get_course($cid); + $copy = clone ($copyrecord); + $copy->backupid = $isbackup ? $copyrecord->backupid : null; + $copy->restoreid = $rc ? $rc->get_restoreid() : null; + $copy->destination = $rc ? $rc->get_copy()->shortname : null; + $copy->source = $course->shortname; + $copy->sourceid = $course->id; + } catch (\Exception $e) { + continue; + } + + // Filter out anything that's not relevant. + if ($courseid) { + if ($isbackup && $copyrecord->itemid != $courseid) { + continue; + } + + if (!$isbackup && $rc->get_copy()->courseid != $courseid) { + continue; + } + } + + // A backup here means that the associated restore controller has not started. + // + // There's a few situations to consider: + // + // 1. The backup is waiting or in progress + // 2. The backup failed somehow + // 3. Something went wrong (e.g., solar flare) and the backup controller saved, but the restore controller didn't + // 4. The restore hasn't been created yet (race condition) + // + // In the case of 1, we add it to the return list. In the case of 2, 3 and 4 we just ignore it and move on. + // The backup cleanup task will take care of updating/deleting invalid controllers. + if ($isbackup) { + if ($copyrecord->status != \backup::STATUS_FINISHED_ERR && !is_null($rc)) { + $copies[] = $copy; + } + + continue; + } + + // A backup in copyrecords, indicates that the associated backup has not + // successfully finished. We shouldn't do anything with this restore record. + if ($copyrecords[$rc->get_tempdir()] ?? null) { + continue; + } + + // This is a restore record, and the backup has finished. Return it. + $copies[] = $copy; + } + + return $copies; + } + + /** + * Returns a mapping between copy controller IDs and the restore controller. + * For example if there exists a copy with backup ID abc and restore ID 123 + * then this mapping will map both keys abc and 123 to the same (instantiated) + * restore controller. + * + * @param array $backuprecords An array of records from {backup_controllers} + * @return array An array of mappings between backup ids and restore controllers + */ + private static function map_backupids_to_restore_controller(array $backuprecords): array { + // Needed for PHP 7.3 - array_merge only accepts 0 parameters in PHP >= 7.4. + if (empty($backuprecords)) { + return []; + } + + return array_merge( + ...array_map( + function (\stdClass $backuprecord): array { + $iscopyrestore = $backuprecord->operation == \backup::OPERATION_RESTORE && + $backuprecord->purpose == \backup::MODE_COPY; + $isfinished = $backuprecord->status == \backup::STATUS_FINISHED_OK; + + if (!$iscopyrestore || $isfinished) { + return []; + } + + $rc = \restore_controller::load_controller($backuprecord->backupid); + return [$backuprecord->backupid => $rc, $rc->get_tempdir() => $rc]; + }, + array_values($backuprecords) + ) + ); + } + + /** + * Detects and deletes/fails controllers associated with a course copy that are + * in an invalid state. + * + * @param array $backuprecords An array of records from {backup_controllers} + * @param int $age How old a controller needs to be (in seconds) before its considered for cleaning + * @return void + */ + public static function cleanup_orphaned_copy_controllers(array $backuprecords, int $age = MINSECS): void { + global $DB; + + $idtorc = self::map_backupids_to_restore_controller($backuprecords); + + // Helpful to test if a backup exists in $backuprecords. + $bidstorecord = array_combine( + array_column($backuprecords, 'backupid'), + $backuprecords + ); + + foreach ($backuprecords as $record) { + if ($record->purpose != \backup::MODE_COPY || $record->status == \backup::STATUS_FINISHED_OK) { + continue; + } + + $isbackup = $record->operation == \backup::OPERATION_BACKUP; + $restoreexists = isset($idtorc[$record->backupid]); + $nsecondsago = time() - $age; + + if ($isbackup) { + // Sometimes the backup controller gets created, ""something happens"" (like a solar flare) + // and the restore controller (and hence adhoc task) don't. + // + // If more than one minute has passed and the restore controller doesn't exist, it's likely that + // this backup controller is orphaned, so we should remove it as the adhoc task to process it will + // never be created. + if (!$restoreexists && $record->timecreated <= $nsecondsago) { + // It would be better to mark the backup as failed by loading the controller + // and marking it as failed with $bc->set_status(), but we can't: MDL-74711. + // + // Deleting it isn't ideal either as maybe we want to inspect the backup + // for debugging. So manually updating the column seems to be the next best. + $record->status = \backup::STATUS_FINISHED_ERR; + $DB->update_record('backup_controllers', $record); + } + continue; + } + + if ($rc = $idtorc[$record->backupid] ?? null) { + $backuprecord = $bidstorecord[$rc->get_tempdir()] ?? null; + + // Check the status of the associated backup. If it's failed, then mark this + // restore as failed too. + if ($backuprecord && $backuprecord->status == \backup::STATUS_FINISHED_ERR) { + $rc->set_status(\backup::STATUS_FINISHED_ERR); + } + } + } + } +} diff --git a/backup/util/includes/backup_includes.php b/backup/util/includes/backup_includes.php index 9df19647e16..7a1e4288cf6 100644 --- a/backup/util/includes/backup_includes.php +++ b/backup/util/includes/backup_includes.php @@ -61,6 +61,7 @@ require_once($CFG->dirroot . '/backup/util/helper/backup_null_iterator.class.php require_once($CFG->dirroot . '/backup/util/helper/backup_array_iterator.class.php'); require_once($CFG->dirroot . '/backup/util/helper/backup_anonymizer_helper.class.php'); require_once($CFG->dirroot . '/backup/util/helper/backup_file_manager.class.php'); +require_once($CFG->dirroot . '/backup/util/helper/copy_helper.class.php'); require_once($CFG->dirroot . '/backup/util/helper/restore_moodlexml_parser_processor.class.php'); // Required by backup_general_helper::get_backup_information(). require_once($CFG->dirroot . '/backup/util/xml/xml_writer.class.php'); require_once($CFG->dirroot . '/backup/util/xml/output/xml_output.class.php'); @@ -97,7 +98,6 @@ require_once($CFG->dirroot . '/backup/util/ui/backup_moodleform.class.php'); require_once($CFG->dirroot . '/backup/util/ui/backup_ui.class.php'); require_once($CFG->dirroot . '/backup/util/ui/backup_ui_stage.class.php'); require_once($CFG->dirroot . '/backup/util/ui/backup_ui_setting.class.php'); -require_once($CFG->dirroot . '/backup/util/ui/classes/copy/copy.php'); // And some moodle stuff too require_once($CFG->dirroot.'/course/lib.php'); diff --git a/backup/util/includes/restore_includes.php b/backup/util/includes/restore_includes.php index 2a8e020b638..9b8ab15e84d 100644 --- a/backup/util/includes/restore_includes.php +++ b/backup/util/includes/restore_includes.php @@ -37,6 +37,7 @@ require_once($CFG->dirroot . '/backup/util/structure/restore_path_element.class. require_once($CFG->dirroot . '/backup/util/helper/async_helper.class.php'); require_once($CFG->dirroot . '/backup/util/helper/backup_anonymizer_helper.class.php'); require_once($CFG->dirroot . '/backup/util/helper/backup_file_manager.class.php'); +require_once($CFG->dirroot . '/backup/util/helper/copy_helper.class.php'); require_once($CFG->dirroot . '/backup/util/helper/restore_prechecks_helper.class.php'); require_once($CFG->dirroot . '/backup/util/helper/restore_moodlexml_parser_processor.class.php'); require_once($CFG->dirroot . '/backup/util/helper/restore_inforef_parser_processor.class.php'); diff --git a/backup/util/ui/classes/copy/copy.php b/backup/util/ui/classes/copy/copy.php index fbf26958213..1f8ced3a3f4 100644 --- a/backup/util/ui/classes/copy/copy.php +++ b/backup/util/ui/classes/copy/copy.php @@ -135,74 +135,11 @@ class copy { * @return array $copyids THe backup and restore controller ids. */ public function create_copy(): array { - global $USER; - $copyids = array(); - - // Create the initial backupcontoller. - $bc = new \backup_controller(\backup::TYPE_1COURSE, $this->copydata->courseid, \backup::FORMAT_MOODLE, - \backup::INTERACTIVE_NO, \backup::MODE_COPY, $USER->id, \backup::RELEASESESSION_YES); - $copyids['backupid'] = $bc->get_backupid(); - - // Create the initial restore contoller. - list($fullname, $shortname) = \restore_dbops::calculate_course_names( - 0, get_string('copyingcourse', 'backup'), get_string('copyingcourseshortname', 'backup')); - $newcourseid = \restore_dbops::create_new_course($fullname, $shortname, $this->copydata->category); - $rc = new \restore_controller($copyids['backupid'], $newcourseid, - \backup::INTERACTIVE_NO, \backup::MODE_COPY, $USER->id, - \backup::TARGET_NEW_COURSE); - $copyids['restoreid'] = $rc->get_restoreid(); - - // Configure the controllers based on the submitted data. - $copydata = $this->copydata; - $copydata->copyids = $copyids; + debugging('The method \core_backup\copy\copy::create_copy() is deprecated. + Please use the methods provided by copy_helper instead.', DEBUG_DEVELOPER); + $copydata = clone($this->copydata); $copydata->keptroles = $this->roles; - $bc->set_copy($copydata); - $bc->set_status(\backup::STATUS_AWAITING); - $bc->get_status(); - - $rc->set_copy($copydata); - $rc->save_controller(); - - // Create the ad-hoc task to perform the course copy. - $asynctask = new \core\task\asynchronous_copy_task(); - $asynctask->set_blocking(false); - $asynctask->set_custom_data($copyids); - \core\task\manager::queue_adhoc_task($asynctask); - - // Clean up the controller. - $bc->destroy(); - - return $copyids; - } - - /** - * Filters an array of copy records by course ID. - * - * @param array $copyrecords - * @param int $courseid - * @return array $copies Filtered array of records. - */ - static private function filter_copies_course(array $copyrecords, int $courseid): array { - $copies = array(); - - foreach ($copyrecords as $copyrecord) { - if ($copyrecord->operation == \backup::OPERATION_RESTORE) { // Restore records. - if ($copyrecord->status == \backup::STATUS_FINISHED_OK - || $copyrecord->status == \backup::STATUS_FINISHED_ERR) { - continue; - } else { - $rc = \restore_controller::load_controller($copyrecord->restoreid); - if ($rc->get_copy()->courseid == $courseid) { - $copies[] = $copyrecord; - } - } - } else { // Backup records. - if ($copyrecord->itemid == $courseid) { - $copies[] = $copyrecord; - } - } - } - return $copies; + return \copy_helper::create_copy($copydata); } /** diff --git a/backup/util/ui/classes/output/copy_form.php b/backup/util/ui/classes/output/copy_form.php index 78d56c796c7..dfcafb03b8c 100644 --- a/backup/util/ui/classes/output/copy_form.php +++ b/backup/util/ui/classes/output/copy_form.php @@ -72,7 +72,7 @@ class copy_form extends \moodleform { $mform->setConstant('returnto', $returnto); // Notifications of current copies. - $copies = \core_backup\copy\copy::get_copies($USER->id, $course->id); + $copies = \copy_helper::get_copies($USER->id, $course->id); if (!empty($copies)) { $progresslink = new \moodle_url('/backup/copyprogress.php?', array('id' => $course->id)); $notificationmsg = get_string('copiesinprogress', 'backup', $progresslink->out()); diff --git a/backup/util/ui/renderer.php b/backup/util/ui/renderer.php index 7a1774d5772..5f83b6ab530 100644 --- a/backup/util/ui/renderer.php +++ b/backup/util/ui/renderer.php @@ -1022,7 +1022,7 @@ class core_backup_renderer extends plugin_renderer_base { $tabledata = array(); // Get all in progress course copies for this user. - $copies = \core_backup\copy\copy::get_copies($userid, $courseid); + $copies = \copy_helper::get_copies($userid, $courseid); foreach ($copies as $copy) { $sourceurl = new \moodle_url('/course/view.php', array('id' => $copy->sourceid)); @@ -1030,7 +1030,7 @@ class core_backup_renderer extends plugin_renderer_base { $tablerow = array( html_writer::link($sourceurl, $copy->source), $copy->destination, - userdate($copy->time), + userdate($copy->timecreated), get_string($copy->operation), $this->get_status_display($copy->status, $copy->backupid, $copy->restoreid, $copy->operation) ); diff --git a/lang/en/backup.php b/lang/en/backup.php index 190fafec630..cd471701c1f 100644 --- a/lang/en/backup.php +++ b/lang/en/backup.php @@ -172,7 +172,7 @@ $string['copycoursetitle'] = 'Copy course: {$a}'; $string['copydest'] = 'Destination'; $string['copyingcourse'] = 'Course copying in progress'; $string['copyingcourseshortname'] = 'copying'; -$string['copyfieldnotfound'] = 'A required field was not found'; +$string['copyfieldnotfound'] = 'Required field data was not found for field(s): {$a}'; $string['copyformfail'] = 'AJAX submission of course copy form has failed.'; $string['copyop'] = 'Current operation'; $string['copyprogressheading'] = 'Course copies in progress'; diff --git a/lib/classes/task/asynchronous_copy_task.php b/lib/classes/task/asynchronous_copy_task.php index 0f7bb41806b..0d2b8e45b03 100644 --- a/lib/classes/task/asynchronous_copy_task.php +++ b/lib/classes/task/asynchronous_copy_task.php @@ -65,8 +65,10 @@ class asynchronous_copy_task extends adhoc_task { delete_course($restorerecord->itemid, false); // Clean up partially created destination course. return; // Return early as we can't continue. } + + $rc = \restore_controller::load_controller($restoreid); // Get the restore controller by restore id. $bc->set_progress(new \core\progress\db_updater($backuprecord->id, 'backup_controllers', 'progress')); - $copyinfo = $bc->get_copy(); + $copyinfo = $rc->get_copy(); $backupplan = $bc->get_plan(); $keepuserdata = (bool)$copyinfo->userdata; @@ -110,7 +112,6 @@ class asynchronous_copy_task extends adhoc_task { $file = $results['backup_destination']; $file->extract_to_pathname(get_file_packer('application/vnd.moodle.backup'), $backupbasepath); // Start the restore process. - $rc = \restore_controller::load_controller($restoreid); // Get the restore controller by restore id. $rc->set_progress(new \core\progress\db_updater($restorerecord->id, 'backup_controllers', 'progress')); $rc->prepare_copy(); diff --git a/lib/classes/task/backup_cleanup_task.php b/lib/classes/task/backup_cleanup_task.php index dddbc4ecac7..5c018bef3b6 100644 --- a/lib/classes/task/backup_cleanup_task.php +++ b/lib/classes/task/backup_cleanup_task.php @@ -48,8 +48,12 @@ class backup_cleanup_task extends scheduled_task { public function execute() { global $DB; - $loglifetime = get_config('backup', 'loglifetime'); + $sql = 'SELECT * FROM {backup_controllers} WHERE purpose = ? AND status <> ?'; + $params = [\backup::MODE_COPY, \backup::STATUS_FINISHED_OK]; + $copyrecords = $DB->get_records_sql($sql, $params); + \copy_helper::cleanup_orphaned_copy_controllers($copyrecords); + $loglifetime = get_config('backup', 'loglifetime'); if (empty($loglifetime)) { mtrace('The \'loglifetime\' config is not set. Can\'t proceed and delete old backup records.'); return;