MDL-60174 core_dml: fix miscellaneous incorrect recordset usage

The new recordset support for Postgres requires transactions and
will cause errors if recordsets are not closed correctly. This
commit fixes problems that were identified during unit tests, and
via some basic code analysis, across all core code. Most of these
are incorrect usage of recordset (forgetting to close them).

admin/cli/fix_deleted_users.php

@@ -73,6 +73,7 @@
     echo "Redeleting user $user->id: $user->username ($user->email)\n";
     delete_user($user);
 }
+$rs->close();
 
 cli_heading('Deleting all leftovers');
 

admin/tool/messageinbound/index.php

@@ -40,6 +40,7 @@
     foreach ($records as $record) {
         $instances[] = \core\message\inbound\manager::get_handler($record->classname);
     }
+    $records->close();
 
     echo $OUTPUT->header();
     echo $renderer->messageinbound_handlers_table($instances);

admin/tool/spamcleaner/index.php

@@ -234,15 +234,19 @@ function search_spammers($keywords) {
     $keywordlist = implode(', ', $keywords);
     echo $OUTPUT->box(get_string('spamresult', 'tool_spamcleaner').s($keywordlist)).' ...';
 
-    print_user_list(array($spamusers_desc,
-                          $spamusers_blog,
-                          $spamusers_blogsub,
-                          $spamusers_comment,
-                          $spamusers_message,
-                          $spamusers_forumpost,
-                          $spamusers_forumpostsub
-                         ),
-                         $keywords);
+    $recordsets = [
+        $spamusers_desc,
+        $spamusers_blog,
+        $spamusers_blogsub,
+        $spamusers_comment,
+        $spamusers_message,
+        $spamusers_forumpost,
+        $spamusers_forumpostsub
+    ];
+    print_user_list($recordsets, $keywords);
+    foreach ($recordsets as $rs) {
+        $rs->close();
+    }
 }
 
 

analytics/classes/manager.php

@@ -360,6 +360,7 @@ public static function get_indicator_calculations($analysable, $starttime, $endt
             }
             $existingcalculations[$calculation->indicator][$calculation->sampleid] = $calculation->value;
         }
+        $calculations->close();
         return $existingcalculations;
     }
 

backup/moodle2/restore_stepslib.php

@@ -981,8 +981,8 @@ protected function define_execution() {
                 $DB->set_field('course_' . $table . 's', 'availability', $newvalue,
                         array('id' => $thingid));
             }
+            $rs->close();
         }
-        $rs->close();
     }
 }
 

cohort/lib.php

@@ -504,6 +504,7 @@ function cohort_get_invisible_contexts() {
             $excludedcontexts[] = $ctx->id;
         }
     }
+    $records->close();
     return $excludedcontexts;
 }
 

grade/grading/form/lib.php

@@ -428,6 +428,7 @@ public function get_active_instances($itemid) {
         foreach ($records as $record) {
             $rv[] = $this->get_instance($record);
         }
+        $records->close();
         return $rv;
     }
 

grade/report/singleview/classes/local/screen/screen.php

@@ -419,6 +419,7 @@ protected function load_users() {
         while ($user = $gui->next_user()) {
             $users[$user->user->id] = $user->user;
         }
+        $gui->close();
         return $users;
     }
 

group/lib.php

@@ -618,6 +618,7 @@ function groups_delete_groupings_groups($courseid, $showfeedback=false) {
     foreach ($results as $result) {
         groups_unassign_grouping($result->groupingid, $result->groupid, false);
     }
+    $results->close();
 
     // Invalidate the grouping cache for the course
     cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($courseid));
@@ -646,6 +647,7 @@ function groups_delete_groups($courseid, $showfeedback=false) {
     foreach ($groups as $group) {
         groups_delete_group($group);
     }
+    $groups->close();
 
     // Invalidate the grouping cache for the course
     cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($courseid));
@@ -676,6 +678,7 @@ function groups_delete_groupings($courseid, $showfeedback=false) {
     foreach ($groupings as $grouping) {
         groups_delete_grouping($grouping);
     }
+    $groupings->close();
 
     // Invalidate the grouping cache for the course.
     cache_helper::invalidate_by_definition('core', 'groupdata', array(), array($courseid));

lib/blocklib.php

@@ -800,6 +800,7 @@ public function load_blocks($includeinvisible = null) {
                 $unknown[] = $bi;
             }
         }
+        $blockinstances->close();
 
         // Pages don't necessarily have a defaultregion. The  one time this can
         // happen is when there are no theme block regions, but the script itself

lib/classes/message/inbound/manager.php

@@ -74,6 +74,7 @@ public static function update_handlers_for_component($componentname) {
                 self::remove_messageinbound_handler($handler);
             }
         }
+        $existinghandlers->close();
 
         self::create_missing_messageinbound_handlers_for_component($componentname);
     }

lib/classes/plugininfo/portfolio.php

@@ -43,6 +43,7 @@ public static function get_enabled_plugins() {
         foreach ($rs as $repository) {
             $enabled[$repository->plugin] = $repository->plugin;
         }
+        $rs->close();
 
         return $enabled;
     }
@@ -91,4 +92,4 @@ public function uninstall_cleanup() {
 
         parent::uninstall_cleanup();
     }
-}
+}

lib/db/upgrade.php

@@ -1183,6 +1183,7 @@ function xmldb_main_upgrade($oldversion) {
             $i++;
             $pbar->update($i, $total, "Updating duplicate question category stamp - $i/$total.");
         }
+        $rs->close();
         unset($usedstamps);
 
         // The uniqueness of each (contextid, stamp) pair is now guaranteed, so add the unique index to stop future duplicates.

lib/filestorage/file_storage.php

@@ -2001,6 +2001,7 @@ public function search_references($reference) {
         foreach ($rs as $filerecord) {
             $files[$filerecord->pathnamehash] = $this->get_file_instance($filerecord);
         }
+        $rs->close();
 
         return $files;
     }

lib/navigationlib.php

@@ -3258,6 +3258,7 @@ protected function load_category($categoryid, $nodetype = self::TYPE_CATEGORY) {
                 foreach ($categories as $category){
                     $coursesubcategories = array_merge($coursesubcategories, explode('/', trim($category->path, "/")));
                 }
+                $categories->close();
                 $coursesubcategories = array_unique($coursesubcategories);
 
                 // Only add a subcategory if it is part of the path to user's course and

lib/tablelib.php

@@ -1499,9 +1499,9 @@ function __construct($uniqueid) {
      * method or if other_cols returns NULL then put the data straight into the
      * table.
      *
-     * @return void
+     * After calling this function, don't forget to call close_recordset.
      */
-    function build_table() {
+    public function build_table() {
 
         if ($this->rawdata instanceof \Traversable && !$this->rawdata->valid()) {
             return;
@@ -1515,10 +1515,16 @@ function build_table() {
             $this->add_data_keyed($formattedrow,
                 $this->get_row_class($row));
         }
+    }
 
-        if ($this->rawdata instanceof \core\dml\recordset_walk ||
-                $this->rawdata instanceof moodle_recordset) {
+    /**
+     * Closes recordset (for use after building the table).
+     */
+    public function close_recordset() {
+        if ($this->rawdata && ($this->rawdata instanceof \core\dml\recordset_walk ||
+                $this->rawdata instanceof moodle_recordset)) {
             $this->rawdata->close();
+            $this->rawdata = null;
         }
     }
 
@@ -1629,6 +1635,7 @@ function out($pagesize, $useinitialsbar, $downloadhelpbutton='') {
         $this->setup();
         $this->query_db($pagesize, $useinitialsbar);
         $this->build_table();
+        $this->close_recordset();
         $this->finish_output();
     }
 }

lib/testing/classes/util.php

@@ -676,6 +676,7 @@ public static function reset_database() {
                     $mysqlsequences[$table] = $info->auto_increment;
                 }
             }
+            $rs->close();
         }
 
         foreach ($data as $table => $records) {

message/tests/externallib_test.php

@@ -902,15 +902,15 @@ public function test_mark_all_notifications_as_read() {
         $this->send_message($sender3, $recipient, 'Notification', 1);
 
         core_message_external::mark_all_notifications_as_read($recipient->id, $sender1->id);
-        $readnotifications = $DB->get_recordset('message_read', ['useridto' => $recipient->id]);
-        $unreadnotifications = $DB->get_recordset('message', ['useridto' => $recipient->id]);
+        $readnotifications = $DB->get_records('message_read', ['useridto' => $recipient->id]);
+        $unreadnotifications = $DB->get_records('message', ['useridto' => $recipient->id]);
 
         $this->assertCount(2, $readnotifications);
         $this->assertCount(4, $unreadnotifications);
 
         core_message_external::mark_all_notifications_as_read($recipient->id, 0);
-        $readnotifications = $DB->get_recordset('message_read', ['useridto' => $recipient->id]);
-        $unreadnotifications = $DB->get_recordset('message', ['useridto' => $recipient->id]);
+        $readnotifications = $DB->get_records('message_read', ['useridto' => $recipient->id]);
+        $unreadnotifications = $DB->get_records('message', ['useridto' => $recipient->id]);
 
         $this->assertCount(6, $readnotifications);
         $this->assertCount(0, $unreadnotifications);

mod/feedback/classes/responses_table.php

@@ -504,8 +504,6 @@ public function build_table() {
             }
         }
         $this->build_table_chunk($chunk, $columnsgroups);
-
-        $this->rawdata->close();
     }
 
     /**
@@ -631,6 +629,7 @@ public function export_external_structure($page = 0, $perpage = 0) {
         }
         $this->query_db($this->pagesize, false);
         $this->build_table();
+        $this->close_recordset();
         return $this->dataforexternal;
     }
 }

mod/forum/tests/subscriptions_test.php

@@ -28,11 +28,12 @@
 require_once($CFG->dirroot . '/mod/forum/lib.php');
 
 class mod_forum_subscriptions_testcase extends advanced_testcase {
-
     /**
      * Test setUp.
      */
     public function setUp() {
+        global $DB;
+
         // We must clear the subscription caches. This has to be done both before each test, and after in case of other
         // tests using these functions.
         \mod_forum\subscriptions::reset_forum_cache();
@@ -973,11 +974,11 @@ public function test_subscription_cache_prefill() {
         // Reset the subscription cache.
         \mod_forum\subscriptions::reset_forum_cache();
 
-        // Filling the subscription cache should only use a single query.
+        // Filling the subscription cache should use a query.
         $startcount = $DB->perf_get_reads();
         $this->assertNull(\mod_forum\subscriptions::fill_subscription_cache($forum->id));
         $postfillcount = $DB->perf_get_reads();
-        $this->assertEquals(1, $postfillcount - $startcount);
+        $this->assertNotEquals($postfillcount, $startcount);
 
         // Now fetch some subscriptions from that forum - these should use
         // the cache and not perform additional queries.
@@ -1049,7 +1050,7 @@ public function test_discussion_subscription_cache_fill_for_course() {
         $result = \mod_forum\subscriptions::fill_subscription_cache_for_course($course->id, $user->id);
         $this->assertNull($result);
         $postfillcount = $DB->perf_get_reads();
-        $this->assertEquals(1, $postfillcount - $startcount);
+        $this->assertNotEquals($postfillcount, $startcount);
         $this->assertFalse(\mod_forum\subscriptions::fetch_subscription_cache($disallowforum->id, $user->id));
         $this->assertFalse(\mod_forum\subscriptions::fetch_subscription_cache($chooseforum->id, $user->id));
         $this->assertTrue(\mod_forum\subscriptions::fetch_subscription_cache($initialforum->id, $user->id));
@@ -1064,7 +1065,7 @@ public function test_discussion_subscription_cache_fill_for_course() {
             $this->assertTrue(\mod_forum\subscriptions::fetch_subscription_cache($initialforum->id, $user->id));
         }
         $finalcount = $DB->perf_get_reads();
-        $this->assertEquals(count($users), $finalcount - $postfillcount);
+        $this->assertNotEquals($finalcount, $postfillcount);
     }
 
     /**
@@ -1117,7 +1118,7 @@ public function test_discussion_subscription_cache_prefill() {
         $startcount = $DB->perf_get_reads();
         $this->assertNull(\mod_forum\subscriptions::fill_discussion_subscription_cache($forum->id));
         $postfillcount = $DB->perf_get_reads();
-        $this->assertEquals(1, $postfillcount - $startcount);
+        $this->assertNotEquals($postfillcount, $startcount);
 
         // Now fetch some subscriptions from that forum - these should use
         // the cache and not perform additional queries.
@@ -1184,7 +1185,7 @@ public function test_discussion_subscription_cache_fill() {
             $this->assertInternalType('array', $result);
         }
         $finalcount = $DB->perf_get_reads();
-        $this->assertEquals(20, $finalcount - $startcount);
+        $this->assertNotEquals($finalcount, $startcount);
     }
 
     /**

mod/glossary/lib.php

@@ -3795,7 +3795,7 @@ function glossary_get_search_terms_sql(array $terms, $fullsearch = true, $glossa
  * @param  array $options Accepts:
  *                        - (bool) includenotapproved. When false, includes the non-approved entries created by
  *                          the current user. When true, also includes the ones that the user has the permission to approve.
- * @return array The first element being the recordset, the second the number of entries.
+ * @return array The first element being the array of results, the second the number of entries.
  * @since Moodle 3.1
  */
 function glossary_get_entries_by_search($glossary, $context, $query, $fullsearch, $order, $sort, $from, $limit,

mod/glossary/print.php

@@ -216,6 +216,10 @@
 
         glossary_print_entry($course, $cm, $glossary, $entry, $mode, $hook, 1, $displayformat, true);
     }
+    // The all entries value may be a recordset or an array.
+    if ($allentries instanceof moodle_recordset) {
+        $allentries->close();
+    }
 }
 
 echo $OUTPUT->footer();

mod/glossary/view.php

@@ -521,6 +521,10 @@
         glossary_print_entry($course, $cm, $glossary, $entry, $mode, $hook,1,$displayformat);
         $entriesshown++;
     }
+    // The all entries value may be a recordset or an array.
+    if ($allentries instanceof moodle_recordset) {
+        $allentries->close();
+    }
 }
 if ( !$entriesshown ) {
     echo $OUTPUT->box(get_string("noentries","glossary"), "generalbox boxaligncenter boxwidthwide");

mod/quiz/tests/attempts_test.php

@@ -306,13 +306,15 @@ public function test_bulk_update_functions() {
             $count++;
 
         }
+        $attempts->close();
         $this->assertEquals($DB->count_records_select('quiz_attempts', 'timecheckstate IS NOT NULL'), $count);
 
         $attempts = $overduehander->get_list_of_overdue_attempts(0); // before all attempts
         $count = 0;
         foreach ($attempts as $attempt) {
             $count++;
         }
+        $attempts->close();
         $this->assertEquals(0, $count);
 
     }

mod/wiki/locallib.php

@@ -1240,7 +1240,7 @@ function wiki_delete_page_versions($deleteversions, $context = null) {
             list($insql, $param) = $DB->get_in_or_equal($versions);
             $insql .= ' AND pageid = ?';
             array_push($param, $params['pageid']);
-            $oldversions = $DB->get_recordset_select('wiki_versions', 'version ' . $insql, $param);
+            $oldversions = $DB->get_records_select('wiki_versions', 'version ' . $insql, $param);
             $DB->delete_records_select('wiki_versions', 'version ' . $insql, $param);
         }
         foreach ($oldversions as $version) {

question/classes/bank/view.php

@@ -408,6 +408,7 @@ protected function load_page_questions($page, $perpage) {
         $questions = $DB->get_recordset_sql($this->loadsql, $this->sqlparams, $page * $perpage, $perpage);
         if (!$questions->valid()) {
             // No questions on this page. Reset to page 0.
+            $questions->close();
             $questions = $DB->get_recordset_sql($this->loadsql, $this->sqlparams, 0, $perpage);
         }
         return $questions;
@@ -708,6 +709,7 @@ protected function display_question_list($contexts, $pageurl, $categoryandcontex
             $this->print_table_row($question, $rowcount);
             $rowcount += 1;
         }
+        $questions->close();
         $this->end_table();
         echo "</div>\n";
 

report/security/locallib.php

@@ -828,6 +828,7 @@ function report_security_check_riskbackup($detailed=false) {
                                'contextname'=>$context->get_context_name());
             $users[] = '<li>'.get_string('check_riskbackup_unassign', 'report_security', $a).'</li>';
         }
+        $rs->close();
         if (!empty($users)) {
             $users = '<ul>'.implode('', $users).'</ul>';
             $result->details .= get_string('check_riskbackup_details_users', 'report_security', $users);