From a938e4096c6ea0021eecc6eca6997f1d3437532e Mon Sep 17 00:00:00 2001 From: sam marshall Date: Wed, 11 Oct 2017 12:16:12 +0100 Subject: [PATCH] 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 | 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 b53ffa67e18..f17a6edcc79 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 4de545e5a26..5f257cde1bb 100644 --- a/cohort/lib.php +++ b/cohort/lib.php @@ -504,6 +504,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 5df273b81fc..55ee9003456 100644 --- a/group/lib.php +++ b/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)); diff --git a/lib/blocklib.php b/lib/blocklib.php index 81d563ffb84..219a3782299 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 91eb81fc9c2..88cdee7d0b9 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 15b4ef4e58b..49960e28514 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 7c46fc357f5..9f22f184cea 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 f4d4ffaa84a..94ab2c654df 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 bad8317f10a..305f8a74e1f 100644 --- a/mod/glossary/lib.php +++ b/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, 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 b0ecb65c438..4ee7cc14301 100644 --- a/mod/glossary/view.php +++ b/mod/glossary/view.php @@ -521,6 +521,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 901ca66c002..af8a4e6fbc8 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 59123c49a06..65fba97e4de 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);