ENH Zero limit no results #10652

maxime-rainville · 2023-01-18T11:11:32Z

Parent issue:

maxime-rainville · 2023-01-18T11:16:24Z

src/ORM/ArrayList.php

+        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.");
        }


We weren't validating for this before. If you give a negative length, methods like array slice will want to start from the end.

While this might make sense for an array, most DB don't think like that. Seems more logical to throw an exception.

maxime-rainville · 2023-01-18T11:17:37Z

src/ORM/ArrayList.php

+
+        if ($length === 0) {
+            // If we set the limit to 0, we return an empty list.
+            $list->items = [];


If you let array slice below do its thing, you'll end up with the same results. Seems clearer to do it explicitly.

maxime-rainville · 2023-01-18T11:19:47Z

src/ORM/DataList.php

+        if ($length !== null && $length < 0) {
+            throw new InvalidArgumentException('$length can not be a negative value');
+        }
+
+        if ($offset < 0) {
+            throw new InvalidArgumentException('$offset can not be a negative value');
+        }
+


Exceptions would be thrown any way by the queries below eventually. Seems better to just do it now.

maxime-rainville · 2023-01-18T11:20:42Z

src/ORM/DataQuery.php

     */
-    public function limit($limit, $offset = 0)
+    public function limit(?int $limit, int $offset = 0): static


Not necessary to meet the ACs, but seems logical to change this while we are at it.

maxime-rainville · 2023-01-18T11:25:31Z

src/ORM/Queries/SQLSelect.php

@@ -249,7 +249,7 @@ public function setLimit($limit, $offset = 0)
            throw new InvalidArgumentException("SQLSelect::setLimit() only takes positive values");
        }

-        if (is_numeric($limit) && ($limit || $offset)) {


The second part of this IF statement would fail if 0,0 was provided. This would effectively prevent you from setting a zero limit.

setLimit() actually accept strings and array. This seems a bit weird but refactoring this is probably beyond the scope of this card.

maxime-rainville · 2023-01-18T11:27:29Z

tests/php/ORM/ArrayListTest.php

@@ -117,34 +117,69 @@ function ($item) use (&$count, $test) {
        $this->assertEquals($list->Count(), $count);
    }

-    public function testLimit()


It's a bit weird how limited the test coverage was for such a critical method.

maxime-rainville · 2023-01-18T11:27:42Z

tests/php/ORM/DataListTest.php

        $check = $list->limit(false);
-        $this->assertEquals(3, $check->count());
+        $this->assertEquals(0, $check->count());


Not done with this part yet.

maxime-rainville · 2023-01-18T11:29:56Z

src/ORM/Limitable.php

     */
-    public function limit($limit, $offset = 0);
+    public function limit(?int $length, int $offset = 0): static;


I've decided to rename the first parameter $length. My instinct is that this makes clearer that there's multiple possible parameters here.

GuySartorelli · 2023-01-18T20:07:16Z

src/ORM/ArrayList.php

-            $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 = sizeof($this->items);


What's the advantage of sizeof over count? count seems to be the more standard way to get the number of items in a Countable or array.

Apparently it's exactly the same thing. https://www.php.net/manual/en/function.sizeof.php

For some reason, I've always defaulted to sizeof. Don't have any preference either way.

GuySartorelli · 2023-01-18T20:10:51Z

src/ORM/DataList.php

+            throw new InvalidArgumentException('$length can not be a negative value');
+        }
+
+        if ($offset < 0) {
+            throw new InvalidArgumentException('$offset can not be a negative value');


Make these the same as the text used for the ArrayList exceptions

GuySartorelli · 2023-01-18T20:11:40Z

src/ORM/DataQuery.php

     */
-    public function limit($limit, $offset = 0)
+    public function limit(?int $limit, int $offset = 0): static
    {
        $this->query->setLimit($limit, $offset);


No exceptions on this one?

This method is really just an alias for calling $query->setLimit(). So it seems rational to just let setLimit do all the work.

GuySartorelli · 2023-01-18T20:13:20Z

src/ORM/Queries/SQLSelect.php

@@ -249,7 +249,7 @@ public function setLimit($limit, $offset = 0)
            throw new InvalidArgumentException("SQLSelect::setLimit() only takes positive values");


Make this the same as the other exceptions

SQLSelect::setLimit() does substantially different things than the other limit methods. It accepts all sort of weird values like arrays and strings that the other don't.

We could change this error message to something more specific ... but I think it needs to be different than the other methods.

maxime-rainville · 2023-01-23T10:55:14Z

src/ORM/Limitable.php

     */
-    public function limit($limit, $offset = 0);
+    public function limit(?int $length, int $offset = 0): Limitable;


I wanted to return a static here ... but list decorator doesn't actually return a version of itself, so I couldn't do that.

maxime-rainville · 2023-01-25T03:02:46Z

Ran the full sink against my PR branch and got all greens: https://github.com/maxime-rainville/recipe-kitchen-sink/actions/runs/4002316914

maxime-rainville · 2023-01-25T04:10:05Z

src/ORM/ListDecorator.php

@@ -16,21 +16,16 @@ abstract class ListDecorator extends ViewableData implements SS_List, Sortable,
    /**
     * @var SS_List
     */
-    protected $list;
+    protected SS_List&Sortable&Filterable&Limitable $list;


Basically, every method on the class assumes that we will return a List that implements all these interfaces.

maxime-rainville · 2023-01-25T04:32:20Z

Added a test for the tweak to SQLSelect ... I also got a for of the sink running all the CI to make sure no other module needs updating https://github.com/maxime-rainville/recipe-kitchen-sink/actions/runs/4002850477

src/ORM/DataList.php

src/ORM/ArrayList.php

tests/php/ORM/SQLSelectTest.php

src/ORM/DataList.php

tests/php/ORM/SQLSelectTest.php

maxime-rainville · 2023-01-26T21:04:35Z

All feedback answered.

sabina-talipova

Looks good for me

sabina-talipova · 2023-01-26T21:51:40Z

Passed tests locally. Approved and merged.

maxime-rainville force-pushed the pulls/5/limit-0 branch from 78e2146 to 9b0ab46 Compare January 18, 2023 11:13

maxime-rainville commented Jan 18, 2023

View reviewed changes

GuySartorelli reviewed Jan 18, 2023

View reviewed changes

GuySartorelli requested changes Jan 18, 2023

View reviewed changes

maxime-rainville force-pushed the pulls/5/limit-0 branch from 8c607a7 to 9c4302f Compare January 23, 2023 08:44

maxime-rainville commented Jan 23, 2023

View reviewed changes

maxime-rainville mentioned this pull request Jan 25, 2023

CMS 5 ORM limit(0) and limit(null) #10604

Closed

3 tasks

maxime-rainville commented Jan 25, 2023

View reviewed changes

maxime-rainville marked this pull request as ready for review January 25, 2023 04:31

sabina-talipova suggested changes Jan 25, 2023

View reviewed changes

src/ORM/DataList.php Show resolved Hide resolved

src/ORM/ArrayList.php Show resolved Hide resolved

tests/php/ORM/SQLSelectTest.php Outdated Show resolved Hide resolved

maxime-rainville commented Jan 26, 2023

View reviewed changes

src/ORM/DataList.php Show resolved Hide resolved

tests/php/ORM/SQLSelectTest.php Outdated Show resolved Hide resolved

Maxime Rainville added 3 commits January 27, 2023 10:00

ENH Make limit method return no results when zero is provided

d1e0e1e

MNT Add more test coverage to DataList::limit()

5090f49

MNT Add test to SQLSelect for Zero limit

fc6c45d

maxime-rainville force-pushed the pulls/5/limit-0 branch from 5f5db9c to fc6c45d Compare January 26, 2023 21:01

maxime-rainville requested review from GuySartorelli and sabina-talipova January 26, 2023 21:04

sabina-talipova approved these changes Jan 26, 2023

View reviewed changes

sabina-talipova merged commit 0fee1aa into silverstripe:5 Jan 26, 2023

sabina-talipova deleted the pulls/5/limit-0 branch January 26, 2023 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH Zero limit no results #10652

ENH Zero limit no results #10652

maxime-rainville commented Jan 18, 2023

maxime-rainville Jan 18, 2023 •

edited

Loading

maxime-rainville Jan 18, 2023

maxime-rainville Jan 18, 2023

maxime-rainville Jan 18, 2023

maxime-rainville Jan 18, 2023

maxime-rainville Jan 18, 2023

maxime-rainville Jan 18, 2023

maxime-rainville Jan 18, 2023

maxime-rainville Jan 18, 2023

GuySartorelli Jan 18, 2023

maxime-rainville Jan 23, 2023

GuySartorelli Jan 18, 2023

GuySartorelli Jan 18, 2023

maxime-rainville Jan 23, 2023 •

edited

Loading

GuySartorelli Jan 18, 2023

maxime-rainville Jan 23, 2023

maxime-rainville Jan 23, 2023

maxime-rainville commented Jan 25, 2023

maxime-rainville Jan 25, 2023 •

edited

Loading

maxime-rainville commented Jan 25, 2023

maxime-rainville commented Jan 26, 2023

sabina-talipova left a comment

sabina-talipova commented Jan 26, 2023

		@@ -249,7 +249,7 @@ public function setLimit($limit, $offset = 0)
		throw new InvalidArgumentException("SQLSelect::setLimit() only takes positive values");

ENH Zero limit no results #10652

ENH Zero limit no results #10652

Conversation

maxime-rainville commented Jan 18, 2023

maxime-rainville Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxime-rainville commented Jan 25, 2023

maxime-rainville Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

maxime-rainville commented Jan 25, 2023

maxime-rainville commented Jan 26, 2023

sabina-talipova left a comment

Choose a reason for hiding this comment

sabina-talipova commented Jan 26, 2023

maxime-rainville Jan 18, 2023 •

edited

Loading

maxime-rainville Jan 23, 2023 •

edited

Loading

maxime-rainville Jan 25, 2023 •

edited

Loading