From a2c9c409a8d76214a189a054e8996179a905ca91 Mon Sep 17 00:00:00 2001 From: Mateusz Uzdowski Date: Wed, 18 Apr 2012 14:46:54 +1200 Subject: [PATCH 1/4] BUGFIX: fixed asserts and file cleanup DataObject::get_one returns false if not found, so better check for object. Also, the directory would not be cleaned, so on the subsequent run the files could end up having suffixes. missed this one --- tests/forms/uploadfield/UploadFieldTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/forms/uploadfield/UploadFieldTest.php b/tests/forms/uploadfield/UploadFieldTest.php index 7df16d2ad..7a3750891 100644 --- a/tests/forms/uploadfield/UploadFieldTest.php +++ b/tests/forms/uploadfield/UploadFieldTest.php @@ -28,7 +28,7 @@ $this->assertFalse($response->isError()); $this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/$tmpFileName"); $uploadedFile = DataObject::get_one('File', sprintf('"Name" = \'%s\'', $tmpFileName)); - $this->assertNotNull($uploadedFile); + $this->assertTrue(is_object($uploadedFile), 'The file object is created'); } function testUploadHasOneRelation() { @@ -48,7 +48,7 @@ $this->assertFalse($response->isError()); $this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/$tmpFileName"); $uploadedFile = DataObject::get_one('File', sprintf('"Name" = \'%s\'', $tmpFileName)); - $this->assertNotNull($uploadedFile); + $this->assertTrue(is_object($uploadedFile), 'The file object is created'); $record = DataObject::get_by_id($record->class, $record->ID, false); $this->assertTrue($record->HasOneFile()->exists()); @@ -69,7 +69,7 @@ $this->assertFalse($response->isError()); $this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/$tmpFileName"); $uploadedFile = DataObject::get_one('File', sprintf('"Name" = \'%s\'', $tmpFileName)); - $this->assertNotNull($uploadedFile); + $this->assertTrue(is_object($uploadedFile), 'The file object is created'); $record = DataObject::get_by_id($record->class, $record->ID, false); $this->assertEquals(3, $record->HasManyFiles()->Count()); @@ -91,7 +91,7 @@ $this->assertFalse($response->isError()); $this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/$tmpFileName"); $uploadedFile = DataObject::get_one('File', sprintf('"Name" = \'%s\'', $tmpFileName)); - $this->assertNotNull($uploadedFile); + $this->assertTrue(is_object($uploadedFile), 'The file object is created'); $record = DataObject::get_by_id($record->class, $record->ID, false); $this->assertEquals($relationCount+1, $record->ManyManyFiles()->Count()); @@ -605,7 +605,7 @@ } // Remove left over folders and any files that may exist - if(file_exists('../assets/UploadFieldTest')) Filesystem::removeFolder('../assets/FileTest'); + if(file_exists('../assets/UploadFieldTest')) Filesystem::removeFolder('../assets/UploadFieldTest'); } } From 5d7f6a0d69fd8ee947f077a4346edba4a29e9064 Mon Sep 17 00:00:00 2001 From: Mateusz Uzdowski Date: Wed, 18 Apr 2012 15:18:19 +1200 Subject: [PATCH 2/4] MINOR: add some comments to UploadField --- forms/UploadField.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/forms/UploadField.php b/forms/UploadField.php index 78338e5c8..27fcfe7b7 100644 --- a/forms/UploadField.php +++ b/forms/UploadField.php @@ -442,14 +442,20 @@ class UploadField extends FileField { 'error' => $tmpfile['error'] ); } + + // Check for constraints on the record to which the file will be attached. if (!$return['error'] && $record && $record->exists()) { $tooManyFiles = false; + // Some relationships allow many files to be attached. if ($this->getConfig('allowedMaxFileNumber') && ($record->has_many($name) || $record->many_many($name))) { if(!$record->isInDB()) $record->write(); $tooManyFiles = $record->{$name}()->count() >= $this->getConfig('allowedMaxFileNumber'); + // has_one only allows one file at any given time. } elseif($record->has_one($name)) { $tooManyFiles = $record->{$name}() && $record->{$name}()->exists(); } + + // Report the constraint violation. if ($tooManyFiles) { if(!$this->getConfig('allowedMaxFileNumber')) $this->setConfig('allowedMaxFileNumber', 1); $return['error'] = sprintf(_t( @@ -469,9 +475,12 @@ class UploadField extends FileField { if ($this->upload->isError()) { $return['error'] = implode(' '.PHP_EOL, $this->upload->getErrors()); } else { + // The file has been uploaded successfully, attach it to the related record. $file = $this->upload->getFile(); $file->write(); $this->attachFile($file); + + // Collect all output data. $file = $this->customiseFile($file); $return = array_merge($return, array( 'id' => $file->ID, From 47d7392e88568b0e3d1a3ee29787b7faaef2c1c7 Mon Sep 17 00:00:00 2001 From: Mateusz Uzdowski Date: Wed, 18 Apr 2012 15:21:13 +1200 Subject: [PATCH 3/4] BUGFIX: remove unnecessary write from uploading routine --- forms/UploadField.php | 1 - 1 file changed, 1 deletion(-) diff --git a/forms/UploadField.php b/forms/UploadField.php index 27fcfe7b7..6241c286f 100644 --- a/forms/UploadField.php +++ b/forms/UploadField.php @@ -477,7 +477,6 @@ class UploadField extends FileField { } else { // The file has been uploaded successfully, attach it to the related record. $file = $this->upload->getFile(); - $file->write(); $this->attachFile($file); // Collect all output data. From a9e7de0cf4b5ac07fb40839cb77e2fb6ab483039 Mon Sep 17 00:00:00 2001 From: Mateusz Uzdowski Date: Wed, 18 Apr 2012 15:22:40 +1200 Subject: [PATCH 4/4] BUGFIX: make UploadField aware of relations to derived classes (os7140) UploadField was relying entirely on the File::get_class_for_file_extension to select a class, so it could only create File or Image objects. This would break the relationships based on derived objects. Also make it respect the FileField::relationAutoSetting. --- forms/UploadField.php | 36 ++++++++++++++++--- tests/forms/uploadfield/UploadFieldTest.php | 39 +++++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/forms/UploadField.php b/forms/UploadField.php index 6241c286f..9a9fd416e 100644 --- a/forms/UploadField.php +++ b/forms/UploadField.php @@ -432,6 +432,7 @@ class UploadField extends FileField { $tmpfile = $request->postVar($name); $record = $this->getRecord(); + // Check if the file has been uploaded into the temporary storage. if (!$tmpfile) { $return = array('error' => _t('UploadField.FIELDNOTSET', 'File information not found')); } else { @@ -444,7 +445,7 @@ class UploadField extends FileField { } // Check for constraints on the record to which the file will be attached. - if (!$return['error'] && $record && $record->exists()) { + if (!$return['error'] && $this->relationAutoSetting && $record && $record->exists()) { $tooManyFiles = false; // Some relationships allow many files to be attached. if ($this->getConfig('allowedMaxFileNumber') && ($record->has_many($name) || $record->many_many($name))) { @@ -464,20 +465,47 @@ class UploadField extends FileField { ), $this->getConfig('allowedMaxFileNumber')); } } + + // Process the uploaded file if (!$return['error']) { + $fileObject = null; + + if ($this->relationAutoSetting) { + // Search for classes that can hold the uploaded files by traversing the relationship. + if ($record->hasMethod($name)) { + $remote = $record->$name(); + if ($remote instanceof DataList) { + // has_many or many_many + $desiredClass = $remote->dataClass(); + } + else if (is_object($remote)) { + // has_one + $desiredClass = $remote->ClassName; + } + + // If we have a specific class, create new object explicitly. Otherwise rely on Upload::load to choose the class. + if (is_string($desiredClass)) $fileObject = Object::create($desiredClass); + } + } + + // Get the uploaded file into a new file object. try { - $this->upload->loadIntoFile($tmpfile, null, $this->folderName); + $this->upload->loadIntoFile($tmpfile, $fileObject, $this->folderName); } catch (Exception $e) { // we shouldn't get an error here, but just in case $return['error'] = $e->getMessage(); } + if (!$return['error']) { if ($this->upload->isError()) { $return['error'] = implode(' '.PHP_EOL, $this->upload->getErrors()); } else { - // The file has been uploaded successfully, attach it to the related record. $file = $this->upload->getFile(); - $this->attachFile($file); + + // Attach the file to the related record. + if ($this->relationAutoSetting) { + $this->attachFile($file); + } // Collect all output data. $file = $this->customiseFile($file); diff --git a/tests/forms/uploadfield/UploadFieldTest.php b/tests/forms/uploadfield/UploadFieldTest.php index 7a3750891..7340cbfd4 100644 --- a/tests/forms/uploadfield/UploadFieldTest.php +++ b/tests/forms/uploadfield/UploadFieldTest.php @@ -55,6 +55,31 @@ $this->assertEquals($record->HasOneFile()->Name, $tmpFileName); } + function testUploadHasOneRelationWithExtendedFile() { + $this->loginWithPermission('ADMIN'); + + // Unset existing has_one relation before re-uploading + $record = $this->objFromFixture('UploadFieldTest_Record', 'record1'); + $record->HasOneExtendedFileID = null; + $record->write(); + + $tmpFileName = 'testUploadHasOneRelationWithExtendedFile.txt'; + $_FILES = array('HasOneExtendedFile' => $this->getUploadFile($tmpFileName)); + $response = $this->post( + 'UploadFieldTest_Controller/Form/field/HasOneExtendedFile/upload', + array('HasOneExtendedFile' => $this->getUploadFile($tmpFileName)) + ); + $this->assertFalse($response->isError()); + + $this->assertFileExists(ASSETS_PATH . "/UploadFieldTest/$tmpFileName"); + $uploadedFile = DataObject::get_one('UploadFieldTest_ExtendedFile', sprintf('"Name" = \'%s\'', $tmpFileName)); + $this->assertTrue(is_object($uploadedFile), 'The file object is created'); + + $record = DataObject::get_by_id($record->class, $record->ID, false); + $this->assertTrue($record->HasOneExtendedFile()->exists(), 'The extended file is attached to the class'); + $this->assertEquals($record->HasOneExtendedFile()->Name, $tmpFileName, 'Proper file has been attached'); + } + function testUploadHasManyRelation() { $this->loginWithPermission('ADMIN'); @@ -620,6 +645,7 @@ class UploadFieldTest_Record extends DataObject implements TestOnly { 'HasOneFile' => 'File', 'HasOneFileMaxOne' => 'File', 'HasOneFileMaxTwo' => 'File', + 'HasOneExtendedFile' => 'UploadFieldTest_ExtendedFile' ); static $has_many = array( @@ -666,6 +692,10 @@ class UploadFieldTest_Controller extends Controller implements TestOnly { $fieldHasOne = new UploadField('HasOneFile'); $fieldHasOne->setFolderName('UploadFieldTest'); $fieldHasOne->setRecord($record); + + $fieldHasOneExtendedFile = new UploadField('HasOneExtendedFile'); + $fieldHasOneExtendedFile->setFolderName('UploadFieldTest'); + $fieldHasOneExtendedFile->setRecord($record); $fieldHasOneMaxOne = new UploadField('HasOneFileMaxOne'); $fieldHasOneMaxOne->setFolderName('UploadFieldTest'); @@ -712,6 +742,7 @@ class UploadFieldTest_Controller extends Controller implements TestOnly { $fieldHasOne, $fieldHasOneMaxOne, $fieldHasOneMaxTwo, + $fieldHasOneExtendedFile, $fieldHasMany, $fieldHasManyMaxTwo, $fieldManyMany, @@ -727,6 +758,7 @@ class UploadFieldTest_Controller extends Controller implements TestOnly { 'HasOneFile', 'HasOneFileMaxOne', 'HasOneFileMaxTwo', + 'HasOneExtendedFile', 'HasManyFiles', 'HasManyFilesMaxTwo', 'ManyManyFiles', @@ -743,3 +775,10 @@ class UploadFieldTest_Controller extends Controller implements TestOnly { } } + +/** + * Used for testing the create-on-upload + */ +class UploadFieldTest_ExtendedFile extends File implements TestOnly { + +}