kirby icon indicating copy to clipboard operation
kirby copied to clipboard

Deleting file fields makes file fields in the panel seem empty, while they are not

Open trych opened this issue 10 months ago • 5 comments

Description

I ran into an issue with file fields in a live deployed site that seemed very counter intuitive to me.

I had this snippet throw an error, although I safeguarded it with an if statement:

  if ($page->musicmp3()->isNotEmpty()) {
    $audioMusic[] = $page->musicmp3()->toFile()->url();
  }

This threw an error on the audioMusic line. I expected it to not even be able to reach that line if there was no file in the field. Upon further investigation I found out that the content file had a file reference there that was apparently linking to a file that no longer existed, as it must have been deleted earlier.

Now, this makes for some confusing behaviour, as my supposed safeguard isNotEmpty() failed to save me here. It's also confusing that the field in the panel shows me it's empty, when apparently it is not.

Expected behavior
There's two solutions to this issue: A) remove corresponding file references in the content file when files are deleted B) show a "File Not Found" symbol or something like this in the file field that holds a file that no longer exists

I think isNotEmpty() should be consistent with what the users see in the panel.

I haven't tested it, but the same probably goes for file fields and user fields, which is where I realize that Option A is probably really complicated as probably all the website's content files would need to be searched for corresponding content fields.

Having said all that, I would also appreciate a short hint, how I could improve my safeguard here and quickly test if the file reference in the file field actually points at an existing file without throwing an error.

trych avatar Mar 26 '24 15:03 trych

I think the easy workaround would be

if ($file = $page->musicmp3()->toFile()) {
    $audioMusic[] = $fille->url();
}

distantnative avatar Mar 26 '24 15:03 distantnative

Thanks for the super quick workaround, this solves my immediate problem. 🙏

However, I think the issue itself remains, the fields should not appear empty in the panel when they still hold content. 🙂

trych avatar Mar 26 '24 15:03 trych

@trych Is it possible that no file template has been assigned to the file? Is the file still in the content folder? Please upload another file and check the .txt file to see if a template has been assigned.

gbdesign2023 avatar Mar 26 '24 19:03 gbdesign2023

@gbdesign2023 what he's saying is that the file itself was deleted but the files field still holds the reference to that non-existing file. He's right that the a panel won't show it, but tests like Empty/NotEmpty will say it has content.

distantnative avatar Mar 26 '24 19:03 distantnative

I guess the question is whether a "File not found" symbol or graphic (like a ~~crossed out filename~~ or something) is really better than simply hiding the link to the non existing file. I mean would it make the UX really better? If I think of an example where a page has a pages field with min: 1, multiple: false, then something deletes the linked file from the server and therefore the user needs to relink a new file: would it really be better to have it show a non existing file first?

That also means the files field would need to store the name of the file (not only the uuid), otherwise, at most it could only display a random identifier which isn't really useful to a user. But storing the filename makes everything more complicated: when someone renames a file, kirby now has to update every reference to it in the whole website.

My 2 cts: I'm fine with having the reference missing in the panel as this reflects the results of a ->toFiles(), which is what the docs tell us to use

rasteiner avatar Apr 18 '24 12:04 rasteiner

Closing this for the reasons outlined. Those kind of fields (pages, files) should always be used with ->toFiles() etc. to check for retrieving valid objects.

distantnative avatar Jul 23 '24 19:07 distantnative