Merge pull request #10652 from creative-commoners/pulls/5/limit-0

ENH Zero limit no results

src/ORM/ArrayList.php

@@ -171,23 +171,29 @@ class ArrayList extends ViewableData implements SS_List, Filterable, Sortable, L
         return $result;
     }
 
-    /**
+    public function limit(?int $length, int $offset = 0): static
-     * 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)
     {
-        if (!$length) {
+        if ($length === null) {
-            $length = count($this->items ?? []);
+            // 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;
     }

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) {
+        if ($length !== null && $length < 0) {
-            $query->limit($limit, $offset);
+            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);
         });
     }
 

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;

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
+     * If `$length` is null, then no limit is applied. If `$length` is 0, then an empty list is returned.
-     * @param int $offset
+     *
-     * @return static
+     * @throws InvalidArgumentException if $length or offset are negative
      */
-    public function limit($limit, $offset = 0);
+    public function limit(?int $length, int $offset = 0): Limitable;
 }

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();
     }
 
-    /**
+    public function getList(): SS_List&Sortable&Filterable&Limitable
-     * Returns the list this decorator wraps around.
-     *
-     * @return SS_List
-     */
-    public function getList()
     {
         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);
     }
 
     /**

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,

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']);
 

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(),
+            $list->toArray(),
-            [
+            $data,
-            ['Key' => 2], ['Key' => 3]
+            '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(-1);
-            $list->limit(null, 0)->toArray(),
+    }
+
+    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()

tests/php/ORM/DataListTest.php

@@ -121,31 +121,43 @@ class DataListTest extends SapphireTest
         $this->assertEquals(['Joe', 'Phil'], $list->limit(2, 1)->column('Name'));
     }
 
-    public function testLimitAndOffset()
+    public function limitDataProvider(): array
+    {
+        return [
+            'no limit' => [null, 0, 3],
+            'smaller limit' => [2, 0, 2],
+            'greater limit' => [4, 0, 3],
+            'one limit' => [1, 0, 1],
+            'zero limit' => [0, 0, 0],
+            'limit and offset' => [1, 1, 1],
+            'false limit equivalent to 0' => [false, 0, 0],
+            'offset only' => [null, 2, 1],
+            'offset greater than list length' => [null, 3, 0],
+            'negative length' => [-1, 0, 0, true],
+            'negative offset' => [0, -1, 0, true],
+        ];
+    }
+
+    /**
+     * @dataProvider limitDataProvider
+     */
+    public function testLimitAndOffset($length, $offset, $expectedCount, $expectException = false)
     {
         $list = TeamComment::get();
-        $check = $list->limit(3);
 
-        $this->assertEquals(3, $check->count());
+        if ($expectException) {
+            $this->expectException(\InvalidArgumentException::class);
+        }
 
-        $check = $list->limit(1);
+        $this->assertCount($expectedCount, $list->limit($length, $offset));
-        $this->assertEquals(1, $check->count());
+        $this->assertCount(
-
+            $expectedCount,
-        $check = $list->limit(1, 1);
+            $list->limit(0, 9999)->limit($length, $offset),
-        $this->assertEquals(1, $check->count());
+            'Follow up limit calls unset previous ones'
-
+        );
-        $check = $list->limit(false);
-        $this->assertEquals(3, $check->count());
-
-        $check = $list->limit(null);
-        $this->assertEquals(3, $check->count());
-
-        $check = $list->limit(null, 2);
-        $this->assertEquals(1, $check->count());
 
         // count()/first()/last() methods may alter limit/offset, so run the query and manually check the count
-        $check = $list->limit(null, 1)->toArray();
+        $this->assertCount($expectedCount, $list->limit($length, $offset)->toArray());
-        $this->assertEquals(2, count($check ?? []));
     }
 
     public function testDistinct()

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');
+        $mockLimitedList = $this->list;
-        $this->assertSame('mock', $this->decorator->limit(5, 3));
+        $this->list->expects($this->once())->method('limit')->with(5, 3)->willReturn($mockLimitedList);
+        $this->assertSame($mockLimitedList, $this->decorator->limit(5, 3));
     }
 
     public function testTotalItems()

tests/php/ORM/SQLSelectTest.php

@@ -173,6 +173,7 @@ class SQLSelectTest extends SapphireTest
         $this->assertSQLEquals("SELECT * FROM MyTable LIMIT 99 OFFSET 97", $query->sql($parameters));
     }
 
+
     public function testSelectWithOrderbyClause()
     {
         $query = new SQLSelect();
@@ -255,6 +256,18 @@ class SQLSelectTest extends SapphireTest
         );
     }
 
+    public function testZeroLimit()
+    {
+        $query = new SQLSelect();
+        $query->setFrom("MyTable");
+        $query->setLimit(0);
+
+        $this->assertSQLEquals(
+            'SELECT * FROM MyTable LIMIT 0',
+            $query->sql($parameters)
+        );
+    }
+
     public function testNegativeLimit()
     {
         $this->expectException(\InvalidArgumentException::class);