From f83d748f7767f1621ed2aebbffe366dacc6eaed5 Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 11 Oct 2017 12:16:12 +0100 Subject: [PATCH] MDL-60915 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 | 1 + admin/tool/messageinbound/index.php | 1 + admin/tool/spamcleaner/index.php | 22 +++++++++++-------- analytics/classes/manager.php | 1 + backup/moodle2/restore_stepslib.php | 2 +- cohort/lib.php | 1 + grade/grading/form/lib.php | 1 + .../classes/local/screen/screen.php | 1 + group/lib.php | 3 +++ lib/blocklib.php | 1 + lib/classes/message/inbound/manager.php | 1 + lib/classes/plugininfo/portfolio.php | 3 ++- lib/db/upgrade.php | 1 + lib/filestorage/file_storage.php | 1 + lib/navigationlib.php | 1 + lib/tablelib.php | 15 +++++++++---- lib/testing/classes/util.php | 1 + message/tests/externallib_test.php | 8 +++---- mod/feedback/classes/responses_table.php | 3 +-- mod/forum/tests/subscriptions_test.php | 15 +++++++------ mod/glossary/lib.php | 2 +- mod/glossary/print.php | 4 ++++ mod/glossary/view.php | 4 ++++ mod/quiz/tests/attempts_test.php | 2 ++ mod/wiki/locallib.php | 2 +- question/classes/bank/view.php | 2 ++ report/security/locallib.php | 1 + 27 files changed, 70 insertions(+), 30 deletions(-) diff --git a/admin/cli/fix_deleted_users.php b/admin/cli/fix_deleted_users.php index d2a895057a2..cc052b4e702 100644 --- a/admin/cli/fix_deleted_users.php +++ b/admin/cli/fix_deleted_users.php @@ -73,6 +73,7 @@ foreach ($rs as $user) { echo "Redeleting user $user->id: $user->username ($user->email)\n"; delete_user($user); } +$rs->close(); cli_heading('Deleting all leftovers'); diff --git a/admin/tool/messageinbound/index.php b/admin/tool/messageinbound/index.php index 8d04d450aed..90c2a128933 100644 --- a/admin/tool/messageinbound/index.php +++ b/admin/tool/messageinbound/index.php @@ -40,6 +40,7 @@ if (empty($classname)) { foreach ($records as $record) { $instances[] = \core\message\inbound\manager::get_handler($record->classname); } + $records->close(); echo $OUTPUT->header(); echo $renderer->messageinbound_handlers_table($instances); diff --git a/admin/tool/spamcleaner/index.php b/admin/tool/spamcleaner/index.php index 63d3485e5dd..2f98bf41006 100644 --- a/admin/tool/spamcleaner/index.php +++ b/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(); + } } diff --git a/analytics/classes/manager.php b/analytics/classes/manager.php index 4b0bcac480d..cdf787e2853 100644 --- a/analytics/classes/manager.php +++ b/analytics/classes/manager.php @@ -360,6 +360,7 @@ class manager { } $existingcalculations[$calculation->indicator][$calculation->sampleid] = $calculation->value; } + $calculations->close(); return $existingcalculations; } diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 9b87054f72e..529645f9bf8 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -981,8 +981,8 @@ class restore_process_course_modules_availability extends restore_execution_step $DB->set_field('course_' . $table . 's', 'availability', $newvalue, array('id' => $thingid)); } + $rs->close(); } - $rs->close(); } } diff --git a/cohort/lib.php b/cohort/lib.php index c319946dc7a..f3c3a16a933 100644 --- a/cohort/lib.php +++ b/cohort/lib.php @@ -507,6 +507,7 @@ function cohort_get_invisible_contexts() { $excludedcontexts[] = $ctx->id; } } + $records->close(); return $excludedcontexts; } diff --git a/grade/grading/form/lib.php b/grade/grading/form/lib.php index b79f5c77362..2f0666221df 100644 --- a/grade/grading/form/lib.php +++ b/grade/grading/form/lib.php @@ -428,6 +428,7 @@ abstract class gradingform_controller { foreach ($records as $record) { $rv[] = $this->get_instance($record); } + $records->close(); return $rv; } diff --git a/grade/report/singleview/classes/local/screen/screen.php b/grade/report/singleview/classes/local/screen/screen.php index 56709dfcb6f..db3ecb601e3 100644 --- a/grade/report/singleview/classes/local/screen/screen.php +++ b/grade/report/singleview/classes/local/screen/screen.php @@ -419,6 +419,7 @@ abstract class screen { while ($user = $gui->next_user()) { $users[$user->user->id] = $user->user; } + $gui->close(); return $users; } diff --git a/group/lib.php b/group/lib.php index 99a4c6ec309..57bd61610da 100644 --- a/group/lib.php +++ b/group/lib.php @@ -628,6 +628,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)); @@ -658,6 +659,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)); @@ -690,6 +692,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)); diff --git a/lib/blocklib.php b/lib/blocklib.php index 7e2d79b6707..c8fa2455a08 100644 --- a/lib/blocklib.php +++ b/lib/blocklib.php @@ -800,6 +800,7 @@ class block_manager { $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 diff --git a/lib/classes/message/inbound/manager.php b/lib/classes/message/inbound/manager.php index 8534df679e4..9bc16cddb38 100644 --- a/lib/classes/message/inbound/manager.php +++ b/lib/classes/message/inbound/manager.php @@ -74,6 +74,7 @@ class manager { self::remove_messageinbound_handler($handler); } } + $existinghandlers->close(); self::create_missing_messageinbound_handlers_for_component($componentname); } diff --git a/lib/classes/plugininfo/portfolio.php b/lib/classes/plugininfo/portfolio.php index 78e94f40a96..80f57d50c77 100644 --- a/lib/classes/plugininfo/portfolio.php +++ b/lib/classes/plugininfo/portfolio.php @@ -43,6 +43,7 @@ class portfolio extends base { foreach ($rs as $repository) { $enabled[$repository->plugin] = $repository->plugin; } + $rs->close(); return $enabled; } @@ -91,4 +92,4 @@ class portfolio extends base { parent::uninstall_cleanup(); } -} \ No newline at end of file +} diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index a731b661628..376a3010071 100644 --- a/lib/db/upgrade.php +++ b/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. diff --git a/lib/filestorage/file_storage.php b/lib/filestorage/file_storage.php index e15d0715d17..2c1906db990 100644 --- a/lib/filestorage/file_storage.php +++ b/lib/filestorage/file_storage.php @@ -2001,6 +2001,7 @@ class file_storage { foreach ($rs as $filerecord) { $files[$filerecord->pathnamehash] = $this->get_file_instance($filerecord); } + $rs->close(); return $files; } diff --git a/lib/navigationlib.php b/lib/navigationlib.php index 11ef46b7200..226d024c199 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -3258,6 +3258,7 @@ class global_navigation_for_ajax extends global_navigation { 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 diff --git a/lib/tablelib.php b/lib/tablelib.php index d1f1adc0b4e..835c751e522 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -1499,9 +1499,9 @@ class table_sql extends flexible_table { * 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 @@ class table_sql extends flexible_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 @@ class table_sql extends flexible_table { $this->setup(); $this->query_db($pagesize, $useinitialsbar); $this->build_table(); + $this->close_recordset(); $this->finish_output(); } } diff --git a/lib/testing/classes/util.php b/lib/testing/classes/util.php index 4a8b682e133..533200850a6 100644 --- a/lib/testing/classes/util.php +++ b/lib/testing/classes/util.php @@ -676,6 +676,7 @@ abstract class testing_util { $mysqlsequences[$table] = $info->auto_increment; } } + $rs->close(); } foreach ($data as $table => $records) { diff --git a/message/tests/externallib_test.php b/message/tests/externallib_test.php index 8f28c29dc93..b7392f18833 100644 --- a/message/tests/externallib_test.php +++ b/message/tests/externallib_test.php @@ -902,15 +902,15 @@ class core_message_externallib_testcase extends externallib_advanced_testcase { $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); diff --git a/mod/feedback/classes/responses_table.php b/mod/feedback/classes/responses_table.php index 9426775eba1..50b7d9efbff 100644 --- a/mod/feedback/classes/responses_table.php +++ b/mod/feedback/classes/responses_table.php @@ -504,8 +504,6 @@ class mod_feedback_responses_table extends table_sql { } } $this->build_table_chunk($chunk, $columnsgroups); - - $this->rawdata->close(); } /** @@ -631,6 +629,7 @@ class mod_feedback_responses_table extends table_sql { } $this->query_db($this->pagesize, false); $this->build_table(); + $this->close_recordset(); return $this->dataforexternal; } } diff --git a/mod/forum/tests/subscriptions_test.php b/mod/forum/tests/subscriptions_test.php index f435f00b982..276ccf7d081 100644 --- a/mod/forum/tests/subscriptions_test.php +++ b/mod/forum/tests/subscriptions_test.php @@ -28,11 +28,12 @@ global $CFG; 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 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { // 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 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $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 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $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 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $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 @@ class mod_forum_subscriptions_testcase extends advanced_testcase { $this->assertInternalType('array', $result); } $finalcount = $DB->perf_get_reads(); - $this->assertEquals(20, $finalcount - $startcount); + $this->assertNotEquals($finalcount, $startcount); } /** diff --git a/mod/glossary/lib.php b/mod/glossary/lib.php index 7edc45b777b..c78f9a2a5f1 100644 --- a/mod/glossary/lib.php +++ b/mod/glossary/lib.php @@ -3805,7 +3805,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, diff --git a/mod/glossary/print.php b/mod/glossary/print.php index ece56511dcc..292d892c5c8 100644 --- a/mod/glossary/print.php +++ b/mod/glossary/print.php @@ -216,6 +216,10 @@ if ( $allentries ) { 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(); diff --git a/mod/glossary/view.php b/mod/glossary/view.php index b24ca88b3b3..5adc32fc268 100644 --- a/mod/glossary/view.php +++ b/mod/glossary/view.php @@ -524,6 +524,10 @@ if ($allentries) { 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"); diff --git a/mod/quiz/tests/attempts_test.php b/mod/quiz/tests/attempts_test.php index a0f1dae918b..00a638d8ef4 100644 --- a/mod/quiz/tests/attempts_test.php +++ b/mod/quiz/tests/attempts_test.php @@ -306,6 +306,7 @@ class mod_quiz_attempt_overdue_testcase extends advanced_testcase { $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 @@ -313,6 +314,7 @@ class mod_quiz_attempt_overdue_testcase extends advanced_testcase { foreach ($attempts as $attempt) { $count++; } + $attempts->close(); $this->assertEquals(0, $count); } diff --git a/mod/wiki/locallib.php b/mod/wiki/locallib.php index 725b5312ab5..f3fe8382290 100644 --- a/mod/wiki/locallib.php +++ b/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) { diff --git a/question/classes/bank/view.php b/question/classes/bank/view.php index 102531dbc17..8c4142b580c 100644 --- a/question/classes/bank/view.php +++ b/question/classes/bank/view.php @@ -408,6 +408,7 @@ class view { $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 @@ class view { $this->print_table_row($question, $rowcount); $rowcount += 1; } + $questions->close(); $this->end_table(); echo "\n"; diff --git a/report/security/locallib.php b/report/security/locallib.php index a7e67401868..acbd1483133 100644 --- a/report/security/locallib.php +++ b/report/security/locallib.php @@ -828,6 +828,7 @@ function report_security_check_riskbackup($detailed=false) { 'contextname'=>$context->get_context_name()); $users[] = '
  • '.get_string('check_riskbackup_unassign', 'report_security', $a).'
    • '; } + $rs->close(); if (!empty($users)) { $users = ''; $result->details .= get_string('check_riskbackup_details_users', 'report_security', $users);