diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php index 55e35cfe578..ea01b013591 100644 --- a/backup/moodle2/backup_stepslib.php +++ b/backup/moodle2/backup_stepslib.php @@ -44,11 +44,9 @@ class create_and_clean_temp_stuff extends backup_execution_step { } /** - * Delete the temp dir used by backup/restore (conditionally), - * delete old directories and drop temp ids table. Note we delete - * the directory but not the corresponding log file that will be - * there for, at least, 1 week - only delete_old_backup_dirs() or cron - * deletes log files (for easier access to them). + * Delete the temp dir used by backup/restore (conditionally) and drop temp ids table. + * Note we delete the directory but not the corresponding log file that will be + * there until cron cleans it up. */ class drop_and_clean_temp_stuff extends backup_execution_step { @@ -58,7 +56,6 @@ class drop_and_clean_temp_stuff extends backup_execution_step { global $CFG; backup_controller_dbops::drop_backup_ids_temp_table($this->get_backupid()); // Drop ids temp table - backup_helper::delete_old_backup_dirs(strtotime('-1 week')); // Delete > 1 week old temp dirs. // Delete temp dir conditionally: // 1) If $CFG->keeptempdirectoriesonbackup is not enabled // 2) If backup temp dir deletion has been marked to be avoided diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 140dacde06c..237d993e915 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -57,21 +57,19 @@ class restore_create_and_clean_temp_stuff extends restore_execution_step { } /** - * delete the temp dir used by backup/restore (conditionally), - * delete old directories and drop temp ids table + * Drop temp ids table and delete the temp dir used by backup/restore (conditionally). */ class restore_drop_and_clean_temp_stuff extends restore_execution_step { protected function define_execution() { global $CFG; restore_controller_dbops::drop_restore_temp_tables($this->get_restoreid()); // Drop ids temp table - $progress = $this->task->get_progress(); - $progress->start_progress('Deleting backup dir'); - backup_helper::delete_old_backup_dirs(strtotime('-1 week'), $progress); // Delete > 1 week old temp dirs. if (empty($CFG->keeptempdirectoriesonbackup)) { // Conditionally + $progress = $this->task->get_progress(); + $progress->start_progress('Deleting backup dir'); backup_helper::delete_backup_dir($this->task->get_tempdir(), $progress); // Empty restore dir + $progress->end_progress(); } - $progress->end_progress(); } } diff --git a/backup/tests/backup_cleanup_task_test.php b/backup/tests/backup_cleanup_task_test.php new file mode 100644 index 00000000000..e4b71c8c9d5 --- /dev/null +++ b/backup/tests/backup_cleanup_task_test.php @@ -0,0 +1,157 @@ +. + +/** + * Tests for the \core\task\backup_cleanup_task scheduled task. + * + * @package core_backup + * @copyright 2021 Mikhail Golenkov + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); + +/** + * Tests for the \core\task\backup_cleanup_task scheduled task. + * + * @copyright 2021 Mikhail Golenkov + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class core_backup_cleanup_task_testcase extends advanced_testcase { + + /** + * Set up tasks for all tests. + */ + protected function setUp(): void { + $this->resetAfterTest(true); + } + + /** + * Take a backup of the course provided and return backup id. + * + * @param int $courseid Course id to be backed up. + * @return string Backup id. + */ + private function backup_course(int $courseid): string { + // Backup the course. + $user = get_admin(); + $controller = new \backup_controller( + \backup::TYPE_1COURSE, + $courseid, + \backup::FORMAT_MOODLE, + \backup::INTERACTIVE_NO, + \backup::MODE_AUTOMATED, + $user->id + ); + $controller->execute_plan(); + return $controller->get_backupid(); + } + + /** + * Test the task idle run. Nothing should explode. + */ + public function test_backup_cleanup_task_idle() { + $task = new \core\task\backup_cleanup_task(); + $task->execute(); + } + + /** + * Test the task exits when backup | loglifetime setting is not set. + */ + public function test_backup_cleanup_task_exits() { + set_config('loglifetime', 0, 'backup'); + $task = new \core\task\backup_cleanup_task(); + ob_start(); + $task->execute(); + $output = ob_get_contents(); + ob_end_clean(); + $this->assertStringContainsString('config is not set', $output); + } + + /** + * Test the task deletes records from DB. + */ + public function test_backup_cleanup_task_deletes_records() { + global $DB; + + // Create a course. + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + + // Take two backups of the course. + $backupid1 = $this->backup_course($course->id); + $backupid2 = $this->backup_course($course->id); + + // Emulate the first backup to be done 31 days ago. + $bcrecord = $DB->get_record('backup_controllers', ['backupid' => $backupid1]); + $bcrecord->timecreated -= DAYSECS * 31; + $DB->update_record('backup_controllers', $bcrecord); + + // Run the task. + $task = new \core\task\backup_cleanup_task(); + $task->execute(); + + // There should be no records related to the first backup. + $this->assertEquals(0, $DB->count_records('backup_controllers', ['backupid' => $backupid1])); + $this->assertEquals(0, $DB->count_records('backup_logs', ['backupid' => $backupid1])); + + // Records related to the second backup should remain. + $this->assertGreaterThan(0, $DB->count_records('backup_controllers', ['backupid' => $backupid2])); + $this->assertGreaterThanOrEqual(0, $DB->count_records('backup_logs', ['backupid' => $backupid2])); + } + + /** + * Test the task deletes files from file system. + */ + public function test_backup_cleanup_task_deletes_files() { + global $CFG; + + // Create a course. + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + + // Take two backups of the course and get their logs. + $backupid1 = $this->backup_course($course->id); + $backupid2 = $this->backup_course($course->id); + $filepath1 = $CFG->backuptempdir . '/' . $backupid1 . '.log'; + $filepath2 = $CFG->backuptempdir . '/' . $backupid2 . '.log'; + + // Create a subdirectory. + $subdir = $CFG->backuptempdir . '/subdir'; + make_writable_directory($subdir); + + // Both logs and the dir should exist. + $this->assertTrue(file_exists($filepath1)); + $this->assertTrue(file_exists($filepath2)); + $this->assertTrue(file_exists($subdir)); + + // Change modification time of the first log and the sub dir to be 8 days ago. + touch($filepath1, time() - 8 * DAYSECS); + touch($subdir, time() - 8 * DAYSECS); + + // Run the task. + $task = new \core\task\backup_cleanup_task(); + $task->execute(); + + // Files and directories older than a week are supposed to be removed. + $this->assertFalse(file_exists($filepath1)); + $this->assertFalse(file_exists($subdir)); + $this->assertTrue(file_exists($filepath2)); + } +} diff --git a/backup/util/helper/backup_helper.class.php b/backup/util/helper/backup_helper.class.php index 167eae7bbae..2222540bfd6 100644 --- a/backup/util/helper/backup_helper.class.php +++ b/backup/util/helper/backup_helper.class.php @@ -153,25 +153,27 @@ abstract class backup_helper { * If supplied, progress object should be ready to receive indeterminate * progress reports. * - * @param int $deletefrom Time to delete from + * @param int $deletebefore Delete files and directories older than this time * @param \core\progress\base $progress Optional progress reporting object */ - static public function delete_old_backup_dirs($deletefrom, \core\progress\base $progress = null) { + static public function delete_old_backup_dirs($deletebefore, \core\progress\base $progress = null) { $status = true; - // Get files and directories in the backup temp dir without descend. + // Get files and directories in the backup temp dir. $backuptempdir = make_backup_temp_directory(''); - $list = get_directory_list($backuptempdir, '', false, true, true); - foreach ($list as $file) { - $file_path = $backuptempdir . '/' . $file; - $moddate = filemtime($file_path); - if ($status && $moddate < $deletefrom) { - //If directory, recurse - if (is_dir($file_path)) { - // $file is really the backupid - $status = self::delete_backup_dir($file, $progress); - //If file - } else { - unlink($file_path); + $items = new DirectoryIterator($backuptempdir); + foreach ($items as $item) { + if ($item->isDot()) { + continue; + } + if ($item->getMTime() < $deletebefore) { + if ($item->isDir()) { + // The item is a directory for some backup. + if (!self::delete_backup_dir($item->getFilename(), $progress)) { + // Something went wrong. Finish the list of items and then throw an exception. + $status = false; + } + } else if ($item->isFile()) { + unlink($item->getPathname()); } } } diff --git a/lang/en/admin.php b/lang/en/admin.php index 3ecba847145..e697dee502f 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -1307,7 +1307,7 @@ $string['tasktype'] = 'Type'; $string['taskadmintitle'] = 'Tasks'; $string['taskanalyticscleanup'] = 'Analytics cleanup'; $string['taskautomatedbackup'] = 'Automated backups'; -$string['taskbackupcleanup'] = 'Clean backup tables and logs'; +$string['taskbackupcleanup'] = 'Clean backup tables, logs and files'; $string['taskbadgescron'] = 'Award badges'; $string['taskbadgesmessagecron'] = 'Background processing for sending badges notifications'; $string['taskblogcron'] = 'Sync external blogs'; diff --git a/lib/classes/task/backup_cleanup_task.php b/lib/classes/task/backup_cleanup_task.php index 6876b9846a0..dddbc4ecac7 100644 --- a/lib/classes/task/backup_cleanup_task.php +++ b/lib/classes/task/backup_cleanup_task.php @@ -23,6 +23,10 @@ */ namespace core\task; +defined('MOODLE_INTERNAL') || die(); + +require_once($CFG->dirroot . '/backup/util/includes/backup_includes.php'); + /** * Simple task to delete old backup records. */ @@ -47,7 +51,8 @@ class backup_cleanup_task extends scheduled_task { $loglifetime = get_config('backup', 'loglifetime'); if (empty($loglifetime)) { - throw new coding_exception('The \'loglifetime\' config is not set. Can\'t proceed and delete old backup records.'); + mtrace('The \'loglifetime\' config is not set. Can\'t proceed and delete old backup records.'); + return; } // First, get the list of all backupids older than loglifetime. @@ -65,6 +70,9 @@ class backup_cleanup_task extends scheduled_task { $DB->delete_records('backup_controllers', array('backupid' => $record->backupid)); } } + + // Delete files and dirs older than 1 week. + \backup_helper::delete_old_backup_dirs(strtotime('-1 week')); } }