diff --git a/src/ORM/ArrayList.php b/src/ORM/ArrayList.php index 3d3c98910..ae45dac2d 100644 --- a/src/ORM/ArrayList.php +++ b/src/ORM/ArrayList.php @@ -171,23 +171,29 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L return $result; } - /** - * Get a sub-range of this dataobjectset as an array - * Pass null to "remove the limit" - this is for consistency with DataList::limit(null) which itself will - * call SQLSelect::setLimit(null) - * - * @param int|null $length - * @param int $offset - * @return static - */ - public function limit($length, $offset = 0) + public function limit(?int $length, int $offset = 0): static { - if (!$length) { - $length = count($this->items ?? []); + if ($length === null) { + // If we unset the limit, we set the length to the size of the list. We still want the offset to be picked up + $length = count($this->items); + } + + if ($length < 0) { + throw new InvalidArgumentException("\$length can not be negative. $length was provided."); + } + + if ($offset < 0) { + throw new InvalidArgumentException("\$offset can not be negative. $offset was provided."); } $list = clone $this; - $list->items = array_slice($this->items ?? [], $offset ?? 0, $length); + + if ($length === 0) { + // If we set the limit to 0, we return an empty list. + $list->items = []; + } else { + $list->items = array_slice($this->items ?? [], $offset ?? 0, $length); + } return $list; } diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index 8fe1911c1..158039445 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -287,15 +287,19 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li /** * Return a new DataList instance with the records returned in this query * restricted by a limit clause. - * - * @param int $limit - * @param int $offset - * @return static */ - public function limit($limit, $offset = 0) + public function limit(?int $length, int $offset = 0): static { - return $this->alterDataQuery(function (DataQuery $query) use ($limit, $offset) { - $query->limit($limit, $offset); + if ($length !== null && $length < 0) { + throw new InvalidArgumentException("\$length can not be negative. $length was provided."); + } + + if ($offset < 0) { + throw new InvalidArgumentException("\$offset can not be negative. $offset was provided."); + } + + return $this->alterDataQuery(function (DataQuery $query) use ($length, $offset) { + $query->limit($length, $offset); }); } diff --git a/src/ORM/DataQuery.php b/src/ORM/DataQuery.php index 06acea92a..18951ecae 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -752,12 +752,8 @@ class DataQuery /** * Set the limit of this query. - * - * @param int $limit - * @param int $offset - * @return $this */ - public function limit($limit, $offset = 0) + public function limit(?int $limit, int $offset = 0): static { $this->query->setLimit($limit, $offset); return $this; diff --git a/src/ORM/Limitable.php b/src/ORM/Limitable.php index 120b104ba..f30083272 100644 --- a/src/ORM/Limitable.php +++ b/src/ORM/Limitable.php @@ -20,9 +20,9 @@ interface Limitable extends SS_List * If $offset is specified, then that many records at the beginning of the list will be skipped. * This matches the behaviour of the SQL LIMIT clause. * - * @param int $limit - * @param int $offset - * @return static + * If `$length` is null, then no limit is applied. If `$length` is 0, then an empty list is returned. + * + * @throws InvalidArgumentException if $length or offset are negative */ - public function limit($limit, $offset = 0); + public function limit(?int $length, int $offset = 0): Limitable; } diff --git a/src/ORM/ListDecorator.php b/src/ORM/ListDecorator.php index 7a3226dd4..5a682964a 100644 --- a/src/ORM/ListDecorator.php +++ b/src/ORM/ListDecorator.php @@ -17,21 +17,16 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable, /** * @var SS_List */ - protected $list; + protected SS_List&Sortable&Filterable&Limitable $list; - public function __construct(SS_List $list) + public function __construct(SS_List&Sortable&Filterable&Limitable $list) { $this->setList($list); parent::__construct(); } - /** - * Returns the list this decorator wraps around. - * - * @return SS_List - */ - public function getList() + public function getList(): SS_List&Sortable&Filterable&Limitable { return $this->list; } @@ -44,7 +39,7 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable, * * @return SS_List */ - public function setList($list) + public function setList(SS_List&Sortable&Filterable&Limitable $list): self { $this->list = $list; $this->failover = $this->list; @@ -246,9 +241,9 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable, return $output; } - public function limit($limit, $offset = 0) + public function limit(?int $length, int $offset = 0): SS_List&Sortable&Filterable&Limitable { - return $this->list->limit($limit, $offset); + return $this->list->limit($length, $offset); } /** diff --git a/src/ORM/Queries/SQLSelect.php b/src/ORM/Queries/SQLSelect.php index 9b2f3c440..fa38005aa 100644 --- a/src/ORM/Queries/SQLSelect.php +++ b/src/ORM/Queries/SQLSelect.php @@ -249,7 +249,7 @@ class SQLSelect extends SQLConditionalExpression throw new InvalidArgumentException("SQLSelect::setLimit() only takes positive values"); } - if (is_numeric($limit) && ($limit || $offset)) { + if (is_numeric($limit)) { $this->limit = [ 'start' => (int)$offset, 'limit' => (int)$limit, diff --git a/src/ORM/Search/SearchContext.php b/src/ORM/Search/SearchContext.php index ca374ff69..3dae7058b 100644 --- a/src/ORM/Search/SearchContext.php +++ b/src/ORM/Search/SearchContext.php @@ -133,7 +133,7 @@ class SearchContext * for example "Comments__Name" instead of the filter name "Comments.Name". * @param array|bool|string $sort Database column to sort on. * Falls back to {@link DataObject::$default_sort} if not provided. - * @param array|bool|string $limit + * @param array|null|string $limit * @param DataList $existingQuery * @return DataList * @throws Exception @@ -168,7 +168,7 @@ class SearchContext * Prepare the query to begin searching * * @param array|bool|string $sort Database column to sort on. - * @param array|bool|string $limit + * @param array|null|string $limit */ private function prepareQuery($sort, $limit, ?DataList $existingQuery): DataList { @@ -320,11 +320,11 @@ class SearchContext * * @param array $searchParams * @param array|bool|string $sort - * @param array|bool|string $limit + * @param array|null|string $limit * @return DataList * @throws Exception */ - public function getResults($searchParams, $sort = false, $limit = false) + public function getResults($searchParams, $sort = false, $limit = null) { $searchParams = array_filter((array)$searchParams, [$this, 'clearEmptySearchFields']); diff --git a/tests/php/ORM/ArrayListTest.php b/tests/php/ORM/ArrayListTest.php index 0a8040c19..8a13ee084 100644 --- a/tests/php/ORM/ArrayListTest.php +++ b/tests/php/ORM/ArrayListTest.php @@ -117,34 +117,69 @@ class ArrayListTest extends SapphireTest $this->assertEquals($list->Count(), $count); } - public function testLimit() + public function limitDataProvider(): array { - $list = new ArrayList( - [ + $all = [ ['Key' => 1], ['Key' => 2], ['Key' => 3] ]; + list($one, $two, $three) = $all; + + return [ + 'smaller limit' => [2, 0, [$one, $two]], + 'limit equal to array' => [3, 0, $all], + 'limit bigger than array' => [4, 0, $all], + 'zero limit' => [0, 0, []], + 'false limit' => [0, 0, []], + 'null limit' => [null, 0, $all], + + 'smaller limit with offset' => [1, 1, [$two]], + 'limit to end with offset' => [2, 1, [$two, $three]], + 'bigger limit with offset' => [3, 1, [$two, $three]], + 'offset beyond end of list' => [4, 3, []], + 'zero limit with offset' => [0, 1, []], + 'null limit with offset' => [null, 2, [$three]], + + ]; + } + + /** + * @dataProvider limitDataProvider + */ + public function testLimit($length, $offset, array $expected) + { + $data = [ ['Key' => 1], ['Key' => 2], ['Key' => 3] - ] + ]; + $list = new ArrayList($data); + $this->assertEquals( + $list->limit($length, $offset)->toArray(), + $expected ); $this->assertEquals( - $list->limit(2, 1)->toArray(), - [ - ['Key' => 2], ['Key' => 3] - ] + $list->toArray(), + $data, + 'limit is immutable and does not affect the original list' ); } - public function testLimitNull() + public function testLimitNegative() { + $this->expectException(\InvalidArgumentException::class, 'Calling limit with a negative length throws exception'); $list = new ArrayList( [ ['Key' => 1], ['Key' => 2], ['Key' => 3] ] ); - $this->assertEquals( - $list->limit(null, 0)->toArray(), + $list->limit(-1); + } + + public function testLimitNegativeOffset() + { + $this->expectException(\InvalidArgumentException::class, 'Calling limit with a negative offset throws exception'); + $list = new ArrayList( [ ['Key' => 1], ['Key' => 2], ['Key' => 3] ] ); + $list->limit(1, -1); } public function testAddRemove() diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index ebb13003a..94c12b3e3 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -134,8 +134,11 @@ class DataListTest extends SapphireTest $check = $list->limit(1, 1); $this->assertEquals(1, $check->count()); + $check = $list->limit(0); + $this->assertEquals(0, $check->count()); + $check = $list->limit(false); - $this->assertEquals(3, $check->count()); + $this->assertEquals(0, $check->count()); $check = $list->limit(null); $this->assertEquals(3, $check->count()); diff --git a/tests/php/ORM/ListDecoratorTest.php b/tests/php/ORM/ListDecoratorTest.php index 213857aa1..e0c011a90 100644 --- a/tests/php/ORM/ListDecoratorTest.php +++ b/tests/php/ORM/ListDecoratorTest.php @@ -243,8 +243,9 @@ class ListDecoratorTest extends SapphireTest public function testLimit() { - $this->list->expects($this->once())->method('limit')->with(5, 3)->willReturn('mock'); - $this->assertSame('mock', $this->decorator->limit(5, 3)); + $mockLimitedList = $this->list; + $this->list->expects($this->once())->method('limit')->with(5, 3)->willReturn($mockLimitedList); + $this->assertSame($mockLimitedList, $this->decorator->limit(5, 3)); } public function testTotalItems()