Strange behavior when deleting pages with children

[texnixe]

Description

When deleting a parent page with children, in some situations the page is not deleted, but the page content file renamed to default.txt, while the previously default numbered children are all prepended with 1_.

Expected behavior
Page should be deleted

Screenshots

To reproduce

1. In a 5.1.2 Starterkit, open the Panel
2. From the Dashboard, open the Photography page, then from the Settings menu, select Delete
3. Enter the page title and confirm with the Delete button
4. You are redirected to the Dashboard, but you will see that the page is still there.
5. Inspect the filesystem to the effects described above.
6. Note that if you delete the Notes page afterwards, this does not happen. Also, when you delete the Photography page again now, it will be gone.

Your setup

Kirby Version
Fresh 5.1.2 Starterkit on MacOS with Herd as local dev server

Console output

Your system (please complete the following information)

• Device:
• OS:
• Browser:
• Version:

Additional context

See also: https://forum.getkirby.com/t/publish-pages-in-batch/34785/

[afbora]

I can confirm that this issue is valid for 5.0.0+ and works as expected in 4.8.0.

[afbora]

First sight of the issue

I've found related line where issue starts. The source of the problem is the ->resortSiblingsAfterUnlisting() call in PageActions::delete() method. My guess is that this method may still be storing old siblings when it runs.

kirby/src/Cms/PageActions.php

Lines 592 to 597 in 0f916be

	if (
	$old->isListed() === true &&
	$old->blueprint()->num() === 'default'
	) {
	$old->resortSiblingsAfterUnlisting();
	}

Note: The reason it works on the notes page is because notes use a num date. This only affects those with the default num.

Possible solution

I think we should delete the actually files and folders from the disc before re-sorting.

// delete all public media files
Dir::remove($old->mediaRoot());

// delete the content folder for this page
Dir::remove($old->root());

// re-sorting stuff

[distantnative]

@afbora but that's what supposedly the line above ($old->versions()->delete()) should do. So something might be broken there?

Cause we cannot add those file system method to page actions as with v5 now the page might use a different storage than plaintext and then it would crash.

[afbora]

@distantnative Yes, it's possible. As I said, if the resortSiblingsAfterUnlisting method isn't called, there's no problem. Not sure but I guess this means there's still old data in the $old object's cache.

[afbora]

@distantnative I think I've found a more detailed clue to the issue.

This issue only occurs if the following conditions are met at the same time: (This corresponds exactly to the photography page in our starterkit)

• Blueprint num is default
• A page has children
• Children has files
• Children are listed

In the PageActions::delete() method, $old->files() returns empty, except for the first object in the children loop.

My guess is:

• When the parent is deleted, it enters the children loop and begins deleting its children one by one.
• It deletes the first child and re-sort the chilren.
• The paths of the listed children will change with resorting.
• When it reaches the second child, the ->files() method now returns empty because the path has changed
• So the child page can't be deleted since it has still files

[bastianallgeier]

@afbora I think your guesses are perfectly on point. Before we switched to the Versions API and also use this to delete versions individually, we simply removed the entire Page folder. This is why we never really ran into issues before. The resorting happened as well, but it didn't matter because we wiped the folder anyway.

As a quick fix, we could introduce a second paramter for the delete method and call it $resortSiblings or something. This could then be set to false for children. But it feels a bit nasty.

[bastianallgeier]

@afbora could you test my fix on branch fix/7615-resort-siblings It works like a charm for me. But again, it is a bit ugly to have another argument here. @distantnative @lukasbestle @texnixe how do you feel about another argument for the delete method?

$page->delete(force: true, resortSiblings: false)