MDL-84300 router: Path params should be aware of unlimited captures

Where a path contains an unlimited parameter, designated by `{name:.*}`
or `{name:.*?}` it may be empty and therefore cannot be required.

lib/classes/router/schema/parameters/path_parameter.php

@@ -103,11 +103,30 @@ class path_parameter extends parameter {
     public function is_required(route $route): bool {
         $path = $route->get_path();
 
-        // Find the position of the parameter in the path.
-        $paramposition = strpos($path, '{' . $this->name . '}');
+        // Find the parameter in the path.
+        // Search for `{value` with an optional : followed by anything except a closing `}`, and then a closing `}`.
+        // ~^(?<match>.*\{{$this->name}(?:\:[^}]*)?\})~
+        // ~                                          ~ => Delimiters
+        //  ^(?<match>                               )  => Named capture group
+        //            .*\{                              => Any character, any number of times, followed by {
+        //                {$this->name}                 => The parameter name
+        //                             (?:       )?     => Optional non-capturing group
+        //                                \:[^}]*       => : followed by anything except }
+        //                                         \}   => Closing }
+        // If the parameter is not found in the path, then it is not required.
+        $matchesfound = preg_match(
+            "~^(?<match>.*\{{$this->name}(?:\:[^}]*)?\})~",
+            $path,
+            $matches,
+        );
+
+        if ($matchesfound === 0) {
+            // Parameter not found in the path.
+            return false;
+        }
 
         // If _any_ part of the path before the parameter contains a '[' character, then this _must_ be optional.
         // A required parameter cannot follow an optional parameter.
-        return !str_contains(substr($path, 0, $paramposition), '[');
+        return str_contains($matches['match'], '[') === false;
     }
 }

lib/classes/router/schema/specification.php

@@ -280,9 +280,17 @@ class specification implements
         // Note: We use this helper because OpenAPI does not support optional parameters.
         // Therefore we must handle that in Moodle, adding path variants with and without each optional parameter.
         $addpath = function (string $path) use ($route, $component) {
-            // Remove the optional parameters delimiters from the path.
             $path = str_replace(
-                ['[', ']'],
+                [
+                    // Remove the optional parameters delimiters from the path.
+                    '[',
+                    ']',
+
+                    // Remove the greedy and non-greedy unlimited delimters from the path too.
+                    // These are a FastRoute feature not compatible with OpenAPI.
+                    ':.*?',
+                    ':.*',
+                ],
                 '',
                 $path,
             );

lib/tests/router/request_validator_test.php

@@ -90,34 +90,6 @@ final class request_validator_test extends route_testcase {
         );
     }
 
-    /**
-     * When a defined pathtype is missing from the path.
-     */
-    public function test_validate_request_missing_path_component(): void {
-        // A route with a parameter defined in the path, but no pathtype for it.
-        $route = new route(
-            path: '/example/123',
-            pathtypes: [
-                new path_parameter(
-                    name: 'required',
-                    type: param::INT,
-                ),
-            ],
-        );
-
-        $request = $this->get_request_for_routed_route($route, '/example/123');
-
-        $validator = \core\di::get(request_validator::class);
-        $this->expectException(\coding_exception::class);
-        $this->expectExceptionMessageMatches('/Route.*has 0 arguments.* 1 pathtypes./');
-        $result = $validator->validate_request($request);
-
-        $this->assertInstanceOf(
-            ServerRequestInterface::class,
-            $result,
-        );
-    }
-
     /**
      * When a pathtype fails to validate, it will result in an HttpNotFoundException.
      */

lib/tests/router/schema/parameters/path_parameter_test.php

@@ -64,9 +64,18 @@ final class path_parameter_test extends route_testcase {
      */
     public static function is_required_provider(): array {
         return [
+            ['/is/not/found', false],
+            ['/is/not/found/{values}', false],
+            ['/is/not/found/{values:.*}', false],
+            ['/is/not/found/{values:.*?}', false],
             ['/is/required/{value}', true],
+            ['/is/required/{value:.*}', true],
+            ['/is/required/{value:.*?}/example', true],
             ['/is/optional/[{value}]', false],
             ['/is/[optional/[{value}]]', false],
+            ['/is/[optional/[{value:.*}]]', false],
+            ['/is/[optional/[{value:.*?}/example]]', false],
+            ['/is/required/{value}[/example]', true],
         ];
     }
 

lib/tests/router/schema/specification_test.php

@@ -139,13 +139,25 @@ final class specification_test extends route_testcase {
         $this->assertObjectHasProperty('/core/example/path/with/{option}', $schema->paths);
     }
 
-    public function test_add_path_with_options(): void {
+    /**
+     * Test add_path with optional parameters.
+     *
+     * @param string $component
+     * @param string $path
+     * @param array $expectedpaths
+     * @dataProvider add_path_with_options_provider
+     */
+    public function test_add_path_with_options(
+        string $component,
+        string $path,
+        array $expectedpaths,
+    ): void {
         $spec = new specification();
 
         $spec->add_path(
-            'core',
+            $component,
             new route(
-                path: '/example/path/with[/{optional}][/{extras}]',
+                path: $path,
                 pathtypes: [
                     new path_parameter(name: 'optional', type: param::INT),
                     new path_parameter(name: 'extras', type: param::INT),
@@ -154,9 +166,44 @@ final class specification_test extends route_testcase {
         );
 
         $schema = $spec->get_schema();
-        $this->assertObjectHasProperty('/core/example/path/with', $schema->paths);
-        $this->assertObjectHasProperty('/core/example/path/with/{optional}', $schema->paths);
-        $this->assertObjectHasProperty('/core/example/path/with/{optional}/{extras}', $schema->paths);
+        foreach ($expectedpaths as $expectedpath) {
+            $this->assertObjectHasProperty($expectedpath, $schema->paths);
+        }
+    }
+
+    /**
+     * Data provider for add_path with optional parameters.
+     *
+     * @return \Iterator
+     */
+    public static function add_path_with_options_provider(): \Iterator {
+        yield 'With options' => [
+            'component' => 'core',
+            'path' => '/example/path/with[/{optional}][/{extras}]',
+            'expectedpaths' => [
+                '/core/example/path/with',
+                '/core/example/path/with/{optional}',
+                '/core/example/path/with/{optional}/{extras}',
+            ],
+        ];
+        yield 'With greedy unlimited options' => [
+            'component' => 'core',
+            'path' => '/example/path/with[/{optional}][/{extras:.*}]',
+            'expectedpaths' => [
+                '/core/example/path/with',
+                '/core/example/path/with/{optional}',
+                '/core/example/path/with/{optional}/{extras}',
+            ],
+        ];
+        yield 'With non-greedy unlimited options' => [
+            'component' => 'core',
+            'path' => '/example/path/with[/{optional}][/{extras:.*?}]',
+            'expectedpaths' => [
+                '/core/example/path/with',
+                '/core/example/path/with/{optional}',
+                '/core/example/path/with/{optional}/{extras}',
+            ],
+        ];
     }
 
     public function test_add_parameter(): void {