Make request property lazy loaded & fix most sorting issues.

Make the request object access happen only when it is required instead of during the constructor. Remove automagic detection of associated fields and virtual fields (which don't exist yet). If you need to sort on associated fields, you will need to use the whitelist features. CakePHP will only check the primary table being pagintaed for columns. This keeps the magic to a minimum and still affords prototyping.
cakephp · Nov 21, 2013 · 2292838 · 2292838
1 parent 7dd9bea
commit 2292838
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 59 deletions.
diff --git a/Cake/Controller/Component/PaginatorComponent.php b/Cake/Controller/Component/PaginatorComponent.php
@@ -31,13 +31,6 @@
  */
 class PaginatorComponent extends Component {
 
-/**
- * The current request instance.
- *
- * @var Cake\Network\Request
- */
-	public $request;
-
 /**
  * Default pagination settings.
  *
@@ -67,18 +60,6 @@ class PaginatorComponent extends Component {
 		'limit', 'sort', 'page', 'direction'
 	);
 
-/**
- * Constructor
- *
- * @param ComponentRegistry $collection A ComponentRegistry this component can use to lazy load its components
- * @param array $settings Array of configuration settings.
- */
-	public function __construct(ComponentRegistry $collection, $settings = []) {
-		$settings = array_merge($this->_defaultConfig, (array)$settings);
-		$this->request = $collection->getController()->request;
-		parent::__construct($collection, $settings);
-	}
-
 /**
  * Handles automatic pagination of model records.
  *
@@ -106,7 +87,7 @@ public function __construct(ComponentRegistry $collection, $settings = []) {
  *    'Articles' => [
  *      'limit' => 20,
  *      'maxLimit' => 100
- *    [,
+ *    ],
  *    'Comments' => [ ... ]
  *  ];
  *  $results = $paginator->paginate($table, $settings);
@@ -142,9 +123,6 @@ public function paginate($object, $settings = array(), $whitelist = array()) {
 		$alias = $object->alias();
 
 		$options = $this->mergeOptions($alias, $settings);
-
-		// TODO perhaps move this until after the query has been created.
-		// Then we could look at the fields in the query.
 		$options = $this->validateSort($object, $options, $whitelist);
 		$options = $this->checkLimit($options);
 
@@ -181,7 +159,6 @@ public function paginate($object, $settings = array(), $whitelist = array()) {
 		$query = $object->find($type, array_merge($parameters, $extra));
 
 		// TODO Validate sort and apply them here.
-
 		$results = $query->execute();
 		$numResults = count($results);
 
@@ -192,7 +169,8 @@ public function paginate($object, $settings = array(), $whitelist = array()) {
 			$count = 0;
 		} else {
 			$parameters = compact('conditions');
-			$count = $object->find($type, array_merge($parameters, $extra))->count();
+			$query = $object->find($type, array_merge($parameters, $extra));
+			$count = $query->count();
 		}
 
 		$pageCount = intval(ceil($count / $limit));
@@ -202,6 +180,8 @@ public function paginate($object, $settings = array(), $whitelist = array()) {
 			throw new Error\NotFoundException();
 		}
 
+		$request = $this->_registry->getController()->request;
+
 		if (!is_array($order)) {
 			$order = (array)$order;
 		}
@@ -219,11 +199,11 @@ public function paginate($object, $settings = array(), $whitelist = array()) {
 			'limit' => $defaults['limit'] != $options['limit'] ? $options['limit'] : null,
 		);
 
-		if (!isset($this->request['paging'])) {
-			$this->request['paging'] = array();
+		if (!isset($request['paging'])) {
+			$request['paging'] = array();
 		}
-		$this->request['paging'] = array_merge(
-			(array)$this->request['paging'],
+		$request['paging'] = array_merge(
+			(array)$request['paging'],
 			array($alias => $paging)
 		);
 		return $results;
@@ -247,8 +227,8 @@ public function paginate($object, $settings = array(), $whitelist = array()) {
  */
 	public function mergeOptions($alias, $settings) {
 		$defaults = $this->getDefaults($alias, $settings);
-		$request = $this->request->query;
-		$request = array_intersect_key($request, array_flip($this->whitelist));
+		$request = $this->_registry->getController()->request;
+		$request = array_intersect_key($request->query, array_flip($this->whitelist));
 		return array_merge($defaults, $request);
 	}
 
@@ -302,26 +282,28 @@ public function validateSort(Table $object, array $options, array $whitelist = a
 			}
 			$options['order'] = array($options['sort'] => $direction);
 		}
+		unset($options['sort'], $options['direction']);
 
-		if (empty($options['order']) || (isset($options['order']) && is_array($options['order']))) {
-			$options['order'] = null;
-			return $options;
+		if (empty($options['order'])) {
+			$options['order'] = [];
+		}
+		if (!is_array($options['order'])) {
+			$options['order'] = (array)$options['order'];
 		}
 
 		if (!empty($whitelist)) {
 			$field = key($options['order']);
 			$inWhitelist = in_array($field, $whitelist, true);
 			if (!$inWhitelist) {
-				$options['order'] = null;
+				$options['order'] = [];
 			}
 			return $options;
 		}
 
-		if (!empty($options['order']) && is_array($options['order'])) {
+		if (is_array($options['order'])) {
 			$tableAlias = $object->alias();
-			$order = array();
+			$order = [];
 
-			// TODO Remove associated field checks and rely on the whitelist.
 			foreach ($options['order'] as $key => $value) {
 				$field = $key;
 				$alias = $tableAlias;
@@ -332,16 +314,10 @@ public function validateSort(Table $object, array $options, array $whitelist = a
 
 				if ($correctAlias && $object->hasField($field)) {
 					$order[$tableAlias . '.' . $field] = $value;
-				} elseif ($correctAlias && $object->hasField($key, true)) {
-					$order[$field] = $value;
-				} elseif (isset($object->{$alias}) && $object->{$alias}->hasField($field, true)) {
-					// TODO fix associated sorting.
-					$order[$alias . '.' . $field] = $value;
 				}
 			}
 			$options['order'] = $order;
 		}
-		unset($options['sort'], $options['direction']);
 
 		return $options;
 	}

diff --git a/Cake/Test/TestCase/Controller/Component/PaginatorComponentTest.php b/Cake/Test/TestCase/Controller/Component/PaginatorComponentTest.php
@@ -89,11 +89,7 @@ public function testPageParamCasting() {
 			->will($this->returnValue('Posts'));
 
 		$query = $this->_getMockFindQuery();
-		$this->Post->expects($this->at(1))
-			->method('find')
-			->will($this->returnValue($query));
-
-		$this->Post->expects($this->at(2))
+		$this->Post->expects($this->any())
 			->method('find')
 			->will($this->returnValue($query));
 
@@ -178,7 +174,7 @@ public function testPaginateCustomFinder() {
  */
 	public function testDefaultPaginateParams() {
 		$settings = array(
-			'order' => 'PaginatorPosts.id DESC',
+			'order' => ['PaginatorPosts.id' => 'DESC'],
 			'maxLimit' => 10,
 		);
 
@@ -192,7 +188,7 @@ public function testDefaultPaginateParams() {
 				'fields' => null,
 				'limit' => 10,
 				'page' => 1,
-				'order' => 'PaginatorPosts.id DESC'
+				'order' => ['PaginatorPosts.id' => 'DESC']
 			])
 			->will($this->returnValue($query));
 
@@ -496,7 +492,7 @@ public function testValidateSortWhitelistFailure() {
 		$options = array('sort' => 'body', 'direction' => 'asc');
 		$result = $this->Paginator->validateSort($model, $options, array('title', 'id'));
 
-		$this->assertNull($result['order']);
+		$this->assertEquals([], $result['order']);
 	}
 
 /**
@@ -615,12 +611,7 @@ public function testValidateSortNoSort() {
 
 		$options = array('direction' => 'asc');
 		$result = $this->Paginator->validateSort($model, $options, array('title', 'id'));
-		$this->assertFalse(isset($result['order']));
-
-		$options = array('order' => 'invalid desc');
-		$result = $this->Paginator->validateSort($model, $options, array('title', 'id'));
-
-		$this->assertEquals($options['order'], $result['order']);
+		$this->assertEquals([], $result['order']);
 	}
 
 /**
@@ -877,8 +868,9 @@ protected function _getMockFindQuery() {
 			->will($this->returnValue($results));
 
 		$query->expects($this->any())
-			->method('total')
+			->method('count')
 			->will($this->returnValue(2));
+
 		return $query;
 	}