Deleting multiple pages from a collection fails with manually sorted pages

[texnixe]

Describe the bug
When trying to delete a set of pages, e.g. with this code in a fresh Starterkit, e.g. on the home template, this is no problem with the subpages of the notes page:

$kirby->impersonate('kirby');
foreach (page('notes')->children() as $child) {
  $child->delete();
}
$kirby->impersonate();

Using the same code, but this time trying to delete the children of the photography page, results in only one item to be deleted per page reload:

$kirby->impersonate('kirby');
foreach (page('photography')->children() as $child) {
  $child->delete();
}
$kirby->impersonate();

As soon as I remove the manual sorting numbers from the photography page's children, those subpages all get deleted as expected as well.

Expected behavior
All pages should be deleted, not matter what their sorting scheme.

Screenshots

Kirby Version
Tested with 3.3.0

Additional context
https://github.com/getkirby/kirby/issues/1681

[afbora]

https://github.com/getkirby/kirby/blob/master/src/Cms/PageActions.php#L703-L710 https://github.com/getkirby/kirby/blob/master/src/Cms/PageActions.php#L54

I tracked down and found that this issue is about changeNum() method. When changeNum() method run for siblings after deleted the first item, all siblings moved to new directory (/2_xxx to /1_xxx), so finding and deleting content directory failed for siblings.

I think @lukasbestle can help with that.

[afbora]

I guess, found the solution with update the root path of the old page with the root path of the moved new page to use fly actions on old page in loop.

$oldPage->setRoot($newPage->root());

What do you think @lukasbestle @distantnative @bastianallgeier ?

Whole changeNum() method https://github.com/getkirby/kirby/blob/master/src/Cms/PageActions.php#L34-L65

public function changeNum(int $num = null)
{
    if ($this->isDraft() === true) {
        throw new LogicException('Drafts cannot change their sorting number');
    }

    // don't run the action if everything stayed the same
    if ($this->num() === $num) {
        return $this;
    }

    return $this->commit('changeNum', [$this, $num], function ($oldPage, $num) {
        $newPage = $oldPage->clone([
            'num'     => $num,
            'dirname' => null,
            'root'    => null
        ]);

        // actually move the page on disk
        if ($oldPage->exists() === true) {
            if (Dir::move($oldPage->root(), $newPage->root()) === true) {
                // Updates the root path of the old page with the root path
                // of the moved new page to use fly actions on old page in loop
                $oldPage->setRoot($newPage->root());
            } else {
                throw new LogicException('The page directory cannot be moved');
            }
        }

        // overwrite the child in the parent page
        $newPage
            ->parentModel()
            ->children()
            ->set($newPage->id(), $newPage);

        return $newPage;
    });
}

[distantnative]

I think the comment is wrong :D it’s not keeping the old page root, it’s updating the old page with the new page’s root

Other than that it sounds like a need fix. Haven’t had the chance to try it though.

[afbora]

Ahh, my bad English :) You pronounced it correctly and i just updated 😇

[texnixe]

That works! 🎉

[distantnative]

@afbora Since it seem to work for Sonja, could you please open a PR with the solution, then we can also run the whole test suite on it :) Great!

[texnixe]

@afbora The original poster found an issue with reversed numbering: https://forum.getkirby.com/t/deleting-multiple-pages-from-frontend/16424/5?u=texnixe

[afbora]

@texnixe I tracked down that reversed numbering and this issue is independent of the batch sorted pages deletion. It's a good thing we caught this bug. I'il try to figure it out.

[afbora]

I found the issue of reverse numbering and this is about PageActions::resortSiblingsAfterUnlisting() method.

Reverse order issue on line:

$parent->children = $siblings->sortBy('num', 'desc');

Works properly without sortBy() method:

$parent->children = $siblings;

or with sortBy('num', 'asc'):

$parent->children = $siblings->sortBy('num', 'asc');

Actually, I have no idea why we're doing a descending sort 🤔 Looks like it was written by mistake instead of asc 😄

https://github.com/getkirby/kirby/blob/master/src/Cms/PageActions.php#L709

Do you have an idea? @lukasbestle @distantnative

[distantnative]

Honestly no. But @bastianallgeier wrote that line, so he should have the final call.

[lukasbestle]

To be honest I'm not sure if it's a good idea to mutate an immutable object. Maybe it's the only way, but if we can find a way that works without doing that, that would probably be better. But that's also something for @bastianallgeier to decide.

[afbora]

✅

[bastianallgeier]

Mutating the old page is not really elegant, but I see no other way to update the object in the same loop. This stuff is really tricky :/ This fix could lead to a potential issue in hooks if you expect the old page object to still have the old root. But I think this scenario described in the ticket is a lot more likely than the hook scenario.