When deleting entry descendants, don't move descendants to the root but the end of the tree.

[jelleroorda]

trafficstars

This prevents entries from being moved to the root of a tree; which will cause exceptions to be thrown.

While testing this, I noticed that the tree at content/trees/collections/pages.yaml seems to have another bug.

Take @robdekort's example, but instead of deleting the 'Examples' page with it's children, you only delete a child page (for example: Contact form). Instead of the child page being deleted from the tree, it gets appended to the end of the tree. The entry however gets deleted, which means in the user interface you do end up with "the right tree".

From what I can see the cause of this lies in the CollectionTreeController. It first creates a valid tree structure, which will append the entry which is about to be deleted to the end of the structure.

This PR does not fix the above mentioned 'bug', or it might not be an issue at all, you will likely know for sure.

It does however fix the fact that deleting a parent with it's child entries tries to move the child entries under the root. Instead now it'll move them to the end of the tree.

Fixes #4453

[isarphilipp]

Nice job @jelleroorda. We ran into that today unknowingly when the customer called upset that his control panel refuses to work :)

[isarphilipp]

After running this PR in production for over a week I can confirm it works very well.

[jelleroorda]

Sure, I'll try to wrap it up today or tomorrow.

[jelleroorda]

So while writing an extra test I encountered a new issue. When simply 'removing' and then 'appending' an entry to the tree, the children of that entry will not be moved along. This might be the intended behavior, but it causes the tree to be 'flattened'.

When you have the following tree:

[
    ['entry' => 'root'],
    ['entry' => '1', 'children' => [
        ['entry' => '2'],
        ['entry' => '3', 'children' => [
            ['entry' => '5'],
            ['entry' => '4'],
        ]],
    ]],
]

When you remove entry 1, the tree should probably morph into this:

[
    ['entry' => 'root'],
    ['entry' => '2'],
    ['entry' => '3', 'children' => [
        ['entry' => '5'],
        ['entry' => '4'],
    ]
],

But right now it changes into this (where you can notice it just disregarded the children, and then appended them in random order again later [see the previously ordered 5, 4 now being ordered 4, 5]):

[
    ['entry' => 'root'],
    ['entry' => '2'],
    ['entry' => '3'],
    ['entry' => '4'],
    ['entry' => '5'],
]

I don't think I can sort this issue out without adding an extra method on the Tree class which will move an artibrary page to the end of the tree, along with its children.

So we would have a method like appendWithChildren(Page $page, ?Page $parent = null) where $parent === null would move them to the end of the tree, instead of under the root. So something along these lines:

    public function appendWithChildren(Page $page, ?Page $parent = null): void
    {
        if ($parent === null) {
            $this->append($page);
        } else {
            $this->appendTo($parent->id(), $page->id());
        }

        foreach ($page->pages()->all() as $childPage) {
            $this->appendWithChildren($childPage, $page);
        }
    }

I've added this suggested method to the tree, if you have a better/alternative approach to this I'd love to hear it.

[robdekort]

Ha. I assumed this was merged by now. Ran into it again today. I imagine this is a pretty confusing bug for end users as they have no clue what they've done wrong or how to fix it.

[isarphilipp]

@jelleroorda great finding. I think what you describe it debatable as it is nowhere clearly defined what should happen.

I would say the original problem you tried to solve is solved. And that is that the user cannot nuke the CP anymore without any chance to fix it by himself.

Maybe we should separate your last question into a separate issue to get the original thing shipped.

What’s your opinion @jasonvarga?

[globalexport]

Hi!

As long as this issue is not solved, I would love to see one of the below workarounds to help avoid the problem.

1. Extend the column filter to show the parent element. This makes it easier to select deletable entries.
2. Proceed with the deletion operation on unproblematic entries and just warn about entries which were not deletable.
3. Fix the delete button which often disappears after the error when the user wants to alter the remaining active selection for a new try. Consequently, this results in a forced page reload to get the delete button back which is quite annoying. While repeating the selection the user does not know which entry was causing the error.
4. Implement multi-select in the tree view.

[jasonvarga]

Sorry for the delay. I've been trying to get this wrapped up. I've merged 3.3 into this and now the test you've added shows the "bug" you mentioned. The entry you try to delete now ends up in the tree.

I'm not sure why it wasn't doing that back in 3.2. But anyway, I'll try to get that sorted out and I'll push it to this branch.

[arthurperton]

Hi @jelleroorda, I was looking to solve #3791 which via #4453 led me to this PR by you 😄. Are you planning on continuing this soon?

[jasonvarga]

@jelleroorda has done a great job, don't need to burden him with any more of it. We can take over it.

[arthurperton]

I am closing this in favour of #6644, which fixes several (hopefully all) issues when deleting an entry with children. You can always reopen this off course if 6644 doesn't fix the case you were solving here.