diff --git a/availability/classes/tree_node.php b/availability/classes/tree_node.php index 2b1ddc082be..218ae551d4a 100644 --- a/availability/classes/tree_node.php +++ b/availability/classes/tree_node.php @@ -34,6 +34,10 @@ defined('MOODLE_INTERNAL') || die(); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ abstract class tree_node { + + /** @var int Counter to be used in {@link tree_node::unique_sql_parameter()}. */ + protected static $uniquesqlparametercounter = 1; + /** * Determines whether this particular item is currently available * according to the availability criteria. @@ -242,10 +246,11 @@ abstract class tree_node { * @return SQL code for the parameter, e.g. ':pr1234' */ protected static function unique_sql_parameter(array &$params, $value) { - static $count = 1; + + // Note we intentionally do not use self:: here. + $count = tree_node::$uniquesqlparametercounter++; $unique = 'usp' . $count; $params[$unique] = $value; - $count++; return ':' . $unique; } } diff --git a/availability/tests/tree_test.php b/availability/tests/tree_test.php index ab6c30c7bcc..f82d78a08d3 100644 --- a/availability/tests/tree_test.php +++ b/availability/tests/tree_test.php @@ -675,6 +675,63 @@ class tree_testcase extends \advanced_testcase { json_encode(tree::get_root_json(array($child, $child), tree::OP_AND, array(true, false)))); } + /** + * Tests the behaviour of the counter in unique_sql_parameter(). + * + * There was a problem with static counters used to implement a sequence of + * parameter placeholders (MDL-53481). As always with static variables, it + * is a bit tricky to unit test the behaviour reliably as it depends on the + * actual tests executed and also their order. + * + * To minimise risk of false expected behaviour, this test method should be + * first one where {@link core_availability\tree::get_user_list_sql()} is + * used. We also use higher number of condition instances to increase the + * risk of the counter collision, should there remain a problem. + */ + public function test_unique_sql_parameter_behaviour() { + global $DB; + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + + // Create a test course with multiple groupings and groups and a student in each of them. + $course = $generator->create_course(); + $user = $generator->create_user(); + $studentroleid = $DB->get_field('role', 'id', array('shortname' => 'student')); + $generator->enrol_user($user->id, $course->id, $studentroleid); + // The total number of groupings and groups must not be greater than 61. + // There is a limit in MySQL on the max number of joined tables. + $groups = []; + for ($i = 0; $i < 25; $i++) { + $group = $generator->create_group(array('courseid' => $course->id)); + groups_add_member($group, $user); + $groups[] = $group; + } + $groupings = []; + for ($i = 0; $i < 25; $i++) { + $groupings[] = $generator->create_grouping(array('courseid' => $course->id)); + } + foreach ($groupings as $grouping) { + foreach ($groups as $group) { + groups_assign_grouping($grouping->id, $group->id); + } + } + $info = new \core_availability\mock_info($course); + + // Make a huge tree with 'AND' of all groups and groupings conditions. + $conditions = []; + foreach ($groups as $group) { + $conditions[] = \availability_group\condition::get_json($group->id); + } + foreach ($groupings as $groupingid) { + $conditions[] = \availability_grouping\condition::get_json($grouping->id); + } + shuffle($conditions); + $tree = new tree(tree::get_root_json($conditions)); + list($sql, $params) = $tree->get_user_list_sql(false, $info, false); + $result = $DB->get_fieldset_sql($sql, $params); + $this->assertEquals(array($user->id), $result); + } + /** * Tests get_user_list_sql. */