Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deleting multiple pages from a collection fails with manually sorted pages #2323

Closed
texnixe opened this issue Nov 24, 2019 · 13 comments
Closed

Deleting multiple pages from a collection fails with manually sorted pages #2323

texnixe opened this issue Nov 24, 2019 · 13 comments
Assignees
Milestone

Comments

@texnixe
Copy link
Contributor

@texnixe texnixe commented Nov 24, 2019

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
#1681

@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Nov 24, 2019

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

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Nov 25, 2019

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

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Nov 25, 2019

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

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Nov 25, 2019

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

@texnixe

This comment has been minimized.

Copy link
Contributor Author

@texnixe texnixe commented Nov 25, 2019

That works! 🎉

@distantnative

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Nov 25, 2019

@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!

@afbora afbora self-assigned this Nov 25, 2019
@afbora afbora added this to the 3.3.2 milestone Nov 25, 2019
afbora added a commit that referenced this issue Nov 25, 2019
@afbora afbora mentioned this issue Nov 25, 2019
4 of 4 tasks complete
@texnixe

This comment has been minimized.

Copy link
Contributor Author

@texnixe texnixe commented Nov 25, 2019

@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

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Nov 25, 2019

@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

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Nov 25, 2019

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

This comment has been minimized.

Copy link
Contributor

@distantnative distantnative commented Nov 25, 2019

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

@lukasbestle

This comment has been minimized.

Copy link
Contributor

@lukasbestle lukasbestle commented Nov 25, 2019

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 added a commit that referenced this issue Dec 2, 2019
bastianallgeier added a commit that referenced this issue Dec 9, 2019
bastianallgeier added a commit that referenced this issue Dec 9, 2019
@afbora

This comment has been minimized.

Copy link
Contributor

@afbora afbora commented Dec 9, 2019

@afbora afbora closed this Dec 9, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

@bastianallgeier bastianallgeier commented Dec 9, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.