API Enhanced the overwriteWarning behaviour to work independently of the replaceFile setting #2926

Closed
wants to merge 1 commit into from

3 participants

@tractorcow

This should hopefully provide a solution to the issue raised at #2920

When we introduced the overwriteWarning option, this field would force replaceFile to true in any situation, even if the default value is set to false (or set to false in user-code).

To acommodate more flexibility in configuration I've enhanced the notification message slightly, so that the message displayed to users when they upload an already existing file differs slightly, depending on whether they have replaceFile set to true or false.

with replaceFile set to true:

clipboard-1

with replaceFile set to false:

clipboard-2

I've updated the documentation to hopefully better explain how these field options should be configured, and updated the front end usage guidelines.

Unfortunately, in order to maintain backwards compatibility, I've had to introduce an UploadField.replaceFile default config. The reason for this introduction is that according to the current behaviour, overwriteWarning is true, and this forces replaceFile to be true on the instance. If I let it inherit the default value from Upload.replaceFile, then this would effectively change the current behaviour so that renaming was the default option, instead of replacing. Hopefully, this at least means that there is no behaviour change for those simply upgrading from the previous version.

Upload.replaceFile is still in place, but is only useful in cases where the Upload class is used as a stand-alone uploader.

Now the default behaviour for UploadField can be easily configured by updating the overwriteWarning and replaceFile configuration keys on UploadField, and also customised on a per-instance basis.

@tractorcow tractorcow API Enhanced the overwriteWarning behaviour to work independent of th…
…e replaceFile setting

Upload.replaceFile and UploadField.replaceFile are independent globally configured defaults to separate each use case
e2c24ef
@tractorcow

If a user has the following

$field = UploadField::create('Image')
->setOverwriteWarning(false);

Then upgrading this code would change the default for replaceFile to true, when the existing code would probably expect this to be false. It's a little hard to find a solution that's perfectly backwards compatible.

Does anyone prefer to simply have a single replaceFile setting (keep it on Upload) and inherit it that way, rather than having them indepedently configured? What should the default for this setting be?

@JayDevlin

Yup. This looks fine for me.

Here's my take (though it doesn't really bring anything new to the table except that I renamed some buttons in preparation for Zauberfisch's enhancement suggestion in #2897)

silverstripe-framework/compare/2920-uploadfield-overwritewarning

@simonwelsh

This can't go into 3.1 and I'm still hesitant about it. If I set replaceFile to false (which I still think should be the default, but that's a different debate), I wouldn't expect a warning to appear when renaming. What's the benefit of having a warning in that case?

@tractorcow
@tractorcow tractorcow closed this Mar 7, 2014
@tractorcow

I'll look at a simpler solution to this problem.

Maybe instead of overwriteWarning forcing replaceFile to true, it's ignored if replaceFile is false. (as per @JayDevlin's suggestion). I probably wouldn't SET It to false though, but would simply not make it show a warning if the behaviour is set to rename.

I was hesitant to make replaceFile false by default, since UploadField sets it to true in most situations, but since @simonwelsh seems to feel otherwise I can probably tweak that behaviour a little.

@tractorcow tractorcow deleted the tractorcow:pulls/3.1-uploadfield-overwrite-warning branch Apr 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment