IsLast doesn’t work when PaginatedList has more than one page

[kinglozzer]

Module version(s) affected

5.3.0

Description

In Silverstripe 4, <% if $First %> or <% if $Last %> worked as expected within the scope of the current page. I.e. they would check whether the current item in the iterator is the first/last item on that page.

In Silverstripe 5, $IsFirst still works this way (it returns true if the item is the first item on that page) but $IsLast doesn’t work at all in my testing. I would expect it to behave the same way as $Last did in Silverstripe 4. The bug only occurs when there is more than one page of items, otherwise it works as expected.

At this stage I’m not sure if it’s related to the switch from $First/$Last to $IsFirst/$IsLast, or related to the change to DataList to use generators. I suspect the latter, a good way to confirm would be to re-test with ArrayList.

How to reproduce

Set up a paginated list (e.g. in your Page class):

public function getPaginatedList()
{
    $items = [
        ['Val' => 1],
        ['Val' => 2],
        ['Val' => 3],
        ['Val' => 4],
        ['Val' => 5],
        ['Val' => 6],
        ['Val' => 7],
        ['Val' => 8],
        ['Val' => 9],
        ['Val' => 10],
    ];
    $list = new ArrayList($items);
    $paginated = new PaginatedList($list);
    $paginated->setPageLength(3);
    $paginated->setCurrentPage(2);
    return $paginated;
}

In a template, iterate through that paginated list:

$PaginatedList.TotalItems
<ul>
    <% loop $PaginatedList %>
    <li>$FirstLast $Val/$TotalItems</li>
    <% end_loop %>
</ul>

The output in CMS 4:

10
    first 4/3
    5/3
    last 6/3

The output in CMS 5:

10
    first 4/10
    5/10
    6/10

Possible Solution

No response

Additional Context

If I add {$FromEnd}, it looks like SSViewer_BasicIteratorSupport is taking into account the total number of items in the un-paginated list, and not applying either the limit or offset:

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

CMS 5

• FIX: Make IsFirst and IsLast work as expected for PaginatedList (fixes #11465) #11471

CMS 6

TBD

[kinglozzer]

Hmm, I also can’t seem to use <% if $Pos = $LastItem %> as a workaround, it never seems to call PaginatedList::LastItem(). I have to use <% if $Pos = $Up.PaginatedList.LastItem %>, so I’m now wondering if this is a wider issue with template scope...

Edit: I haven’t tested <% if $Pos = $LastItem %> in SS4, so that may be behaving as expected. I was just trying to document a workaround!

[GuySartorelli]

Regarding the $LastItem scoping - in CMS 4 you could have accessed the scope of the list itself by using $Up, so you could have used $Up.LastItem. Loops added the list as an implicit scope during iteration. In CMS 5 that implicit scope was removed.

Regarding the bug itself - if it's explicitly just PaginatedList that exhibits this problem, the simplest solution may be to simply add a IsLast and IsFirst() method to that class. That wouldn't work because scoping.

[michalkleiner]

For the functionality, I would expect IsFirst and IsLast to operate on the whole list, not on a page of the list, so with a page size of 6 on a list of 10 items IsLast would only be true for the 10th item, not for the 6th. We don't have a concept of IsLastOnThePage.

[GuySartorelli]

I would expect IsFirst and IsLast to operate on the whole list, not on a page of the list

In a future major release maybe the expected behaviour could be changed - but I think for CMS 5 since there was no intentional change to the behaviour, we need to fix the bug by matching the CMS 4 behaviour.

Unless we explicitly identify the CMS 4 behaviour as buggy.

I think I'd expect it to indicate whether the item is first/last in the current iteration which is what the CMS 4 behaviour did.
Looking at the language used in SSViewer_BasicIteratorSupport which provides this functionality, it's concerned with the "iterator" i.e. the iterator returned by $list->getIterator(), not what's in the list that backs a paginated list.

[GuySartorelli]

Can reproduce both the expected behaviour in CMS 4 and the presumed buggy behaviour in CMS 5. I've added reproduction details to the issue description.

[GuySartorelli]

Looks like specifically the changes in SSViewer_Scope::next() from #10484 are causing this.

The same PR changes the iterators to generators, but the generators themselves aren't what's causing this bug (confirmed by moving the change in that method to CMS 4, which then exhibited this same buggy behaviour)

Specifically the problem is using count($this->item) in lieu of iterator_count($this->itemIterator) - i.e. we're now asking the list how many items it has (and PaginatedList is correctly saying "I have 10 items"), instead of asking the iterator how many items it has. So we end up with the total number of items in the backing list, not the number of items that we're going to be iterating over.

That change was necessary because calling iterator_count() over a generator iterates through the whole generator, so the loop would then be empty when you try to iterate over it (and you can't call rewind() on a generator after iterating over it)

[GuySartorelli]

Implementing this in PaginatedList would fix it:

public function Count(): int
{
    return count($this->list->limit($this->getPageLength(), $this->getPageStart()));
}

Of course that would mean calling count() on a paginated list would return the number of items in the current page which would be a breaking change. Arguably that could be considered correct behaviour though, because:
a) that reflects how many items you'll be iterating over which is probably usually want you want when you call that method
b) the getTotalItems() method (and TotalItems() which for some reason also exists) returns the total number of items across all pages if you do need that.

@silverstripe/core-team Does anyone have any thoughts about this? We definitely couldn't implement the above in a patch release. I'd be hesitant to do it in a minor, as well, but could be persuaded...
Or maybe someone can see another way forward?

[michalkleiner]

The main question is whether Count, IsFirst and IsLast should operate on the current page or the whole list, with an argument a page is also a sublist of the list and if you need to know which is the first and last item of that page to e.g. display different markup, there may not be an easy way of the methods operated on the whole list like they do in CMS 5.

Maybe PaginatedList can have special behaviour of the methods aimed at the current page whereas standard ArrayList would operate on the whole list.

[GuySartorelli]

The main question is whether Count, IsFirst and IsLast should operate on the current page or the whole list, with an argument a page is also a sublist of the list

It could be argued any given "page" in a PaginatedList is just a limit applied to a list - and with any other list after applying a limit the count will reflect that.

The expected behaviour of template loop scope methods does seem to be that they are specific to the iterator being looped over regardless of the underlying list that iterator comes from.

there may not be an easy way of the methods operated on the whole list like they do in CMS 5

To be clear, only Count works like this in CMS 5 on a PaginatedList.
IsFirst returns true for the first item in the page
IsLast will never return true unless all items are on the same page.

[kinglozzer]

I’ve had a look at this today. To summarise the behaviour:

CMS 4 behaviour

• $First and $Last operate on the current page
• $Count returns the un-paginated list count
• $TotalItems returns the un-paginated list count

Current behaviour

• $IsFirst operates on the current page, $IsLast doesn’t work
• $Count returns the un-paginated list count
• $TotalItems returns the un-paginated list count

Desired behaviour

• $IsFirst and $IsLast operate on the current page
• $Count returns the count of items on the current page (this will have to wait for CMS 6)
• $TotalItems returns the un-paginated list count

To determine whether the item is the very first/last item across all pages, you can do <% if $IsFirst && $FirstPage %>. That should probably be IsFirstPage but that’s a different issue 😛.

As above, I think adding PaginatedList::count() is a good move for CMS 6 but too risky for CMS 5. What do you think about adding in a specific case to SSViewer_Scope for PaginatedList to fix this in CMS 5? d6cb465

[sminnee]

The original design of First/Last was for making alternative styling on when rendering the current view. So "last" meant the last item of the current page. I wouldn't suggest changing this.

[GuySartorelli]

The original design of First/Last was for making alternative styling on when rendering the current view. So "last" meant the last item of the current page. I wouldn't suggest changing this.

Thank you for the context

What do you think about adding in a specific case to SSViewer_Scope for PaginatedList to fix this in CMS 5?

Sounds sensible to me, assuming others agree.

[kinglozzer]

What do you think about adding in a specific case to SSViewer_Scope for PaginatedList to fix this in CMS 5?

Sounds sensible to me, assuming others agree.

I’ve opened a PR for review: #11471

[GuySartorelli]

CMS 5 PR merged. @kinglozzer Would you like to tackle the fix for CMS 6 as well?

I think changing the count behaviour is probably appropriate, given:

1. there's a totalItems() method to get the total number of un-paginated items in the underlying list
2. count() should usually (if not always) give the number of items you'll be iterating over if you run for ($loop as $items)
   e.g. consider a DataList or ArrayList that you call limit on - calling count() afterwards will give you the amount after the limit operation, not the total amount in the original list. The fact that PaginatedList currently doesn't call limit earlier than it does is probably just an implementation detail that makes it easier to swap pages at runtime - I think it should probably behave as though it's always limited when calling regular list methods.