getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

isFirst()/isLast() methods doesn't work properly when use listed(), published(), unlisted() methods #1947

Closed afbora closed 5 years ago

afbora commented 5 years ago

Describe the bug The isFirst() and isLast() method doesn't work if I use methods such as listed(), published(), unlisted(), and only one element when retrieving child elements of a collection.

First Sample

$children = new Pages([
    new Page(['slug' => 'a']),
    new Page(['slug' => 'b', 'num' => 1]),
    new Page(['slug' => 'c'])
]);

$parent = new Page(['slug' => 'a', 'children' => $children->toArray()]);

foreach ($parent->children()->listed() as $page) {
    if ($page->isFirst()) {
        echo "First page is " . $page->slug() . ":" . $page->status() . "\n";
    }
    else if ($page->isLast()) {
        echo "Last page is " . $page->slug() . ":" . $page->status() . "\n";
    }
    else {
        echo "Page is " . $page->slug() . ":" . $page->status() . "\n";
    }
}

You will see: "Page is b:listed"

Expected behavior Output should be: "First page is b:listed"

Second Sample

Another example with two listed items

$children = new Pages([
    new Page(['slug' => 'a']),
    new Page(['slug' => 'b', 'num' => 1]),
    new Page(['slug' => 'c', 'num' => 2])
]);

You will see if you use children()->listed() method

Page is b:listed Last page is c:listed

Expected output:

First page is b:listed Last page is c:listed

Third Sample

Lets look out last sample with four items and two listed in middle of collection:

$children = new Pages([
    new Page(['slug' => 'a']),
    new Page(['slug' => 'b', 'num' => 1]),
    new Page(['slug' => 'c', 'num' => 2]),
    new Page(['slug' => 'd'])
]);

You will see if you use children()->listed() method

Page is b:listed Page is c:listed

Expected output for this sample:

First page is b:listed Last page is c:listed

I guess this issue about siblingsCollection() method: https://github.com/getkirby/kirby/blob/80b69380e672565a849037232c9951d1e32774c8/src/Cms/PageSiblings.php#L178-L185

Kirby Version 3.2.2

lukasbestle commented 5 years ago

Thanks as always for the detailed bug report!

It's indeed the method you linked to. Unfortunately this issue is very hard to fix as the model (here: page object) doesn't know what collection it was returned from. The only way we could fix this is to create clones of the model objects every time a collection is created. Each clone would then store the actual collection it came from. This would however be a massive performance hit and the RAM usage would increase by a lot because of all the additional objects PHP would need to keep in memory.

You can get around this issue like so:

$children = new Pages([
    new Page(['slug' => 'a']),
    new Page(['slug' => 'b', 'num' => 1]),
    new Page(['slug' => 'c'])
]);

$parent = new Page(['slug' => 'a', 'children' => $children->toArray()]);

$collection = $parent->children()->listed();
foreach ($collection as $page) {
    if ($collection->first()->is($page)) {
        echo "First page is " . $page->slug() . ":" . $page->status() . "\n";
    }
    else if ($collection->last()->is($page)) {
        echo "Last page is " . $page->slug() . ":" . $page->status() . "\n";
    }
    else {
        echo "Page is " . $page->slug() . ":" . $page->status() . "\n";
    }
}

Not entirely as clean, sure, but a lot better performance-wise.

Closing this issue because we can't really fix it. If you have an idea, feel free to reopen. :)

afbora commented 5 years ago

I understand very well 👍

I think $collection->first()->is($page) and $collection->last()->is($page) ideas are great!

I love that you always come up with a solution ❤️

Also if the new extension type that collection methods comes, I think we will solve it in a shorter way :))

I implemented to pagesMethod as your idea for now.

Thank you for your interest


Several pieces of code to help:

Kirby::plugin('my/pagesMethods', [
    'pagesMethods' => [
        'isReallyFirst' => function (\Kirby\Cms\Page $page) {
            return $this->first()->is($page);
        },
        'isReallyLast' => function (\Kirby\Cms\Page $page) {
            return $this->last()->is($page);
        }
    ]
]);

Usage:

foreach ($pages as $page) {
    if ($pages->isReallyFirst($page)) {
        echo "First page";
    }
}