Merge pull request #12745 from cakephp/bugfix/pagination

Fix paginator for falling back to default sort.

src/Datasource/Paginator.php

@@ -363,7 +363,12 @@ class Paginator implements PaginatorInterface
             if (!in_array($direction, ['asc', 'desc'])) {
                 $direction = 'asc';
             }
+
             $order = (isset($options['order']) && is_array($options['order'])) ? $options['order'] : [];
+            if ($order && $options['sort'] && strpos($options['sort'], '.') === false) {
+                $order = $this->_removeAliases($order, $object->getAlias());
+            }
+
             $options['order'] = [$options['sort'] => $direction] + $order;
         } else {
             $options['sort'] = null;
@@ -402,6 +407,35 @@ class Paginator implements PaginatorInterface
     }
 
     /**
+     * Remove alias if needed.
+     *
+     * @param array $fields Current fields
+     * @param string $model Current model alias
+     * @return array $fields Unaliased fields where applicable
+     */
+    protected function _removeAliases($fields, $model)
+    {
+        $result = [];
+        foreach ($fields as $field => $sort) {
+            if (strpos($field, '.') === false) {
+                $result[$field] = $sort;
+                continue;
+            }
+
+            list ($alias, $currentField) = explode('.', $field);
+
+            if ($alias === $model) {
+                $result[$currentField] = $sort;
+                continue;
+            }
+
+            $result[$field] = $sort;
+        }
+
+        return $result;
+    }
+
+    /**
      * Prefixes the field with the table alias if possible.
      *
      * @param \Cake\Datasource\RepositoryInterface $object Repository object.

src/View/Helper/PaginatorHelper.php

@@ -537,6 +537,13 @@ class PaginatorHelper extends Helper
         $paging = $this->params($model);
         $paging += ['page' => null, 'sort' => null, 'direction' => null, 'limit' => null];
 
+        if (!empty($paging['sort']) && !empty($options['sort']) && strpos($options['sort'], '.') === false) {
+            $paging['sort'] = $this->_removeAlias($paging['sort'], $model = null);
+        }
+        if (!empty($paging['sortDefault']) && !empty($options['sort']) && strpos($options['sort'], '.') === false) {
+            $paging['sortDefault'] = $this->_removeAlias($paging['sortDefault'], $model);
+        }
+
         $url = [
             'page' => $paging['page'],
             'limit' => $paging['limit'],
@@ -557,9 +564,10 @@ class PaginatorHelper extends Helper
         if (!empty($url['page']) && $url['page'] == 1) {
             $url['page'] = false;
         }
+
         if (isset($paging['sortDefault'], $paging['directionDefault'], $url['sort'], $url['direction']) &&
             $url['sort'] === $paging['sortDefault'] &&
-            $url['direction'] === $paging['directionDefault']
+            strtolower($url['direction']) === strtolower($paging['directionDefault'])
         ) {
             $url['sort'] = $url['direction'] = null;
         }
@@ -588,6 +596,30 @@ class PaginatorHelper extends Helper
     }
 
     /**
+     * Remove alias if needed.
+     *
+     * @param string $field Current field
+     * @param string|null $model Current model alias
+     * @return string Unaliased field if applicable
+     */
+    protected function _removeAlias($field, $model = null)
+    {
+        $currentModel = $model ?: $this->defaultModel();
+
+        if (strpos($field, '.') === false) {
+            return $field;
+        }
+
+        list ($alias, $currentField) = explode('.', $field);
+
+        if ($alias === $currentModel) {
+            return $currentField;
+        }
+
+        return $field;
+    }
+
+    /**
      * Returns true if the given result set is not at the first page
      *
      * @param string|null $model Optional model name. Uses the default if none is specified.

tests/TestCase/Datasource/PaginatorTest.php

@@ -714,6 +714,52 @@ class PaginatorTest extends TestCase
     }
 
     /**
+     * Test that "sort" and "direction" in paging params is properly set based
+     * on initial value of "order" in paging settings.
+     *
+     * @return void
+     */
+    public function testValidateSortAndDirectionAliased()
+    {
+        $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.title' => 'asc'],
+                'whitelist' => ['limit', 'sort', 'page', 'direction'],
+                'sort' => 'title',
+                'scope' => null,
+            ]);
+
+        $options = [
+            'order' => [
+                'Articles.title' => 'desc',
+            ],
+        ];
+        $queryParams = [
+            'page' => 1,
+            'sort' => 'title',
+            'direction' => 'asc'
+        ];
+
+        $this->Paginator->paginate($table, $queryParams, $options);
+        $pagingParams = $this->Paginator->getPagingParams();
+
+        $this->assertEquals('title', $pagingParams['PaginatorPosts']['sort']);
+        $this->assertEquals('asc', $pagingParams['PaginatorPosts']['direction']);
+
+        $this->assertEquals('Articles.title', $pagingParams['PaginatorPosts']['sortDefault']);
+        $this->assertEquals('desc', $pagingParams['PaginatorPosts']['directionDefault']);
+    }
+
+    /**
      * testValidateSortRetainsOriginalSortValue
      *
      * @return void
@@ -935,14 +981,15 @@ class PaginatorTest extends TestCase
     }
 
     /**
+     * @param string $modelAlias Model alias to use.
      * @return \Cake\Datasource\RepositoryInterface|\PHPUnit\Framework\MockObject\MockObject
      */
-    protected function mockAliasHasFieldModel()
+    protected function mockAliasHasFieldModel($modelAlias = 'model')
     {
         $model = $this->getMockRepository();
         $model->expects($this->any())
             ->method('getAlias')
-            ->will($this->returnValue('model'));
+            ->will($this->returnValue($modelAlias));
         $model->expects($this->any())
             ->method('hasField')
             ->will($this->returnValue(true));
@@ -1018,6 +1065,33 @@ class PaginatorTest extends TestCase
     }
 
     /**
+     * Tests that sort query string and model prefixes default match on assoc merging.
+     *
+     * @return void
+     */
+    public function testValidateSortMultipleWithQueryAndAliasedDefault()
+    {
+        $model = $this->mockAliasHasFieldModel();
+
+        $options = [
+            'sort' => 'created',
+            'direction' => 'desc',
+            'order' => [
+                'model.created' => 'asc'
+            ]
+        ];
+        $result = $this->Paginator->validateSort($model, $options);
+
+        $expected = [
+            'sort' => 'created',
+            'order' => [
+                'model.created' => 'desc',
+            ],
+        ];
+        $this->assertEquals($expected, $result);
+    }
+
+    /**
      * Tests that order strings can used by Paginator
      *
      * @return void

tests/TestCase/View/Helper/PaginatorHelperTest.php

@@ -1224,6 +1224,38 @@ class PaginatorHelperTest extends TestCase
     }
 
     /**
+     * Tests that generated default order URL doesn't include sort and direction parameters.
+     *
+     * @return void
+     */
+    public function testDefaultSortRemovedFromUrlWithAliases()
+    {
+        $request = new ServerRequest([
+            'params' => ['controller' => 'articles', 'action' => 'index', 'plugin' => null],
+            'url' => '/articles?sort=title&direction=asc'
+        ]);
+        Router::setRequestInfo($request);
+
+        $this->Paginator->options(['model' => 'Articles']);
+        $this->Paginator->request = $this->Paginator->request->withParam('paging', [
+            'Articles' => [
+                'page' => 1, 'current' => 3, 'count' => 13,
+                'prevPage' => false, 'nextPage' => true, 'pageCount' => 8,
+                'sort' => 'Articles.title', 'direction' => 'asc',
+                'sortDefault' => 'Articles.title', 'directionDefault' => 'desc',
+            ],
+        ]);
+
+        $result = $this->Paginator->sort('title');
+        $expected = [
+            'a' => ['class' => 'asc', 'href' => '/articles/index'],
+            'Title',
+            '/a',
+        ];
+        $this->assertHtml($expected, $result);
+    }
+
+    /**
      * Test the prev() method.
      *
      * @return void