getkirby / kirby

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

Can not save file field when a child page has the same slug than the parent page #2933

Closed gerricom closed 3 years ago

gerricom commented 3 years ago

This bug got me some kind of crazy because I couldn't find out what was going wrong here... but after several hours I stumbled upon this:

To Reproduce
You have a root page with the slug "test". The blueprint of this page has a file field to select images. You then create a child page that also has the slug "test".

Now you won't be able to select an image on the root page's file field.

When you look into the corresponding file, the panel only writes a [ ] into that field.

Expected behavior
The selected file is saved.

Kirby Version
Tested with Kirby 3.3 to latest 3.4

afbora commented 3 years ago

I think it would be correct to make an update like this on following line 🤔 https://github.com/getkirby/kirby/blob/3.4.4/src/Cms/App.php#L557-L559

if (is_a($parent, 'Kirby\Cms\User') === true || is_a($parent, 'Kirby\Cms\Page') === true) {
    return $parent->file($filename);
}
lukasbestle commented 3 years ago

@afbora Thanks for digging into this, that's a very good lead for further investigation.

@bastianallgeier I need your guidance:

The issue is either that $app->file() handles the $parent argument incorrectly or that the files field calls it incorrectly: When $app->file() gets called with a $parent argument and the $path is not at the root of the site, $app->file() first tries to find a page inside of $parent.

Example: If you call it with $app->file('test/my-file.png', page('test'));, it will try to find a page test inside the $parent and then it will try to find the file my-file.png inside test/test. Only if the page test/test doesn't exist, it will try again without the $parent.

The files field calls the method exactly like that, which leads to this issue.

So the question is: What was the $parent parameter for $app->file() meant for originally? I have a hard time understanding why the method first tries to find a file inside the $parent directory structure, but then falls back to a global search anyway. Maybe there's only a specific undocumented use-case for $parent where this behavior makes sense, but in general I'd expect that it would either search strictly inside $parent or strictly globally, but not in both ways (which leads to weird bugs like this).

In case we can't solve this well in $app->file(), my suggestion is to leave out the $parent argument completely in this line so that the method can do a global search on its own.

lukasbestle commented 3 years ago

Ah, wait: The files field only seems to receive full paths (including the page ID test/) sometimes, but sometimes it receives only the filenames without the page ID (in which case the $parent is required). I think the bug is somewhere here.

In any case, we should also take a look at the logic in $app->file() as that's also a source for the bug.

texnixe commented 3 years ago

Just for reference: https://forum.getkirby.com/t/files-field-error-when-subpage-has-same-slug-as-parent/21336

afbora commented 3 years ago

To confirm, this issue still continue on 3.6.0-alpha.2.

lukasbestle commented 3 years ago

@bastianallgeier Ping

bastianallgeier commented 3 years ago

I cannot reproduce this anymore with beta.4 We might have fixed this with our files route pattern issue.

bastianallgeier commented 3 years ago

I'll close it. If anyone can still reproduce it, feel free to reopen.