MDL-77293 libraries: fix array query parameter handling

matthewhilton · matthewhilton · commit f2ebad09afc5 · 2024-11-25T08:08:53.000+10:00
diff --git a/lib/classes/url.php b/lib/classes/url.php
@@ -153,7 +153,9 @@ public function __construct(
             }
             if (isset($parts['query'])) {
                 // Note: the values may not be correctly decoded, url parameters should be always passed as array.
-                parse_str(str_replace('&amp;', '&', $parts['query']), $this->params);
+                $out = [];
+                parse_str(str_replace('&amp;', '&', $parts['query']), $out);
+                $this->params($out);
             }
             unset($parts['query']);
             foreach ($parts as $key => $value) {
@@ -179,28 +181,36 @@ public function __construct(
      *
      * The added params override existing ones if they have the same name.
      *
-     * @param null|array $params Defaults to null. If null then returns all params.
+     * @param null|array $params Array of parameters to add. Note all values that are not arrays are cast to strings.
      * @return array Array of Params for url.
      * @throws coding_exception
      */
     public function params(?array $params = null) {
         $params = (array)$params;
+        $params = $this->clean_url_params($params);
+        $this->params = array_merge($this->params, $params);
+        return $this->params;
+    }
 
-        foreach ($params as $key => $value) {
-            if (is_int($key)) {
-                throw new coding_exception('Url parameters can not have numeric keys!');
-            }
-            if (!is_string($value)) {
-                if (is_array($value)) {
-                    throw new coding_exception('Url parameters values can not be arrays!');
-                }
-                if (is_object($value) && !method_exists($value, '__toString')) {
-                    throw new coding_exception('Url parameters values can not be objects, unless __toString() is defined!');
-                }
+    /**
+     * Converts given URL parameter values that are not arrays into strings.
+     *
+     * This is to maintain the same behaviour as the original params() function.
+     *
+     * @param array $params
+     * @return array
+     */
+    private function clean_url_params(array $params): array {
+        // Convert all values to strings.
+        // This was the original implementation of the params function,
+        // which we have kept for backwards compatibility.
+        array_walk_recursive($params, function(&$value) {
+            if (is_object($value) && !method_exists($value, '__toString')) {
+                throw new coding_exception('Url parameters values can not be objects, unless __toString() is defined!');
             }
-            $this->params[$key] = (string)$value;
-        }
-        return $this->params;
+            $value = (string) $value;
+        });
+        return $params;
     }
 
     /**
@@ -246,7 +256,7 @@ public function remove_all_params($unused = null) {
      *
      * @param string $paramname name
      * @param string $newvalue Param value. If new value specified current value is overriden or parameter is added
-     * @return mixed string parameter value, null if parameter does not exist
+     * @return array|string|null parameter value, null if parameter does not exist.
      */
     public function param($paramname, $newvalue = '') {
         if (func_num_args() > 1) {
@@ -261,28 +271,65 @@ public function param($paramname, $newvalue = '') {
     }
 
     /**
-     * Merges parameters and validates them
+     * Merges parameters.
      *
      * @param null|array $overrideparams
      * @return array merged parameters
-     * @throws coding_exception
      */
     protected function merge_overrideparams(?array $overrideparams = null) {
-        $overrideparams = (array)$overrideparams;
-        $params = $this->params;
-        foreach ($overrideparams as $key => $value) {
-            if (is_int($key)) {
-                throw new coding_exception('Overridden parameters can not have numeric keys!');
-            }
-            if (is_array($value)) {
-                throw new coding_exception('Overridden parameters values can not be arrays!');
-            }
-            if (is_object($value) && !method_exists($value, '__toString')) {
-                throw new coding_exception('Overridden parameters values can not be objects, unless __toString() is defined!');
+        $overrideparams = (array) $overrideparams;
+        $overrideparams = $this->clean_url_params($overrideparams);
+        return array_merge($this->params, $overrideparams);
+    }
+
+    /**
+     * Recursively transforms the given array of values to query string parts.
+     *
+     * Example query string parts: a=2, a[0]=2
+     *
+     * @param array $data Data to encode into query string parts. Can be a multi level array. All end values must be strings.
+     * @return array array of query string parts. All parts are rawurlencoded.
+     */
+    private function recursively_transform_params_to_query_string_parts(array $data): array {
+        $stringparams = [];
+
+        // Define a recursive function to encode the array into a set of string params.
+        // We need to do this recursively, so that multi level array parameters are properly supported.
+        $parsestringparams = function (array $data) use (&$stringparams, &$parsestringparams) {
+            foreach ($data as $key => $value) {
+                // If this is an array, rewrite the $value keys to track the position in the array.
+                // and pass back to this function recursively until the values are no longer arrays.
+                // E.g. if $key is 'a' and $value was [0 => true, 1 => false]
+                // the new array becomes ['a[0]' => true, 'a[1]' => false].
+                if (is_array($value)) {
+                    $newvalue = [];
+                    foreach ($value as $innerkey => $innervalue) {
+                        $newkey = $key . '[' . $innerkey . ']';
+                        $newvalue[$newkey] = $innervalue;
+                    }
+                    $parsestringparams($newvalue);
+                } else {
+                    // Else no more arrays to traverse - build the final query string part.
+                    // We enforce that all end values are strings for consistency.
+                    // When params() is used, it will convert all params given to strings.
+                    // This will catch out anyone setting the params property directly.
+                    if (!is_string($value)) {
+                        throw new coding_exception('Unexpected query string value type.
+All values that are not arrays should be a string.');
+                    }
+
+                    if (isset($value) && $value !== '') {
+                        $stringparams[] = rawurlencode($key) . '=' . rawurlencode($value);
+                    } else {
+                        $stringparams[] = rawurlencode($key);
+                    }
+                }
             }
-            $params[$key] = (string)$value;
-        }
-        return $params;
+        };
+
+        $parsestringparams($data);
+
+        return $stringparams;
     }
 
     /**
@@ -296,29 +343,18 @@ protected function merge_overrideparams(?array $overrideparams = null) {
      * @return string query string that can be added to a url.
      */
     public function get_query_string($escaped = true, ?array $overrideparams = null) {
-        $arr = [];
         if ($overrideparams !== null) {
             $params = $this->merge_overrideparams($overrideparams);
         } else {
             $params = $this->params;
         }
-        foreach ($params as $key => $val) {
-            if (is_array($val)) {
-                foreach ($val as $index => $value) {
-                    $arr[] = rawurlencode($key . '[' . $index . ']') . "=" . rawurlencode($value);
-                }
-            } else {
-                if (isset($val) && $val !== '') {
-                    $arr[] = rawurlencode($key) . "=" . rawurlencode($val);
-                } else {
-                    $arr[] = rawurlencode($key);
-                }
-            }
-        }
+
+        $stringparams = $this->recursively_transform_params_to_query_string_parts($params);
+
         if ($escaped) {
-            return implode('&amp;', $arr);
+            return implode('&amp;', $stringparams);
         } else {
-            return implode('&', $arr);
+            return implode('&', $stringparams);
         }
     }
 
@@ -330,17 +366,28 @@ public function get_query_string($escaped = true, ?array $overrideparams = null)
      * @return array params array for templates.
      */
     public function export_params_for_template(): array {
-        $data = [];
-        foreach ($this->params as $key => $val) {
-            if (is_array($val)) {
-                foreach ($val as $index => $value) {
-                    $data[] = ['name' => $key . '[' . $index . ']', 'value' => $value];
-                }
-            } else {
-                $data[] = ['name' => $key, 'value' => $val];
+        $querystringparts = $this->recursively_transform_params_to_query_string_parts($this->params);
+
+        return array_map(function ($value) {
+            // First urldecode it, they are encoded by default.
+            $value = rawurldecode($value);
+
+            // Now seperate the parts into name and value.
+            // splitting on the first '=' sign.
+            $parts = explode('=', $value, 2);
+
+            // Parts must be of length 1 or 2, anything else is an invalid.
+            if (count($parts) !== 1 && count($parts) !== 2) {
+                throw new coding_exception('Invalid query string construction, unexpected number of parts');
             }
-        }
-        return $data;
+
+            // There might not always be a '=' e.g. when the value is an empty string.
+            // in this case, just fallback to an empty string.
+            $name = $parts[0];
+            $value = $parts[1] ?? '';
+
+            return ['name' => $name, 'value' => $value];
+        }, $querystringparts);
     }
 
     /**
@@ -847,7 +894,7 @@ public function get_path($includeslashargument = true) {
      * Returns a given parameter value from the URL.
      *
      * @param string $name Name of parameter
-     * @return string Value of parameter or null if not set
+     * @return array|string|null Value of parameter or null if not set.
      */
     public function get_param($name) {
         if (array_key_exists($name, $this->params)) {
diff --git a/lib/tests/url_test.php b/lib/tests/url_test.php
@@ -289,6 +289,16 @@ public static function export_params_for_template_provider(): array {
                         4 => ['name' => 'param3', 'value' => '3'],
                     ],
                 ],
+                'multi level array embedded with other params' => [
+                    'url' => "@{$baseurl}/?param1=1&tags[0][0]=123&tags[0][1]=456&param2=2&param3=3",
+                    'expected' => [
+                        0 => ['name' => 'param1', 'value' => '1'],
+                        1 => ['name' => 'tags[0][0]', 'value' => '123'],
+                        2 => ['name' => 'tags[0][1]', 'value' => '456'],
+                        3 => ['name' => 'param2', 'value' => '2'],
+                        4 => ['name' => 'param3', 'value' => '3'],
+                    ],
+                ],
                 'params with array at the end' => [
                     'url' => "@{$baseurl}/?param1=1&tags[]=123&tags[]=456",
                     'expected' => [
@@ -686,4 +696,72 @@ public function test_routed_path(): void {
         $url = url::routed_path('/example');
         $this->assertSame('/example', $url->out_as_local_url(false));
     }
+
+    /**
+     * Provides various urls with multi level array query parameters
+     *
+     * @return array
+     */
+    public static function multi_level_query_params_provider(): array {
+        return [
+            'multi level with integer values' => [
+                'url' => 'https://example.moodle.net?test[0][0]=1&test[0][1]=0',
+                'extraparams' => [],
+                'expectedparams' => ['test' => [0 => ['1', '0']]],
+                'expectedurlout' => 'https://example.moodle.net?test%5B0%5D%5B0%5D=1&test%5B0%5D%5B1%5D=0',
+            ],
+            'multi level with bool-looking string values' => [
+                'url' => 'https://example.moodle.net?test[0][0]=true&test[0][1]=false',
+                'extraparams' => [],
+                // These are actually strings, and should be interpreted as such,
+                // even if they look like booleans.
+                'expectedparams' => ['test' => [0 => ['true', 'false']]],
+                'expectedurlout' => 'https://example.moodle.net?test%5B0%5D%5B0%5D=true&test%5B0%5D%5B1%5D=false',
+            ],
+            'multi level with bool params values' => [
+                'url' => 'https://example.moodle.net',
+                'extraparams' => ['test' => [0 => [true, false]]],
+                'expectedparams' => ['test' => [0 => ['1', '']]],
+                // Bool values get stringified. This means true = 1 and false = ''.
+                'expectedurlout' => 'https://example.moodle.net?test%5B0%5D%5B0%5D=1&test%5B0%5D%5B1%5D',
+            ],
+            'triple level array with string values' => [
+                'url' => 'https://example.moodle.net?test[0][0][0]=abc&test[0][0][1]=xyz',
+                'extraparams' => [],
+                'expectedparams' => ['test' => [0 => [0 => ['abc', 'xyz']]]],
+                'expectedurlout' => 'https://example.moodle.net?test%5B0%5D%5B0%5D%5B0%5D=abc&test%5B0%5D%5B0%5D%5B1%5D=xyz',
+            ],
+            'multi level params with empty arrays as values' => [
+                'url' => 'https://example.moodle.net',
+                'extraparams' => ['test' => [[[]], [[], []]]],
+                'expectedparams' => ['test' => [[[]], [[], []]]],
+                // Empty arrays don't hold any data; they are just containers.
+                // So this should not have the param present in the query params.
+                'expectedurlout' => 'https://example.moodle.net',
+            ],
+            'multi level params with non sequential arrays keys' => [
+                'url' => 'https://example.moodle.net?test[2]=a&test[0]=b',
+                'extraparams' => [],
+                'expectedparams' => ['test' => [2 => 'a', 0 => 'b']],
+                'expectedurlout' => 'https://example.moodle.net?test%5B2%5D=a&test%5B0%5D=b',
+            ],
+        ];
+    }
+
+    /**
+     * Tests url parameter handling where multi level arrays are involved.
+     *
+     * @param string $urlstring url string to parse.
+     * @param array $extraparams extra parameters to pass directly to ->params() function.
+     * @param array $expectedparams php array of expected parameters expected to be parsed.
+     * @param string $expectedurlout unescaped url string that is expected when calling ->out() on the url object.
+     * @dataProvider multi_level_query_params_provider
+     */
+    public function test_multi_level_array_query_params(string $urlstring, array $extraparams, array $expectedparams,
+        string $expectedurlout): void {
+        $url = new url($urlstring);
+        $url->params($extraparams);
+        $this->assertSame($expectedparams, $url->params());
+        $this->assertSame($expectedurlout, $url->out(false));
+    }
 }
