Fix optional routing keys with braced placeholders

Optional parameters were not working correctly with braced parameters.
While non-core parameters can be optional the controller + action
parameters should never be null as that will result in invalid URLs
generated or incorrect results that only contain the action parameter.

I've removed a few invalid test cases that were providing keys as both
in the template and as a default. This has not worked for a long time
and these tests were relying on the now invalid behavior.

Fixes #13997

src/Routing/Route/Route.php

@@ -753,6 +753,14 @@ class Route
         }
         $url += $hostOptions;
 
+        // Ensure controller/action keys are not null.
+        if (
+            (isset($keyNames['controller']) && !isset($url['controller'])) ||
+            (isset($keyNames['action']) && !isset($url['action']))
+        ) {
+            return false;
+        }
+
         return $this->_writeUrl($url, $pass, $query);
     }
 
@@ -803,12 +811,10 @@ class Route
 
         $search = $replace = [];
         foreach ($this->keys as $key) {
-            $string = null;
-            if (isset($params[$key])) {
-                $string = $params[$key];
-            } elseif (strpos($out, $key) != strlen($out) - strlen($key)) {
-                $key .= '/';
+            if (!array_key_exists($key, $params)) {
+                throw new InvalidArgumentException("Missing required route key `{$key}`");
             }
+            $string = $params[$key];
             if ($this->braceKeys) {
                 $search[] = "{{$key}}";
             } else {

tests/TestCase/Routing/Route/RouteTest.php

@@ -1532,6 +1532,34 @@ class RouteTest extends TestCase
     }
 
     /**
+     * Test match handles optional keys
+     *
+     * @return void
+     */
+    public function testMatchNullValueOptionalKey()
+    {
+        $route = new Route('/path/:optional/fixed');
+        $this->assertSame('/path/fixed', $route->match(['optional' => null]));
+
+        $route = new Route('/path/{optional}/fixed');
+        $this->assertSame('/path/fixed', $route->match(['optional' => null]));
+    }
+
+    /**
+     * Test matching fails on required keys (controller/action)
+     *
+     * @return void
+     */
+    public function testMatchControllerRequiredKeys()
+    {
+        $route = new Route('/:controller/:action', ['action' => 'test']);
+        $this->assertFalse($route->match(['controller' => null]));
+
+        $route = new Route('/test/:action', ['controller' => 'thing']);
+        $this->assertFalse($route->match(['action' => null]));
+    }
+
+    /**
      * Test restructuring args with pass key
      *
      * @return void

tests/TestCase/Routing/RouterTest.php

@@ -66,7 +66,7 @@ class RouterTest extends TestCase
     {
         $this->assertRegExp('/^http(s)?:\/\//', Router::url('/', true));
         $this->assertRegExp('/^http(s)?:\/\//', Router::url(null, true));
-        $this->assertRegExp('/^http(s)?:\/\//', Router::url(['_full' => true]));
+        $this->assertRegExp('/^http(s)?:\/\//', Router::url(['controller' => 'test', '_full' => true]));
     }
 
     /**
@@ -149,25 +149,14 @@ class RouterTest extends TestCase
     }
 
     /**
-     * testRouteDefaultParams method
-     *
-     * @return void
-     */
-    public function testRouteDefaultParams()
-    {
-        Router::connect('/:controller', ['controller' => 'posts']);
-        $this->assertEquals(Router::url(['action' => 'index']), '/');
-    }
-
-    /**
      * testRouteExists method
      *
      * @return void
      */
     public function testRouteExists()
     {
-        Router::connect('/:controller/:action', ['controller' => 'posts']);
-        $this->assertTrue(Router::routeExists(['action' => 'view']));
+        Router::connect('/posts/:action', ['controller' => 'posts']);
+        $this->assertTrue(Router::routeExists(['controller' => 'posts', 'action' => 'view']));
 
         $this->assertFalse(Router::routeExists(['action' => 'view', 'controller' => 'users', 'plugin' => 'test']));
     }