Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

UploadField: Overwrite exisitng produces duplicate in DB #2897

Closed
Zauberfisch opened this Issue Feb 25, 2014 · 7 comments

Comments

Projects
None yet
2 participants
Contributor

Zauberfisch commented Feb 25, 2014

The UploadField offers the choice to overwrite an existing File if a file by that name already exists.
It will overwrite the file as expected, however only the file (Filesystem) but not the record in the File database table.
This means that a file exists twice in the DB, pointing to the same physical file.

Now, that works until SilverStripe does a file Sync, this will remove the duplicate DB records and now all of a sudden some pages Link to files (db records) that no longer exist.

Contributor

tractorcow commented Feb 26, 2014

There are one of two options here:

  • Create a new record and delete the old, breaking existing links to the old image across the site.
  • Reuse the existing record, but potentially replacing the image used in other parts of the website.

I feel that going with option 2 might have unexpected behaviour; Updating a home page image with the same name as a blog image might unintentionally cause your banner to appear on an unrelated blog article.

On the other hand, there are likely cases though where a site is built in such a way that updating an image in one place should replace it everywhere.

At least you would get a warning that a file is about to be overwritten, but it doesn't really indicate the consequences of selecting "yes, overwrite this file".

I suggest this behaviour is thus made configurable, with a reasonable default set to one of the two above options. This would probably be more appropriate as a global config rather than a per-UploadField one, to ensure consistent behaviour across any one site.

Contributor

Zauberfisch commented Feb 26, 2014

I would go with the 2nd option. In fact I wouldn't even consider option 1.
(I am not even sure I would care to have it configurable)

overwriting a file should do what it says, overwrite the file. not remove its relation or any other thing.
If I upload a new terms-of-service.pdf and it tells me it will overwrite, I would expect the file to change at all places, not just at one.

Perhaps in addition we could extend the message telling the user that this will effect all places that use this file. (basically: explain what overwrite means :P)

Contributor

tractorcow commented Feb 26, 2014

I guess so, I was more concerned about some less careful end users who don't watch their filenames. In those cases it'd probably be better to switch to auto-rename, maybe.

Contributor

Zauberfisch commented Feb 26, 2014

How about an additional button asking the user if he wants to replace the file, or rename it and keep both (just like the common operating systems do).

screen shot 2014-02-26 at 05 33 55

  • left button: overwrite the file, use the existing DB record
  • right button: rename file (filename.ext => filename-1.ext) and create a new DB record
Contributor

tractorcow commented Feb 26, 2014

Currently this mechanism is defined by the 'overwriteWarning' option, which forces the replacefile method to true. No warning means it will rename by default.

I feel that in most situations the ability to let the user decide the outcome is going to give the most flexible and most robust behaviour. I suggest that this replaces the overwriteWarning option, since the user-controlled behaviour supercededs the configuration defined one. This would of course be an API change, and would have to target 3.2 (in likelihood).

It might pay to shorten the names on each button to "Replace Existing" and "Rename". Locales such as dutch are notorious for huge button length translations. :)

Contributor

Zauberfisch commented Feb 27, 2014

think about it further, in fact operating systems have solved that problem quiet well already.
we should just do it the way they do it.

os

ss

Contributor

tractorcow commented Mar 5, 2014

Fixed with #2922. @Zauberfisch feel free to open another PR to enhance the overwrite notification dialog, since I think that's an enhancement on top of the issue we've noted in this.

@tractorcow tractorcow closed this Mar 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment