Fix optional parameters in url generator (#2246)

* Fix route collection getting wrong path when optional parameters present, add unit tests

framework/core/src/Http/RouteCollection.php

@@ -11,6 +11,7 @@ namespace Flarum\Http;
 
 use FastRoute\DataGenerator;
 use FastRoute\RouteParser;
+use Illuminate\Support\Arr;
 
 class RouteCollection
 {
@@ -88,10 +89,25 @@ class RouteCollection
     public function getPath($name, array $parameters = [])
     {
         if (isset($this->reverse[$name])) {
-            $parts = $this->reverse[$name][0];
+            $maxMatches = 0;
-            array_walk($parts, [$this, 'fixPathPart'], $parameters);
+            $matchingParts = $this->reverse[$name][0];
 
-            return '/'.ltrim(implode('', $parts), '/');
+            // For a given route name, we want to choose the option that best matches the given parameters.
+            // Each routing option is an array of parts. Each part is either a constant string
+            // (which we don't care about here), or an array where the first element is the parameter name
+            // and the second element is a regex into which the parameter value is inserted, if the parameter matches.
+            foreach ($this->reverse[$name] as $parts) {
+                foreach ($parts as $i => $part) {
+                    if (is_array($part) && Arr::exists($parameters, $part[0]) && $i > $maxMatches) {
+                        $maxMatches = $i;
+                        $matchingParts = $parts;
+                    }
+                }
+            }
+
+            array_walk($matchingParts, [$this, 'fixPathPart'], $parameters);
+
+            return '/'.ltrim(implode('', $matchingParts), '/');
         }
 
         throw new \RuntimeException("Route $name not found");

framework/core/tests/unit/Foundation/RouteCollectionTest.php

@@ -0,0 +1,76 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tests\unit\Foundation;
+
+use Flarum\Http\RouteCollection;
+use Flarum\Tests\unit\TestCase;
+use RuntimeException;
+
+class RouteCollectionTest extends TestCase
+{
+    /** @test */
+    public function it_errors_when_nonexistent_route_requested()
+    {
+        $collection = new RouteCollection();
+
+        $this->expectException(RuntimeException::class);
+
+        $collection->getPath('nonexistent');
+    }
+
+    /** @test */
+    public function it_properly_processes_a_simple_route_with_no_parameters()
+    {
+        $collection = new RouteCollection();
+        // We can use anything for the handler since we're only testing getPath
+        $collection->addRoute('GET', '/custom/route', 'custom', '');
+
+        $this->assertEquals('/custom/route', $collection->getPath('custom'));
+    }
+
+    /** @test */
+    public function it_properly_processes_a_route_with_all_parameters_required()
+    {
+        $collection = new RouteCollection();
+        // We can use anything for the handler since we're only testing getPath
+        $collection->addRoute('GET', '/custom/{route}/{has}/{parameters}', 'custom', '');
+
+        $this->assertEquals('/custom/something/something_else/anything_else', $collection->getPath('custom', [
+            'route' => 'something',
+            'has' => 'something_else',
+            'parameters' => 'anything_else'
+        ]));
+    }
+
+    /** @test */
+    public function it_works_if_optional_parameters_are_missing()
+    {
+        $collection = new RouteCollection();
+        // We can use anything for the handler since we're only testing getPath
+        $collection->addRoute('GET', '/custom/{route}[/{has}]', 'custom', '');
+
+        $this->assertEquals('/custom/something', $collection->getPath('custom', [
+            'route' => 'something'
+        ]));
+    }
+
+    /** @test */
+    public function it_works_with_optional_parameters()
+    {
+        $collection = new RouteCollection();
+        // We can use anything for the handler since we're only testing getPath
+        $collection->addRoute('GET', '/custom/{route}[/{has}]', 'custom', '');
+
+        $this->assertEquals('/custom/something/something_else', $collection->getPath('custom', [
+            'route' => 'something',
+            'has' => 'something_else'
+        ]));
+    }
+}