umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.43k stars 2.67k forks source link

File Uploads are removed when page is saved #14210

Open lumimario opened 1 year ago

lumimario commented 1 year ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

10.4.1

Bug summary

When you use a File Upload datatype in a page (not media picker!), you upload a file and save and published, you can navigate to the upload file properly.

But then if you remove the file using the Remove file(s) option and upload a new one, then save the page, the file is deleted from the files system straight away.

image

This behaviour makes that Saving the current version with a new file makes the Rollback functionality useless as restoring the previous version doesn't restore the previous file.

The uploaded file shouldn't be removed from the filesystem. It looks like the uploaded files always go to the same folder . i.e.: https://localhost:44308/media/zz4lyavm/file1.pdf https://localhost:44308/media/zz4lyavm/file2.pdf

I think it should generate different folders for each uploaded file so it can keep previous versions and don't delete the actual files until the history hasn't been cleared. This way, we could rollback to previous file versions.

Specifics

No response

Steps to reproduce

Extended testing

Expected result / actual result

I expect to rollback to previous versions of the page and still have access to be able to recover the files uploaded don't that version of the page.


_This item has been added to our backlog AB#29285_

github-actions[bot] commented 1 year ago

Hi there @lumimario!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

Zeegaan commented 1 year ago

Heyo! 👋 Just jumping in here to confirm I can reproduce this!

This is very similar to https://github.com/umbraco/Umbraco-CMS/issues/13445 (albeit a file upload property this time) But we should probably be aligning the behavior when we're taking a look at it!

skartknet commented 1 year ago

this looks the same issue: https://github.com/umbraco/Umbraco-CMS/issues/13214

skartknet commented 1 year ago

I just found that in Umbraco version 12, the umbracoFile properties on the media types is required! So this 'fixes' the issue.

EDIT: actually it doesn't :( If you replace the file, the old one is gone before you can even cancel the action.

lumimario commented 1 year ago

It seems that the deletion of the previous file happens on the ImageCropperPropertyValueEditor.FromEditor method, line 187. This is being called from the MapPropertyValuesForPersistence method on the ContentControllerBase.

image

Because this is being called from the controller, there's no way we can cancel this from the MediaService notifications.

I'm not sure if the previous file should even be removed. As I said in the ticket description, if the previous file is removed, we cannot rollback to previous versions of the page.

JasonElkin commented 1 year ago

Hey @lumimario, I raised this with HQ, and it's still on the backlog because it opens up a can of worms when it comes to file persistence. For example, a user could think a file has been "deleted", but it still be publicly accessible. What determines how those files eventually get deleted when multiple versions use the same file etc.

Those (and any other similar knock on effects) will need to be discussed before a fix or new feature that addresses this can be rolled out.

Using a media picker, i.e. separating content versioning from file persistence, looks to be the simplest way to solve this in the short term.

I've not tried this, but I expect you could create your own property editor, inheriting from FileUploadPropertyEditor, modifying this logic to suit your use case, then replacing the core one with your own (you will also need to remove references to the core editor from the MediaUrlGenerators and Notifications)

ronaldbarendse commented 1 year ago

It looks like the uploaded files always go to the same folder . i.e.: https://localhost:44308/media/zz4lyavm/file1.pdf https://localhost:44308/media/zz4lyavm/file2.pdf

I think it should generate different folders for each uploaded file so it can keep previous versions and don't delete the actual files until the history hasn't been cleared.

Just to comment on the generation of the media folders: this is determined by the IMediaPathScheme, which by default uses UniqueMediaPathScheme that combines the content/media item and property keys: https://github.com/umbraco/Umbraco-CMS/blob/c4c524deb54b3651337b4d35b273ab8cb4fa9d82/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.FileSystems.cs#L40 https://github.com/umbraco/Umbraco-CMS/blob/60a5b19dc9fe24f5379a253e1f3fdcb9cb537dfc/src/Umbraco.Core/IO/MediaPathSchemes/UniqueMediaPathScheme.cs#L16-L17

Because these keys don't change when updating content, the same folder will be used to store the file... The same is true for media, so I don't think this is a real issue (and might actually be a feature, since you might want to do an in-place file replacement using the same file name).

We also have the reverse happening when trashing content/media: the files are only deleted when the actual item is deleted from the recycle bin. This might not be the same issue, but should at least be included when discussing a possible fix or new feature that addresses this 😄

smarshallsay commented 4 months ago

This still seems to be a problem in 2024 with v13, I'm implementing a kind of media file versioning but because of this issue I basically need to make a backup copy when a file is uploaded as if in the saving event I want to make a backup copy of the old file it has already disappeared :'(

My messy workaround for this is to store a backup copy of the file that is uploaded in the folder in order to make a versioned copy of it when it is replaced but this is just interim until I can think of something more elegant :D