Avoiding possible key collisions in the Paginator helper

It was possiblle before to pass keys in the query string that collided
with parts of the url params in the router

src/Routing/Route/Route.php

@@ -441,7 +441,9 @@ class Route
         if (!isset($hostOptions['_base']) && isset($context['_base'])) {
             $hostOptions['_base'] = $context['_base'];
         }
-        unset($url['_host'], $url['_scheme'], $url['_port'], $url['_base']);
+
+        $query = !empty($url['?']) ? array_filter((array)$url['?'], 'strlen') : [];
+        unset($url['_host'], $url['_scheme'], $url['_port'], $url['_base'], $url['?']);
 
         // Move extension into the hostOptions so its not part of
         // reverse matches.
@@ -484,8 +486,6 @@ class Route
         }
 
         $pass = [];
-        $query = [];
-
         foreach ($url as $key => $value) {
             // keys that exist in the defaults and have different values is a match failure.
             $defaultExists = array_key_exists($key, $defaults);

src/Routing/Router.php

@@ -577,12 +577,6 @@ class Router
                 $full = true;
                 unset($url['_full']);
             }
-            // Compatibility for older versions.
-            if (isset($url['?'])) {
-                $q = $url['?'];
-                unset($url['?']);
-                $url = array_merge($url, $q);
-            }
             if (isset($url['#'])) {
                 $frag = '#' . $url['#'];
                 unset($url['#']);

src/View/Helper/PaginatorHelper.php

@@ -98,9 +98,14 @@ class PaginatorHelper extends Helper
     {
         parent::__construct($View, $config);
 
+        $query = $this->request->query;
+        if (isset($query['page']) && $query['page'] == 1) {
+            unset($query['page']);
+        }
+
         $this->config(
             'options.url',
-            array_merge($this->request->params['pass'], $this->request->query)
+            array_merge($this->request->params['pass'], ['?' => $query])
         );
     }
 

tests/TestCase/Routing/RouterTest.php

@@ -2972,6 +2972,21 @@ class RouterTest extends TestCase
     }
 
     /**
+     * Test generation of routes with collisions between the query string
+     * and other url params
+     *
+     * @return void
+     */
+    public function testUrlWithCollidingQueryString()
+    {
+        Router::connect('/:controller/:action/:id');
+
+        $query = ['controller' => 'Foo', 'action' => 'bar', 'id' => 100];
+        $result = Router::url(['controller' => 'posts', 'action' => 'view', 'id' => 1, '?' => $query]);
+        $this->assertEquals('/posts/view/1?controller=Foo&action=bar&id=100', $result);
+    }
+
+    /**
      * Connect some fallback routes for testing router behavior.
      *
      * @return void

tests/TestCase/View/Helper/PaginatorHelperTest.php

@@ -814,19 +814,19 @@ class PaginatorHelperTest extends TestCase
         $result = $this->Paginator->numbers();
         $expected = [
             ['li' => ['class' => 'active']], '<a href=""', '1', '/a', '/li',
-            ['li' => []], ['a' => ['href' => '/articles/index/2?page=2&amp;foo=bar&amp;x=y&amp;num=0']], '2', '/a', '/li',
-            ['li' => []], ['a' => ['href' => '/articles/index/2?page=3&amp;foo=bar&amp;x=y&amp;num=0']], '3', '/a', '/li',
-            ['li' => []], ['a' => ['href' => '/articles/index/2?page=4&amp;foo=bar&amp;x=y&amp;num=0']], '4', '/a', '/li',
-            ['li' => []], ['a' => ['href' => '/articles/index/2?page=5&amp;foo=bar&amp;x=y&amp;num=0']], '5', '/a', '/li',
-            ['li' => []], ['a' => ['href' => '/articles/index/2?page=6&amp;foo=bar&amp;x=y&amp;num=0']], '6', '/a', '/li',
-            ['li' => []], ['a' => ['href' => '/articles/index/2?page=7&amp;foo=bar&amp;x=y&amp;num=0']], '7', '/a', '/li',
+            ['li' => []], ['a' => ['href' => '/articles/index/2?foo=bar&amp;x=y&amp;num=0&amp;page=2']], '2', '/a', '/li',
+            ['li' => []], ['a' => ['href' => '/articles/index/2?foo=bar&amp;x=y&amp;num=0&amp;page=3']], '3', '/a', '/li',
+            ['li' => []], ['a' => ['href' => '/articles/index/2?foo=bar&amp;x=y&amp;num=0&amp;page=4']], '4', '/a', '/li',
+            ['li' => []], ['a' => ['href' => '/articles/index/2?foo=bar&amp;x=y&amp;num=0&amp;page=5']], '5', '/a', '/li',
+            ['li' => []], ['a' => ['href' => '/articles/index/2?foo=bar&amp;x=y&amp;num=0&amp;page=6']], '6', '/a', '/li',
+            ['li' => []], ['a' => ['href' => '/articles/index/2?foo=bar&amp;x=y&amp;num=0&amp;page=7']], '7', '/a', '/li',
         ];
         $this->assertHtml($expected, $result);
 
         $result = $this->Paginator->next('Next');
         $expected = [
             'li' => ['class' => 'next'],
-            'a' => ['href' => '/articles/index/2?page=2&amp;foo=bar&amp;x=y&amp;num=0', 'rel' => 'next'],
+            'a' => ['href' => '/articles/index/2?foo=bar&amp;x=y&amp;num=0&amp;page=2', 'rel' => 'next'],
             'Next',
             '/a',
             '/li'

tests/TestCase/View/Helper/UrlHelperTest.php

@@ -97,7 +97,7 @@ class UrlHelperTest extends TestCase
             'controller' => 'posts', 'action' => 'index', 'page' => '1',
             '?' => ['one' => 'value', 'two' => 'value', 'three' => 'purple']
         ]);
-        $this->assertEquals("/posts/index?page=1&one=value&two=value&three=purple", $result);
+        $this->assertEquals("/posts/index?one=value&two=value&three=purple&page=1", $result);
     }
 
     /**