getkirby / kirby

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

Duplicating pages without files does not remove files in panel #4844

Closed FynnZW closed 1 month ago

FynnZW commented 2 years ago

(I've already written a bit about this in a different bug #4831, posting here for visibility)

Description

While duplicating a page, setting 'copy files?' to false used to generate a new page with only text content and no images. Now, it still copies the file-UUIDs from the original content file, and keeps the connection to the images from the original page. This can be confusing for the user, because it is not obvious where those images are coming from. I think that's bad UX: Some clients might keep using these images, then at some point delete the original page and will be suprised that all the images are gone.

Expected behavior
Duplicating a page without files should remove links to file UUIDs not belonging to the new page.

I realise this is more complicated now with UUIDs. With Kirby <= 3.7, the content.txt was simply copied, and no images, pointing to an image that didn't exist anymore in that folder:

Cover: - monster-trees-in-the-fog.jpg

The panel handled that invalid link nicely by just not showing any image. Now with UUIDs, the panel still can find that image from the original page and displays it.

Just an idea: Since the file links have a very distinctive pattern, wouldn't it be possible to use regex (maybe file:\/\/[\w]{16}) to invalidate file links during duplication?

Cover: - file://soTV5M2PG5DJfuHw

becomes

Cover: - file://

Or is that replacement operation too risky?

I would just be nice to be able to use file UUIDs while still having the old, more natural duplication behaviour: When duplication without images -> no images on new, duplicated page.

To reproduce

Your setup

Kirby Version
3.8.1.1

lukasbestle commented 1 year ago

I agree that this is a bug. I'd expect that the Panel would replace all file references in the content with the new UUIDs of the respective copied files. Not only in the page content, but also in the content of all files and duplicated subpages (all of those content files may reference the files of the duplicated page tree).

The same issue occurs with page://... references to duplicated subpages within the whole duplicated page tree.

Unfortunately this all gets very complex to implement:

distantnative commented 1 year ago

I honestly disagree. If I set up a blueprint with UUIDs, I want the persistent reference to that file. I think it'd lead to very frustrating experiences when we introduce behavior that just decides that this shouldn't be the case and when duplicating, let's replace those with pointer to a different file.

If such behavior is wanted, the solution is to not store UUIDs for such files but relative IDs.

texnixe commented 1 year ago

In the example issue above, the files are not even copied, so we wouldn't have to replace, but remove all references. Wouldn't that also mean, if such references are used in Kirbytags, that the Kirbytags would have to be emptied? Or even removed? This would mean a whole lot of assumptions, possible loss of content... I somehow doubt this is a good idea.

FynnZW commented 1 year ago

I understand this is very complex and potentially risky but honestly the current duplicating behaviour is non-intuitive and can lead to user error. For a client, I have currently disabled duplicating pages altogether since 3.8.

distantnative commented 1 year ago

@FynnZW why don't you switch over those fields in question to use normal relative IDs instead of UUIDs?

I do understand that it can be confusing. Then again, I think it can be also confusing in other setups if Kirby would change a stable persistent reference to something else/new. This all depends a lot on the use case, specific function of the fields in question. So I don't think there can be a general rule what Kirby needs to do but developers need to anticipate the expected behavior and make choices in regard to UUIDs or not.

On top of this all, I don't think one could even implement auch replacing of UUIDs as Lukas and Sonja have pointed out multiple problems I doubt we could solve in a satisfying way.

FynnZW commented 1 year ago

I disagree here, for me this is clearly a bug. I cannot think of a use case where it makes sense (ux-wise) to duplicate a page without files and then have files present on the new page.

If technically it’s not possible to solve, how about a different solution: Generally remove the option to duplicate without files, always duplicate with files?

I personally will probably disable file uuids for that client, yes. @distantnative when I switch back to relative ID for those files, will they get switched back automatically? or will all file connections be lost?

FynnZW commented 1 year ago

I feel that I should explain why this behaviour, in my opinion, is wrong in another way:

Basically a CMS can have two ways to do file management:

Kirby can also be setup like wordpress, but the default way, unless otherwise specified in the blueprint is: Every page has their own files. And because most clients know wordpress, this is something they have to get taught and get used to in the beginning.

The current duplication behaviour dissolves the file system logic. It creates a situation where a page has linked files from another page, without being setup this way in the blueprint. It shouldn’t be allowed to link to these files.

lukasbestle commented 1 year ago

Oh, I actually misread the issue. I thought this was about the case "duplicate a page with its files". In this case there's still the issue that all references will point to the files of the original page, not of the duplicated page.

For the case you describe (duplicate a page without its files) I agree with Nico that we cannot assume the expected behavior. Depending on the case, either removing the UUIDs or replacing them will make sense, but we won't know which of them.

when I switch back to relative ID for those files, will they get switched back automatically? or will all file connections be lost?

All existing UUID references will be kept and will still work. But new references will use the relative paths.

The current duplication behaviour dissolves the file system logic. It creates a situation where a page has linked files from another page, without being setup this way in the blueprint. It shouldn’t be allowed to link to these files.

IMO it's not the duplication behavior that causes the issue, but the reference behavior.

Both of the file management approaches you describe can be implemented in Kirby. There are also a lot of Kirby sites out there that use a central file storage that is then used throughout the site. In this case, referencing a file by its UUID makes a lot of sense. So we cannot assume that all referenced files should be relative to the page. This would break such central file storage use cases.


I suggest the following:

FynnZW commented 1 year ago

All existing UUID references will be kept and will still work. But new references will use the relative paths.

That is great news! I will be able to revert to relative file IDs then.

Depending on the case, either removing the UUIDs or replacing them will make sense, but we won't know which of them.

That, unfortunately, is bad news. I thought Kirby was already able to distinguish between folder specific and other files. I thought #4831 (released 3.8.2) already solved the issue of duplicating with files, but I just did a test and noticed that while it now does regenerate new file UUIDs during duplication, it does not update the content file with these new UUIDs. In other words, the files get duplicated, but the page still links files from the old page (duplication source). I have opened another issue regarding that #4867.

lukasbestle commented 1 year ago

@FynnZW Thanks for opening the separate issue. That issue is what I originally thought was what you meant with this issue when I misread the description. :)

FynnZW commented 1 year ago

Great, now everything is clearer :)

I think the only way for Kirby to handle duplication with UUIDs is for the system to learn to differentiate between folder specific and global files. Once Kirby can to that, this should be more straightforward. In my opinion, this is how file references should be handled then:

Duplication with files:

Duplication without files:

FynnZW commented 1 year ago

I have now reverted back to relative file IDs, but unfortunately duplicating is still broken that way. Please see #4870. For now I will disable duplicating completely for the clients site.

distantnative commented 3 months ago

Closing this as we agree that the proposed behavior cannot generally be expected from users and cannot be done in a reliable way. We still agree that https://github.com/getkirby/kirby/issues/4867 is a sensible enhancement and are trying to make it happen. But while replacing UUIDs for others of the same type is feasible, simply removing UUID strings from content will lead to issues in many setups with e.g. KirbyTags etc.

FynnZW commented 3 months ago

I understand. I still would like to point out that it works like that with relative file IDs without a problem. The content.txt points to example.jpg, which does not exist in the new page, and it then fails silently & elegantly by not showing any image or file. Introducing an nullable/empty state (e.g. file://NULL) could recreate that previous behaviour, which in my opinion is way better UX: Because it does not allow unintended file linking to the duplication source page (which introduces weird side effects like deleting an image deletes it from the duplication source page).

distantnative commented 3 months ago

Reopening this as working on https://github.com/getkirby/kirby/pull/6567 I really got to feel the UX part of it and needed to reconsider my stance.

I am not sure if adding file://NULL which would fail makes much of a difference. Maybe it's just the lesser evil we have to live with that if not copying files over, the user has to ensure that content afterwards gets fixed if needed.

distantnative commented 1 month ago

Will be included in Kirby 5 (due to breaking changes we cannot introduce in 4.x)