Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

BUG UploadField: overwriteWarning overwrites replaceFile=false #2920

Closed
wants to merge 1 commit into from

4 participants

@JayDevlin

A warning should not overwrite the config it wants to warn about.

If somebody likes to see replaceFile=true by default, please do it in config.yml

#2904

@tractorcow tractorcow closed this
@tractorcow tractorcow reopened this
@tractorcow
Collaborator

(Sorry, trying to comment on the move)

The overwrite warning setting only applies the replace file on an instance level, not a global config one, and is intended to prevent a situation with an inconsistent user experience; an upload field should not warn about a file overwrite, but then proceed to perform a rename. That's why the existing code is in place.

@JayDevlin

@tractorcow
Which means the config replaceFile=false is completely meaningless right now, because overwriteWarning always overwrites replaceFile.

// default replaceFile=false
$upload = new UploadField('Upload');
// Upload->load(): $this->replaceFile === true

$upload = new UploadField('Upload');
$upload->setOverwriteWarning(true);
$upload->getUpload()->setReplaceFile(false);
// Upload->load(): $this->replaceFile === true

$upload = new UploadField('Upload');
$upload->setOverwriteWarning(false);
$upload->getUpload()->setReplaceFile(false);
// Upload->load(): $this->replaceFile === false

$upload = new UploadField('Upload');
$upload->setOverwriteWarning(false);
$upload->getUpload()->setReplaceFile(true);
// Upload->load(): $this->replaceFile === true

$upload = new UploadField('Upload');
$upload->setOverwriteWarning(true);
$upload->getUpload()->setReplaceFile(true);
// Upload->load(): $this->replaceFile === true
@tractorcow
Collaborator

Overwrite warning is intended to override the replacefile setting. You can still set overwrite warning to false and set the default behaviour with your setReplaceFile. Just not when overwrite warning is true.

Besides, what kind of behaviour do you expect if you set overwriteWarning to true and replaceFile to false? Have it warn an overwrite can occur, and then fail to perform the overwrite? This is the inconsistent state the existing code is there to prevent.

Since the overwriteWarning is on the containing (outer) class, the setting on the encapsulated class is therefore overridden. Doing things the other way (setReplaceFile overriding overwriteWarning) would really convolute the relationship between the two classes. In otherwords, UploadField is the container, so it gets to decide what happens to its property. :)

@JayDevlin

Besides, what kind of behaviour do you expect if you set overwriteWarning to true and replaceFile to false?

I'd expect no behaviour what so ever.
replaceFile=true should always replace files.
replaceFile=false should always version files.
overwriteWarning=true and replaceFile=true should always display a warning before overwriting anything.

Have it warn an overwrite can occur, and then fail to perform the overwrite?

My commit sets overwriteWarning to false if replaceFile = false. No warning is displayed. This is why I mentioned to set replaceFile to true by default.

@tractorcow
Collaborator

Wouldn't then we have the inverse problem? overwriteWarning should always show a warning if there's an existing file.

If I set it to true but don't change a value on a nested object, then I mistakenly am giving the reassurance that the file I'm uploading isn't overwriting an existing one.

@JayDevlin

overwriteWarning should always show a warning if there's an existing file.

Sure. But if the global config says replaceFile=false and the current instance of UploadField doesn't overwrite this config, then i think that the developer clearly expects that files are not to be overwritten.

If I set it to true but don't change a value on a nested object, then I mistakenly am giving the reassurance that the file I'm uploading isn't overwriting an existing one.

If you set it to true, all UploadFields will replace files -- like it's done now -- unless you tell a UploadField, for example for a specific folder, specifically not to do so.
Right now, this would only be possible if you'd overload UploadField.

I think it's the job for the developer to decide what he wants to accomplish. And I think it's a big problem if user A wants to upload a picture to page my-hidden-projects and thinks yeah whatever, clicks overwrite and overwrites a picture of user B on the page home.

@JayDevlin

Overwrite warning is intended to override the replacefile setting.

Not according to the documentation.
"Show a warning when overwriting a file. This requires Upload->replaceFile config to be set to true, otherwise files will be renamed instead of overwritten (although the warning will still be displayed)"

@tractorcow
Collaborator

Yeah, that was old documentation from before the fix to set replace file to true. That needs to be updated.

@JayDevlin

More documentation:
"(boolean) Show a warning when overwriting a file."
"Warning before overwriting existing file (only relevant when Upload: replaceFile is true)"
"Globally disables automatic renaming of files and displays a warning before overwriting an existing file"
reference/uploadfield.md

@tractorcow
Collaborator

@JayDevlin it doesn't globally disable renaming, it's only on an instance level.

The config value is only a default fallback if no instance level value is assigned. See https://github.com/silverstripe/silverstripe-framework/blob/3.1/filesystem/Upload.php#L78 for where the config sets the default.

@tractorcow
Collaborator

Sorry, I see you've taken that comment from the docs/yml, not making documentation suggestions. :) Yeah, the docs still refer to the old behaviour in some places, and need to be updated.

However, we still really cannot have overwriteWarning set to false automatically, for the reasons stated above. The containing class (UploadField) should always override the contained class (Upload), especially when dealing with user-level options. Doing otherwise would end up with behaviour the user doesn't expect.

overwriteWarning setting does control not only the warning, but acts as a permission check on the nested Upload object to say it's allowed to create copies. It's ok for this to this field to deny the make-copy permission by setting setReplaceFile to true, because it doesn't support user-notifications before a copy is made.

I still see your issue with the code, although I disagree with you in your resolution.

If you want to change the behaviour so it doesn't call the setReplaceFile to true, then a possible route would be to remove the setReplaceFile(true) call, but then it would be necessary to ensure that the overwrite dialogue was customised depending on one of the two values for replaceFile.

Option 1 (default option)
replace: true, overwriteWarning: true
If you select a file that exists, a notification is given saying "a file exists, do you want to overwrite it?".

Option 2:
replace: false, overwriteWarning: true
If you select a file that exists, a notification is given saying "This file exists. Do you want to rename on upload?".

Option 3:
replace: true, overwriteWarning: false
If you select a file that exists, the file overwrites this automatically.

Option 2:
replace: false, overwriteWarning: true
If you select a file that exists, the file creates a copy automatically.

@tractorcow
Collaborator

Hey @JayDevlin I've tried to solve this problem with #2926, but it needs some more feedback. Do you think I'm heading in the right direction? Please check the updated documentation also, to see if that helps explain the reasoning behind this change.

@simonwelsh simonwelsh added the 3.1 label
@JayDevlin

Hi @tractorcow,
I see you've closed #2926. So, is there something I can do?

As I statet there, I've prepared another patch which would always show a warning if overwriteWarning is true.
However I've to agree with @simonwelsh that it would be kind of unnecessary to show a overwrite warning if no overwrite is going to happen.

https://github.com/JayDevlin/silverstripe-framework/compare/2920-uploadfield-overwritewarning

@tractorcow
Collaborator

Hey @JayDevlin , I still agree with you that this is an issue. I apologise for not giving my time to look into this, but it is still on the agenda.

We can't change the 3.1 api too greatly, but I would like to implement a solution similar to your suggested one above for 3.2.

The one thing I don't like about your fix is the $this->setOverwriteWarning(false);, since it modifies the state of the object itself. On the front end this should be probably ignored when replaceFile is true, but it shouldn't alter the actual state of the PHP Object. This ironically is the problem with the existing code, and I have the same problem with the setReplaceFile(false) that you (rightly) removed.

If you want to submit a tweaked solution I'll review it. :) It'll need to be rebased against master though, not 3.1, since it's an API change.

Thanks for hanging with us there @JayDevlin , sorry to backtrack on you, but I'm starting to see your view more. ;P

@tractorcow
Collaborator

Also, I think we ditch the "rename notification". This is the part of my suggested fix I'd like to abandon for being unnecessarily complicated.

@JayDevlin

Hi @tractorcow

Sorry for the delay. I've moved replaceFile to the frontend config and rebased the commit against master.

JayDevlin@silverstripe:master...2920-uploadfield-master

Should I do a new pull request or should I force push it in here?

@tractorcow
Collaborator

Hey @JayDevlin

Love it so far. :) I should ask for some behat tests, but that might be a bit harsh to ask of you. I think you can probably squash and force push it up here.

@JayDevlin JayDevlin referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@JayDevlin

I can't change the base branch on an existing pull request, so I've created a new one.

@chillu
Owner

Closed through #3048

@chillu chillu closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 40 additions and 10 deletions.
  1. +5 −10 forms/UploadField.php
  2. +35 −0 tests/forms/uploadfield/UploadFieldTest.php
View
15 forms/UploadField.php
@@ -154,8 +154,7 @@ class UploadField extends FileField {
/**
* Show a warning when overwriting a file.
* This requires Upload->replaceFile config to be set to true, otherwise
- * files will be renamed instead of overwritten (although the warning will
- * still be displayed)
+ * files will be renamed instead of overwritten.
*
* @see Upload
* @var boolean
@@ -998,11 +997,12 @@ public function Field($properties = array()) {
}
}
- //get all the existing files in the current folder
- if ($this->getOverwriteWarning()) {
- //add overwrite warning error message to the config object sent to Javascript
+ // add overwrite warning error message to the config object sent to Javascript
+ if ($this->getUpload()->getReplaceFile() && $this->getOverwriteWarning()) {
$config['errorMessages']['overwriteWarning'] =
_t('UploadField.OVERWRITEWARNING', 'File with the same name already exists');
+ } else {
+ $this->setOverwriteWarning(false);
}
$mergedConfig = array_merge($config, $this->ufConfig);
@@ -1154,11 +1154,6 @@ protected function saveTemporaryFile($tmpFile, &$error = null) {
$fileObject = Object::create($relationClass);
}
- // Allow replacing files (rather than renaming a duplicate) when warning about overwrites
- if($this->getConfig('overwriteWarning')) {
- $this->upload->setReplaceFile(true);
- }
-
// Get the uploaded file into a new file object.
try {
$this->upload->loadIntoFile($tmpFile, $fileObject, $this->getFolderName());
View
35 tests/forms/uploadfield/UploadFieldTest.php
@@ -642,6 +642,41 @@ public function testSelect() {
$this->assertContains($file4->ID, $itemIDs, 'Contains file in assigned folder');
$this->assertNotContains($fileSubfolder->ID, $itemIDs, 'Does not contain file in subfolder');
}
+
+ /**
+ * Test that UploadField:overwriteWarning cannot overwrite Upload:replaceFile
+ */
+ public function testConfigOverwriteWarningCannotRelaceFiles() {
+ $this->loginWithPermission('ADMIN');
+
+ $tmpFileName = 'testUploadBasic.txt';
+ $response = $this->mockFileUpload('NoRelationField', $tmpFileName);
+ $this->assertFalse($response->isError());
+ $responseData = Convert::json2array($response->getBody());
+ $this->assertFileExists(ASSETS_PATH . '/UploadFieldTest/' . $responseData[0]['name']);
+ $uploadedFile = DataObject::get_by_id('File', (int) $responseData[0]['id']);
+ $this->assertTrue(is_object($uploadedFile), 'The file object is created');
+
+ Upload::config()->replaceFile = false;
+ UploadField::config()->defaultConfig = array_merge(
+ UploadField::config()->defaultConfig, array('overwriteWarning' => true)
+ );
+
+ $tmpFileName = 'testUploadBasic.txt';
+ $response = $this->mockFileUpload('NoRelationField', $tmpFileName);
+ $this->assertFalse($response->isError());
+ $responseData = Convert::json2array($response->getBody());
+ $this->assertFileExists(ASSETS_PATH . '/UploadFieldTest/' . $responseData[0]['name']);
+ $uploadedFile2 = DataObject::get_by_id('File', (int) $responseData[0]['id']);
+ $this->assertTrue(is_object($uploadedFile2), 'The file object is created');
+ $this->assertTrue(
+ $uploadedFile->Filename !== $uploadedFile2->Filename, 'Filename is not the same'
+ );
+ $this->assertGreaterThan(
+ $uploadedFile->ID, $uploadedFile2->ID, 'File database record is not the same'
+ );
+ $uploadedFile2->delete();
+ }
protected function getMockForm() {
return new Form(new Controller(), 'Form', new FieldList(), new FieldList());
Something went wrong with that request. Please try again.