Merge pull request #11741 from cakephp/issue-11740-paginator-sort

Fix Paginator unnecessarily modifying "sort" option.

src/Datasource/Paginator.php

@@ -209,7 +209,7 @@ class Paginator implements PaginatorInterface
             'prevPage' => $page > 1,
             'nextPage' => $count > ($page * $limit),
             'pageCount' => $pageCount,
-            'sort' => key($order),
+            'sort' => $options['sort'],
             'direction' => current($order),
             'limit' => $defaults['limit'] != $limit ? $limit : null,
             'sortDefault' => $sortDefault,
@@ -349,8 +349,10 @@ class Paginator implements PaginatorInterface
                 $direction = 'asc';
             }
             $options['order'] = [$options['sort'] => $direction];
+        } else {
+            $options['sort'] = null;
         }
-        unset($options['sort'], $options['direction']);
+        unset($options['direction']);
 
         if (empty($options['order'])) {
             $options['order'] = [];
@@ -365,6 +367,7 @@ class Paginator implements PaginatorInterface
             $inWhitelist = in_array($field, $options['sortWhitelist'], true);
             if (!$inWhitelist) {
                 $options['order'] = [];
+                $options['sort'] = null;
 
                 return $options;
             }

tests/TestCase/Controller/Component/PaginatorComponentTest.php

@@ -192,6 +192,7 @@ class PaginatorComponentTest extends TestCase
                 'page' => 1,
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
         $this->Paginator->paginate($table, $settings);
     }
@@ -327,6 +328,7 @@ class PaginatorComponentTest extends TestCase
                 'order' => ['PaginatorPosts.id' => 'DESC'],
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
 
         $this->Paginator->paginate($table, $settings);
@@ -359,6 +361,7 @@ class PaginatorComponentTest extends TestCase
                 'order' => ['PaginatorPosts.id' => 'DESC'],
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
 
         $this->Paginator->paginate($table, $settings);
@@ -668,6 +671,7 @@ class PaginatorComponentTest extends TestCase
                 'order' => ['PaginatorPosts.id' => 'asc'],
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => 'id',
             ]);
 
         $this->request->query = [
@@ -676,7 +680,7 @@ class PaginatorComponentTest extends TestCase
             'direction' => 'herp'
         ];
         $this->Paginator->paginate($table);
-        $this->assertEquals('PaginatorPosts.id', $this->request->params['paging']['PaginatorPosts']['sort']);
+        $this->assertEquals('id', $this->request->params['paging']['PaginatorPosts']['sort']);
         $this->assertEquals('asc', $this->request->params['paging']['PaginatorPosts']['direction']);
     }
 
@@ -1236,6 +1240,7 @@ class PaginatorComponentTest extends TestCase
                 'order' => [],
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
         $this->Paginator->paginate($table, $settings);
     }
@@ -1270,6 +1275,7 @@ class PaginatorComponentTest extends TestCase
                 'page' => 1,
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
         $this->Paginator->paginate($query, $settings);
     }
@@ -1330,6 +1336,7 @@ class PaginatorComponentTest extends TestCase
                 'page' => 1,
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
         $this->Paginator->paginate($query, $settings);
     }

tests/TestCase/Datasource/PaginatorTest.php

@@ -129,6 +129,7 @@ class PaginatorTest extends TestCase
                 'page' => 1,
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
         $this->Paginator->paginate($table, $params, $settings);
     }
@@ -240,6 +241,7 @@ class PaginatorTest extends TestCase
                 'order' => ['PaginatorPosts.id' => 'DESC'],
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
 
         $this->Paginator->paginate($table, [], $settings);
@@ -272,6 +274,7 @@ class PaginatorTest extends TestCase
                 'order' => ['PaginatorPosts.id' => 'DESC'],
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
 
         $this->Paginator->paginate($table, [], $settings);
@@ -595,6 +598,7 @@ class PaginatorTest extends TestCase
                 'order' => ['PaginatorPosts.id' => 'asc'],
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => 'id',
             ]);
 
         $params = [
@@ -604,7 +608,7 @@ class PaginatorTest extends TestCase
         ];
         $this->Paginator->paginate($table, $params);
         $pagingParams = $this->Paginator->getPagingParams();
-        $this->assertEquals('PaginatorPosts.id', $pagingParams['PaginatorPosts']['sort']);
+        $this->assertEquals('id', $pagingParams['PaginatorPosts']['sort']);
         $this->assertEquals('asc', $pagingParams['PaginatorPosts']['direction']);
     }
 
@@ -630,6 +634,45 @@ class PaginatorTest extends TestCase
     }
 
     /**
+     * testValidateSortRetainsOriginalSortValue
+     *
+     * @return void
+     * @see https://github.com/cakephp/cakephp/issues/11740
+     */
+    public function testValidateSortRetainsOriginalSortValue()
+    {
+        $table = $this->_getMockPosts(['query']);
+        $query = $this->_getMockFindQuery();
+
+        $table->expects($this->once())
+            ->method('query')
+            ->will($this->returnValue($query));
+
+        $query->expects($this->once())->method('applyOptions')
+            ->with([
+                'limit' => 20,
+                'page' => 1,
+                'order' => ['PaginatorPosts.id' => 'asc'],
+                'whitelist' => ['limit', 'sort', 'page', 'direction'],
+                'scope' => null,
+                'sortWhitelist' => ['id'],
+                'sort' => 'id',
+            ]);
+
+        $params = [
+            'page' => 1,
+            'sort' => 'id',
+            'direction' => 'herp'
+        ];
+        $options = [
+            'sortWhitelist' => ['id']
+        ];
+        $this->Paginator->paginate($table, $params, $options);
+        $pagingParams = $this->Paginator->getPagingParams();
+        $this->assertEquals('id', $pagingParams['PaginatorPosts']['sort']);
+    }
+
+    /**
      * Test that a really large page number gets clamped to the max page size.
      *
      * @return void
@@ -1118,6 +1161,7 @@ class PaginatorTest extends TestCase
                 'order' => [],
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
         $this->Paginator->paginate($table, [], $settings);
     }
@@ -1152,6 +1196,7 @@ class PaginatorTest extends TestCase
                 'page' => 1,
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
         $this->Paginator->paginate($query, $params, $settings);
     }
@@ -1212,6 +1257,7 @@ class PaginatorTest extends TestCase
                 'page' => 1,
                 'whitelist' => ['limit', 'sort', 'page', 'direction'],
                 'scope' => null,
+                'sort' => null,
             ]);
         $this->Paginator->paginate($query, $params, $settings);
     }