From e22a93a60396145958947f5e5d0b31455dbcd06c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?David=20Mudr=C3=A1k?= <david@moodle.com>
Date: Tue, 8 Nov 2016 11:07:53 +0100
Subject: [PATCH] MDL-53481 availability: Fix uniqueness of SQL named param
 placeholders

Static variables do not behave the way you might expect when accessing
them through the classes inheritance. When accessing a method via self::
or static:: operators, even though the method is inherited, its variable
scope is not. So the method unique_sql_parameter() was using the scope
of the child class and each child class had its own sequence of usp1,
usp2, usp3, ... placeholders. This led to "Incorrect number of query
parameters" error when multiuple condition classes were contributing to
a single SQL query.

All credit should go to Adam Olley who debugged and described the
essence of the problem in the tracker.
---
 availability/classes/tree_node.php |  9 +++--
 availability/tests/tree_test.php   | 57 ++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 2 deletions(-)

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.
      */