Merge branch 'MDL-50063' of git://github.com/stronk7/moodle

admin/tool/recyclebin/classes/category_bin.php

@@ -126,6 +126,9 @@ class category_bin extends base_bin {
             throw new \moodle_exception('Failed to backup activity prior to deletion.');
         }
 
+        // Have finished with the controller, let's destroy it, freeing mem and resources.
+        $controller->destroy();
+
         // Grab the filename.
         $file = $result['backup_destination'];
         if (!$file->get_contenthash()) {
@@ -259,6 +262,9 @@ class category_bin extends base_bin {
         // Run the import.
         $controller->execute_plan();
 
+        // Have finished with the controller, let's destroy it, freeing mem and resources.
+        $controller->destroy();
+
         // Fire event.
         $event = \tool_recyclebin\event\category_bin_item_restored::create(array(
             'objectid' => $item->id,

admin/tool/recyclebin/classes/course_bin.php

@@ -130,6 +130,9 @@ class course_bin extends base_bin {
             throw new \moodle_exception('Failed to backup activity prior to deletion.');
         }
 
+        // Have finished with the controller, let's destroy it, freeing mem and resources.
+        $controller->destroy();
+
         // Grab the filename.
         $file = $result['backup_destination'];
         if (!$file->get_contenthash()) {
@@ -246,6 +249,9 @@ class course_bin extends base_bin {
         // Run the import.
         $controller->execute_plan();
 
+        // Have finished with the controller, let's destroy it, freeing mem and resources.
+        $controller->destroy();
+
         // Fire event.
         $event = \tool_recyclebin\event\course_bin_item_restored::create(array(
             'objectid' => $item->id,

admin/tool/uploadcourse/classes/course.php

@@ -740,7 +740,6 @@ class tool_uploadcourse_course {
                 $this->error('errorwhilerestoringcourse', new lang_string('errorwhilerestoringthecourse', 'tool_uploadcourse'));
             }
             $rc->destroy();
-            unset($rc); // File logging is a mess, we can only try to rely on gc to close handles.
         }
 
         // Proceed with enrolment data.

admin/tool/uploadcourse/tests/course_test.php

@@ -35,13 +35,6 @@ global $CFG;
  */
 class tool_uploadcourse_course_testcase extends advanced_testcase {
 
-    /**
-     * Tidy up open files that may be left open.
-     */
-    protected function tearDown() {
-        gc_collect_cycles();
-    }
-
     public function test_proceed_without_prepare() {
         $this->resetAfterTest(true);
         $mode = tool_uploadcourse_processor::MODE_CREATE_NEW;

admin/tool/uploadcourse/tests/helper_test.php

@@ -129,7 +129,6 @@ class tool_uploadcourse_helper_testcase extends advanced_testcase {
         $this->assertTrue(isset($result['backup_destination']));
         $c1backupfile = $result['backup_destination']->copy_content_to_temp();
         $bc->destroy();
-        unset($bc); // File logging is a mess, we can only try to rely on gc to close handles.
 
         // Creating backup file.
         $bc = new backup_controller(backup::TYPE_1COURSE, $c2->id, backup::FORMAT_MOODLE,
@@ -139,7 +138,6 @@ class tool_uploadcourse_helper_testcase extends advanced_testcase {
         $this->assertTrue(isset($result['backup_destination']));
         $c2backupfile = $result['backup_destination']->copy_content_to_temp();
         $bc->destroy();
-        unset($bc); // File logging is a mess, we can only try to rely on gc to close handles.
 
         $oldcfg = isset($CFG->keeptempdirectoriesonbackup) ? $CFG->keeptempdirectoriesonbackup : false;
         $CFG->keeptempdirectoriesonbackup = true;

admin/tool/uploadcourse/tests/processor_test.php

@@ -36,13 +36,6 @@ require_once($CFG->libdir . '/csvlib.class.php');
  */
 class tool_uploadcourse_processor_testcase extends advanced_testcase {
 
-    /**
-     * Tidy up open files that may be left open.
-     */
-    protected function tearDown() {
-        gc_collect_cycles();
-    }
-
     public function test_basic() {
         global $DB;
         $this->resetAfterTest(true);

backup/controller/backup_controller.class.php

@@ -158,6 +158,8 @@ class backup_controller extends base_controller {
     public function destroy() {
         // Only need to destroy circulars under the plan. Delegate to it.
         $this->plan->destroy();
+        // Loggers may have also chained references, destroy them. Also closing resources when needed.
+        $this->logger->destroy();
     }
 
     public function finish_ui() {
@@ -184,7 +186,7 @@ class backup_controller extends base_controller {
             $this->save_controller();
             $tbc = self::load_controller($this->backupid);
             $this->logger = $tbc->logger; // wakeup loggers
-            $tbc->destroy(); // Clean temp controller structures
+            $tbc->plan->destroy(); // Clean plan controller structures, keeping logger alive.
 
         } else if ($status == backup::STATUS_FINISHED_OK) {
             // If the operation has ended without error (backup::STATUS_FINISHED_OK)

backup/controller/restore_controller.class.php

@@ -170,6 +170,8 @@ class restore_controller extends base_controller {
     public function destroy() {
         // Only need to destroy circulars under the plan. Delegate to it.
         $this->plan->destroy();
+        // Loggers may have also chained references, destroy them. Also closing resources when needed.
+        $this->logger->destroy();
     }
 
     public function finish_ui() {
@@ -196,7 +198,7 @@ class restore_controller extends base_controller {
             $this->save_controller();
             $tbc = self::load_controller($this->restoreid);
             $this->logger = $tbc->logger; // wakeup loggers
-            $tbc->destroy(); // Clean temp controller structures
+            $tbc->plan->destroy(); // Clean plan controller structures, keeping logger alive.
 
         } else if ($status == backup::STATUS_FINISHED_OK) {
             // If the operation has ended without error (backup::STATUS_FINISHED_OK)

backup/moodle2/tests/moodle2_course_format_test.php

@@ -39,13 +39,6 @@ require_once($CFG->libdir . '/completionlib.php');
  */
 class core_backup_moodle2_course_format_testcase extends advanced_testcase {
 
-    /**
-     * Tidy up open files that may be left open.
-     */
-    protected function tearDown() {
-        gc_collect_cycles();
-    }
-
     /**
      * Tests a backup and restore adds the required section option data
      * when the same course format is used.
@@ -269,4 +262,4 @@ class format_test_cs2_options extends format_test_cs_options {
             ),
         ) + parent::section_format_options($foreditform);
     }
-}
+}

backup/moodle2/tests/moodle2_test.php

@@ -38,13 +38,6 @@ require_once($CFG->libdir . '/completionlib.php');
  */
 class core_backup_moodle2_testcase extends advanced_testcase {
 
-    /**
-     * Tidy up open files that may be left open.
-     */
-    protected function tearDown() {
-        gc_collect_cycles();
-    }
-
     /**
      * Tests the availability field on modules and sections is correctly
      * backed up and restored.
@@ -161,11 +154,6 @@ class core_backup_moodle2_testcase extends advanced_testcase {
                     $thrown->getFile() . ':' . $thrown->getLine(). "]\n\n";
         }
 
-        // Must set restore_controller variable to null so that php
-        // garbage-collects it; otherwise the file will be left open and
-        // attempts to delete it will cause a permission error on Windows
-        // systems, breaking unit tests.
-        $rc = null;
         $this->assertNull($thrown);
 
         // Get information about the resulting course and check that it is set

backup/upgrade.txt

@@ -1,6 +1,14 @@
 This files describes API changes in /backup/*,
 information provided here is intended especially for developers.
 
+=== 3.1 ===
+
+* New close() method added to loggers so they can close any open resource. Previously
+  any backup and restore operation using the file logger may be leaving unclosed files.
+* New destroy() method added to loggers, normally called from backup and restore controllers
+  own destroy() method to ensure that all references in the chained loggers are deleted
+  and any open resource within them is closed properly.
+
 === 3.0 ===
 
 * The backup_auto_keep setting, in automated backups configuration, is now

backup/util/checks/tests/checks_test.php

@@ -131,6 +131,7 @@ class backup_check_testcase extends advanced_testcase {
             backup::INTERACTIVE_NO, backup::MODE_GENERAL, $this->userid);
         $this->assertTrue(backup_check::check_security($bc, true));
         $this->assertTrue($bc instanceof backup_controller);
+        $bc->destroy();
 
     }
 }

backup/util/dbops/restore_dbops.class.php

@@ -101,9 +101,11 @@ abstract class restore_dbops {
 
             // If included, add it
             if ($included) {
-                $includedtasks[] = $task;
+                $includedtasks[] = clone($task); // A clone is enough. In fact we only need the basepath.
             }
         }
+        $rc->destroy(); // Always need to destroy.
+
         return $includedtasks;
     }
 
@@ -1510,8 +1512,12 @@ abstract class restore_dbops {
         // Calculate the context we are going to use for capability checking
         $context = context_course::instance($courseid);
 
+        // TODO: Some day we must kill this dependency and change the process
+        // to pass info around without loading a controller copy.
         // When conflicting users are detected we may need original site info.
-        $restoreinfo = restore_controller_dbops::load_controller($restoreid)->get_info();
+        $rc = restore_controller_dbops::load_controller($restoreid);
+        $restoreinfo = $rc->get_info();
+        $rc->destroy(); // Always need to destroy.
 
         // Calculate if we have perms to create users, by checking:
         // to 'moodle/restore:createuser' and 'moodle/restore:userinfo'

backup/util/helper/backup_helper.class.php

@@ -302,6 +302,7 @@ abstract class backup_helper {
                     $bc = backup_controller::load_controller($backupid);
                     $bc->log('Attempt to copy backup file to the specified directory using filesystem failed - ',
                             backup::LOG_WARNING, $dir);
+                    $bc->destroy();
                 }
                 // bad luck, try to deal with the file the old way - keep backup in file area if we can not copy to ext system
             }

backup/util/loggers/base_logger.class.php

@@ -71,6 +71,30 @@ abstract class base_logger implements checksumable {
         return $this->level;
     }
 
+    /**
+     * Destroy (nullify) the chain of loggers references, also closing resources when needed.
+     *
+     * @since Moodle 3.1
+     */
+    public final function destroy() {
+        // Recursively destroy the chain.
+        if ($this->next !== null) {
+            $this->next->destroy();
+            $this->next = null;
+        }
+        // And close every logger.
+        $this->close();
+    }
+
+    /**
+     * Close any resource the logger may have open.
+     *
+     * @since Moodle 3.1
+     */
+    public function close() {
+        // Nothing to do by default. Only loggers using resources (files, own connections...) need to override this.
+    }
+
 // checksumable interface methods
 
     public function calculate_checksum() {

backup/util/loggers/file_logger.class.php

@@ -66,6 +66,19 @@ class file_logger extends base_logger {
         }
     }
 
+    /**
+     * Close the logger resources (file handle) if still open.
+     *
+     * @since Moodle 3.1
+     */
+    public function close() {
+        // Close the file handle if hasn't been closed already.
+        if (is_resource($this->fhandle)) {
+            fclose($this->fhandle);
+            $this->fhandle = null;
+        }
+    }
+
 // Protected API starts here
 
     protected function action($message, $level, $options = null) {

backup/util/loggers/tests/logger_test.php

@@ -108,6 +108,16 @@ class backup_logger_testcase extends basic_testcase {
         $this->assertEquals($lo1->get_levelstr(backup::LOG_WARNING), 'warn');
         $this->assertEquals($lo1->get_levelstr(backup::LOG_INFO), 'info');
         $this->assertEquals($lo1->get_levelstr(backup::LOG_DEBUG), 'debug');
+
+        // Test destroy.
+        $lo1 = new mock_base_logger1(backup::LOG_ERROR);
+        $lo2 = new mock_base_logger2(backup::LOG_ERROR);
+        $lo1->set_next($lo2);
+        $this->assertInstanceOf('base_logger', $lo1->get_next());
+        $this->assertNull($lo2->get_next());
+        $lo1->destroy();
+        $this->assertNull($lo1->get_next());
+        $this->assertNull($lo2->get_next());
     }
 
     /**
@@ -249,9 +259,9 @@ class backup_logger_testcase extends basic_testcase {
         $result = $lo2->process($message2, backup::LOG_WARNING, $options);
         $this->assertTrue($result);
 
-        // Destruct loggers
-        $lo1 = null;
-        $lo2 = null;
+        // Destroy loggers.
+        $lo1->destroy();
+        $lo2->destroy();
 
         // Load file results to analyze them
         $fcontents = file_get_contents($file);
@@ -275,6 +285,7 @@ class backup_logger_testcase extends basic_testcase {
         $this->assertTrue(file_exists($file));
         $message = 'testing file_logger';
         $result = $lo->process($message, backup::LOG_ERROR, $options);
+        $lo->close(); // Closes logger.
         // Get file contents and inspect them
         $fcontents = file_get_contents($file);
         $this->assertTrue($result);
@@ -282,7 +293,6 @@ class backup_logger_testcase extends basic_testcase {
         $this->assertTrue(strpos($fcontents, '[error]') !== false);
         $this->assertTrue(strpos($fcontents, '  ') !== false);
         $this->assertTrue(substr_count($fcontents , '] ') >= 2);
-        $lo->__destruct(); // closes file handle
         unlink($file); // delete file
 
         // Instantiate, write something, force deletion, try to write again
@@ -292,7 +302,8 @@ class backup_logger_testcase extends basic_testcase {
         $this->assertTrue(file_exists($file));
         $message = 'testing file_logger';
         $result = $lo->process($message, backup::LOG_ERROR);
-        fclose($lo->get_fhandle()); // close file
+        $lo->close();
+        $this->assertNull($lo->get_fhandle());
         try {
             $result = @$lo->process($message, backup::LOG_ERROR); // Try to write again
             $this->assertTrue(false, 'base_logger_exception expected');
@@ -326,6 +337,7 @@ class backup_logger_testcase extends basic_testcase {
         $lo = new file_logger(backup::LOG_NONE, true, true, $file);
         $this->assertTrue($lo instanceof file_logger);
         $this->assertFalse(file_exists($file));
+        $lo->close();
 
         // Remove the test dir and any content
         @remove_dir(dirname($file));

backup/util/plan/tests/plan_test.php

@@ -89,6 +89,8 @@ class backup_plan_testcase extends advanced_testcase {
         // Calculate checksum and check it
         $checksum = $bp->calculate_checksum();
         $this->assertTrue($bp->is_checksum_correct($checksum));
+
+        $bc->destroy();
     }
 
     /**

backup/util/plan/tests/step_test.php

@@ -88,6 +88,7 @@ class backup_step_testcase extends advanced_testcase {
         $this->assertTrue($bs instanceof backup_step);
         $this->assertEquals($bs->get_name(), 'stepname');
 
+        $bc->destroy();
     }
 
     /**
@@ -128,6 +129,8 @@ class backup_step_testcase extends advanced_testcase {
         $this->assertTrue(strpos($contents, '<field2>value2</field2>') !== false);
         $this->assertTrue(strpos($contents, '</test>') !== false);
 
+        $bc->destroy();
+
         unlink($file); // delete file
 
         // Remove the test dir and any content

backup/util/plan/tests/task_test.php

@@ -93,6 +93,7 @@ class backup_task_testcase extends advanced_testcase {
         $checksum = $bt->calculate_checksum();
         $this->assertTrue($bt->is_checksum_correct($checksum));
 
+        $bc->destroy();
     }
 
     /**

course/tests/courselib_test.php

@@ -32,13 +32,6 @@ require_once($CFG->dirroot . '/enrol/imsenterprise/tests/imsenterprise_test.php'
 
 class core_course_courselib_testcase extends advanced_testcase {
 
-    /**
-     * Tidy up open files that may be left open.
-     */
-    protected function tearDown() {
-        gc_collect_cycles();
-    }
-
     /**
      * Set forum specific test values for calling create_module().
      *
@@ -1917,7 +1910,6 @@ class core_course_courselib_testcase extends advanced_testcase {
         $filepath = $CFG->dataroot . '/temp/backup/test-restore-course-event';
         $file->extract_to_pathname($fp, $filepath);
         $bc->destroy();
-        unset($bc);
 
         // Now we want to catch the restore course event.
         $sink = $this->redirectEvents();
@@ -1955,7 +1947,6 @@ class core_course_courselib_testcase extends advanced_testcase {
 
         // Destroy the resource controller since we are done using it.
         $rc->destroy();
-        unset($rc);
     }
 
     /**

course/tests/externallib_test.php

@@ -47,13 +47,6 @@ class core_course_externallib_testcase extends externallib_advanced_testcase {
         require_once($CFG->dirroot . '/course/externallib.php');
     }
 
-    /**
-     * Tidy up open files that may be left open.
-     */
-    protected function tearDown() {
-        gc_collect_cycles();
-    }
-
     /**
      * Test create_categories
      */

lib/tests/questionlib_test.php

@@ -51,13 +51,6 @@ class core_questionlib_testcase extends advanced_testcase {
         $this->resetAfterTest();
     }
 
-    /**
-     * Tidy up open files that may be left open.
-     */
-    protected function tearDown() {
-        gc_collect_cycles();
-    }
-
     /**
      * Return true and false to test functions with feedback on and off.
      *
@@ -248,7 +241,6 @@ class core_questionlib_testcase extends advanced_testcase {
         $filepath = $CFG->dataroot . '/temp/backup/test-restore-course';
         $file->extract_to_pathname($fp, $filepath);
         $bc->destroy();
-        unset($bc);
 
         // Now restore the course.
         $rc = new restore_controller('test-restore-course', $course2->id, backup::INTERACTIVE_NO,
@@ -262,6 +254,8 @@ class core_questionlib_testcase extends advanced_testcase {
 
         // Check that there are two questions in the restored to course's context.
         $this->assertEquals(2, $DB->count_records('question', array('category' => $restoredcategory->id)));
+
+        $rc->destroy();
     }
 
     /**