Confusing `isLast()` behavior when looping pages#229

This isn’t a bug in Kirby (that’s why it’s here and not an issue in GitHub), it’s just something confusing.

I have a group of 50 pages, all unlisted. I also have a Pages field in which I’ve selected 3 of those 50 pages.

When I call $collection = $page->myField()->toPages(), I correctly get a collection of 3 pages. Then, I want to loop those pages:

foreach ($collection as $entry) {
    if ($entry->isLast()) { ... }
}

However, isLast() doesn’t check if the entry is last in the collection, it checks if it’s the last child of those 50, which it isn’t.

In my opinion, isLast() should check if it’s the last entry in the collection, while a new isLastChild() method should check if it’s the last child of its parent.

You can already pass the collection as argument to isLast()/ìsFirst()` etc.:

foreach ($collection as $entry) {
    if ($entry->isLast($collection)) { ... }
}

It’s also nontrivial for a Page object to infer which collection it’s “in” or being “used in”. For example, a single instance of a Page model object may exist in many collections at the same moment (they’re not cloned on each “add”).

My point was that in structures, you have isLast(), which checks if the item is last in the collection. This means that isLast() has a different behavior in Page objects, compared to StructureObject objects.

I don’t necessarily think there should be a functional change. I think renaming Page object’s isLast and isFirst to isLastChild and isFirstChild will be enough to remove the confusion.

I am a bit irritated by the statement that StructureObjects should have a different behavior. Pages as StructureObject both use the some method defined in the HasSiblings trait: https://github.com/getkirby/kirby/blob/master/src/Cms/HasSiblings.php#L161

I understand that this can be confusing, it’s an effect of the immutable nature of a lot of our objects, but we won’t be able to change that in Kirby 3. So for v3, as Neil has mentioned, the page object won’t be able to infer in which collection contet it is used in.

As for your suggestion, I don’t agree. It would be for one a big breaking change, which doesn’t seem worth it to me. And it would be a lot more confusing for using it with a collection parameter. ->isLastChild($collection) would be a very contradicting statement.