sminnee /
silverstripe-framework
Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.
Common duplication problems, and corresponding solutions are:
Complex classes like FileTest often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.
Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.
While breaking up the class, it is a good idea to analyze how other classes use FileTest, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 6 | class FileTest extends SapphireTest { |
||
| 7 | |||
| 8 | protected static $fixture_file = 'FileTest.yml'; |
||
| 9 | |||
| 10 | protected $extraDataObjects = array('FileTest_MyCustomFile'); |
||
| 11 | |||
| 12 | public function testLinkShortcodeHandler() { |
||
| 13 | $testFile = $this->objFromFixture('File', 'asdf'); |
||
| 14 | |||
| 15 | $parser = new ShortcodeParser(); |
||
| 16 | $parser->register('file_link', array('File', 'link_shortcode_handler')); |
||
| 17 | |||
| 18 | $fileShortcode = sprintf('[file_link,id=%d]', $testFile->ID); |
||
| 19 | $fileEnclosed = sprintf('[file_link,id=%d]Example Content[/file_link]', $testFile->ID); |
||
| 20 | |||
| 21 | $fileShortcodeExpected = $testFile->Link(); |
||
| 22 | $fileEnclosedExpected = sprintf( |
||
| 23 | '<a href="%s" class="file" data-type="txt" data-size="977 KB">Example Content</a>', $testFile->Link()); |
||
| 24 | |||
| 25 | $this->assertEquals($fileShortcodeExpected, $parser->parse($fileShortcode), 'Test that simple linking works.'); |
||
| 26 | $this->assertEquals($fileEnclosedExpected, $parser->parse($fileEnclosed), 'Test enclosed content is linked.'); |
||
| 27 | |||
| 28 | $testFile->delete(); |
||
| 29 | |||
| 30 | $fileShortcode = '[file_link,id="-1"]'; |
||
| 31 | $fileEnclosed = '[file_link,id="-1"]Example Content[/file_link]'; |
||
| 32 | |||
| 33 | $this->assertEquals('', $parser->parse('[file_link]'), 'Test that invalid ID attributes are not parsed.'); |
||
| 34 | $this->assertEquals('', $parser->parse('[file_link,id="text"]')); |
||
| 35 | $this->assertEquals('', $parser->parse('[file_link]Example Content[/file_link]')); |
||
| 36 | |||
| 37 | if(class_exists('ErrorPage')) { |
||
| 38 | $errorPage = ErrorPage::get()->filter('ErrorCode', 404)->First(); |
||
| 39 | $this->assertEquals( |
||
| 40 | $errorPage->Link(), |
||
| 41 | $parser->parse($fileShortcode), |
||
| 42 | 'Test link to 404 page if no suitable matches.' |
||
| 43 | ); |
||
| 44 | $this->assertEquals( |
||
| 45 | sprintf('<a href="%s">Example Content</a>', $errorPage->Link()), |
||
| 46 | $parser->parse($fileEnclosed) |
||
| 47 | ); |
||
| 48 | } else { |
||
| 49 | $this->assertEquals('', $parser->parse($fileShortcode), |
||
| 50 | 'Short code is removed if file record is not present.'); |
||
| 51 | $this->assertEquals('', $parser->parse($fileEnclosed)); |
||
| 52 | } |
||
| 53 | } |
||
| 54 | |||
| 55 | public function testCreateWithFilenameWithSubfolder() { |
||
| 56 | // Note: We can't use fixtures/setUp() for this, as we want to create the db record manually. |
||
| 57 | // Creating the folder is necessary to avoid having "Filename" overwritten by setName()/setRelativePath(), |
||
| 58 | // because the parent folders don't exist in the database |
||
| 59 | $folder = Folder::find_or_make('/FileTest/'); |
||
| 60 | $testfilePath = 'assets/FileTest/CreateWithFilenameHasCorrectPath.txt'; // Important: No leading slash |
||
| 61 | $fh = fopen(BASE_PATH . '/' . $testfilePath, "w"); |
||
| 62 | fwrite($fh, str_repeat('x',1000000)); |
||
| 63 | fclose($fh); |
||
| 64 | |||
| 65 | $file = new File(); |
||
| 66 | $file->Filename = $testfilePath; |
||
| 67 | // TODO This should be auto-detected |
||
| 68 | $file->ParentID = $folder->ID; |
||
| 69 | $file->write(); |
||
| 70 | |||
| 71 | $this->assertEquals('CreateWithFilenameHasCorrectPath.txt', $file->Name, |
||
| 72 | '"Name" property is automatically set from "Filename"'); |
||
| 73 | $this->assertEquals($testfilePath, $file->Filename, |
||
| 74 | '"Filename" property remains unchanged'); |
||
| 75 | |||
| 76 | // TODO This should be auto-detected, see File->updateFilesystem() |
||
| 77 | // $this->assertInstanceOf('Folder', $file->Parent(), 'Parent folder is created in database'); |
||
| 78 | // $this->assertFileExists($file->Parent()->getFullPath(), 'Parent folder is created on filesystem'); |
||
| 79 | // $this->assertEquals('FileTest', $file->Parent()->Name); |
||
| 80 | // $this->assertInstanceOf('Folder', $file->Parent()->Parent(), 'Grandparent folder is created in database'); |
||
| 81 | // $this->assertFileExists($file->Parent()->Parent()->getFullPath(), |
||
| 82 | // 'Grandparent folder is created on filesystem'); |
||
| 83 | // $this->assertEquals('assets', $file->Parent()->Parent()->Name); |
||
| 84 | } |
||
| 85 | |||
| 86 | public function testGetExtension() { |
||
| 87 | $this->assertEquals('', File::get_file_extension('myfile'), |
||
| 88 | 'No extension'); |
||
| 89 | $this->assertEquals('txt', File::get_file_extension('myfile.txt'), |
||
| 90 | 'Simple extension'); |
||
| 91 | $this->assertEquals('gz', File::get_file_extension('myfile.tar.gz'), |
||
| 92 | 'Double-barrelled extension only returns last bit'); |
||
| 93 | } |
||
| 94 | |||
| 95 | public function testValidateExtension() { |
||
| 96 | Session::set('loggedInAs', null); |
||
| 97 | |||
| 98 | $orig = Config::inst()->get('File', 'allowed_extensions'); |
||
| 99 | Config::inst()->remove('File', 'allowed_extensions'); |
||
| 100 | Config::inst()->update('File', 'allowed_extensions', array('txt')); |
||
| 101 | |||
| 102 | $file = $this->objFromFixture('File', 'asdf'); |
||
| 103 | |||
| 104 | // Invalid ext |
||
| 105 | $file->Name = 'asdf.php'; |
||
|
|
|||
| 106 | $v = $file->doValidate(); |
||
| 107 | $this->assertFalse($v->valid()); |
||
| 108 | $this->assertContains('Extension is not allowed', $v->message()); |
||
| 109 | |||
| 110 | // Valid ext |
||
| 111 | $file->Name = 'asdf.txt'; |
||
| 112 | $v = $file->doValidate(); |
||
| 113 | $this->assertTrue($v->valid()); |
||
| 114 | |||
| 115 | // Capital extension is valid as well |
||
| 116 | $file->Name = 'asdf.TXT'; |
||
| 117 | $v = $file->doValidate(); |
||
| 118 | $this->assertTrue($v->valid()); |
||
| 119 | |||
| 120 | Config::inst()->remove('File', 'allowed_extensions'); |
||
| 121 | Config::inst()->update('File', 'allowed_extensions', $orig); |
||
| 122 | } |
||
| 123 | |||
| 124 | View Code Duplication | public function testSetNameChangesFilesystemOnWrite() { |
|
| 125 | $file = $this->objFromFixture('File', 'asdf'); |
||
| 126 | $oldPath = $file->getFullPath(); |
||
| 127 | |||
| 128 | // Before write() |
||
| 129 | $file->Name = 'renamed.txt'; |
||
| 130 | $this->assertFileExists($oldPath, |
||
| 131 | 'Old path is still present'); |
||
| 132 | $this->assertFileNotExists($file->getFullPath(), |
||
| 133 | 'New path is updated in memory, not written before write() is called'); |
||
| 134 | |||
| 135 | $file->write(); |
||
| 136 | |||
| 137 | // After write() |
||
| 138 | clearstatcache(); |
||
| 139 | $this->assertFileNotExists($oldPath, 'Old path is removed after write()'); |
||
| 140 | $this->assertFileExists($file->getFullPath(), 'New path is created after write()'); |
||
| 141 | } |
||
| 142 | |||
| 143 | View Code Duplication | public function testSetParentIDChangesFilesystemOnWrite() { |
|
| 144 | $file = $this->objFromFixture('File', 'asdf'); |
||
| 145 | $subfolder = $this->objFromFixture('Folder', 'subfolder'); |
||
| 146 | $oldPath = $file->getFullPath(); |
||
| 147 | |||
| 148 | // set ParentID |
||
| 149 | $file->ParentID = $subfolder->ID; |
||
| 150 | |||
| 151 | // Before write() |
||
| 152 | $this->assertFileExists($oldPath, |
||
| 153 | 'Old path is still present'); |
||
| 154 | $this->assertFileNotExists($file->getFullPath(), |
||
| 155 | 'New path is updated in memory, not written before write() is called'); |
||
| 156 | |||
| 157 | $file->write(); |
||
| 158 | |||
| 159 | // After write() |
||
| 160 | clearstatcache(); |
||
| 161 | $this->assertFileNotExists($oldPath, |
||
| 162 | 'Old path is removed after write()'); |
||
| 163 | $this->assertFileExists($file->getFullPath(), |
||
| 164 | 'New path is created after write()'); |
||
| 165 | } |
||
| 166 | |||
| 167 | /** |
||
| 168 | * @see http://open.silverstripe.org/ticket/5693 |
||
| 169 | * |
||
| 170 | * @expectedException ValidationException |
||
| 171 | */ |
||
| 172 | public function testSetNameWithInvalidExtensionDoesntChangeFilesystem() { |
||
| 173 | $orig = Config::inst()->get('File', 'allowed_extensions'); |
||
| 174 | Config::inst()->remove('File', 'allowed_extensions'); |
||
| 175 | Config::inst()->update('File', 'allowed_extensions', array('txt')); |
||
| 176 | |||
| 177 | $file = $this->objFromFixture('File', 'asdf'); |
||
| 178 | $oldPath = $file->getFullPath(); |
||
| 179 | |||
| 180 | $file->Name = 'renamed.php'; // evil extension |
||
| 181 | try { |
||
| 182 | $file->write(); |
||
| 183 | } catch(ValidationException $e) { |
||
| 184 | Config::inst()->remove('File', 'allowed_extensions'); |
||
| 185 | Config::inst()->update('File', 'allowed_extensions', $orig); |
||
| 186 | throw $e; |
||
| 187 | } |
||
| 188 | } |
||
| 189 | |||
| 190 | public function testLinkAndRelativeLink() { |
||
| 191 | $file = $this->objFromFixture('File', 'asdf'); |
||
| 192 | $this->assertEquals(ASSETS_DIR . '/FileTest.txt', $file->RelativeLink()); |
||
| 193 | $this->assertEquals(Director::baseURL() . ASSETS_DIR . '/FileTest.txt', $file->Link()); |
||
| 194 | } |
||
| 195 | |||
| 196 | public function testGetRelativePath() { |
||
| 197 | $rootfile = $this->objFromFixture('File', 'asdf'); |
||
| 198 | $this->assertEquals('assets/FileTest.txt', $rootfile->getRelativePath(), 'File in assets/ folder'); |
||
| 199 | |||
| 200 | $subfolderfile = $this->objFromFixture('File', 'subfolderfile'); |
||
| 201 | $this->assertEquals('assets/FileTest-subfolder/FileTestSubfolder.txt', $subfolderfile->getRelativePath(), |
||
| 202 | 'File in subfolder within assets/ folder, with existing Filename'); |
||
| 203 | |||
| 204 | $subfolderfilesetfromname = $this->objFromFixture('File', 'subfolderfile-setfromname'); |
||
| 205 | $this->assertEquals('assets/FileTest-subfolder/FileTestSubfolder2.txt', |
||
| 206 | $subfolderfilesetfromname->getRelativePath(), |
||
| 207 | 'File in subfolder within assets/ folder, with Filename generated through setName()'); |
||
| 208 | } |
||
| 209 | |||
| 210 | public function testGetFullPath() { |
||
| 214 | |||
| 215 | public function testGetURL() { |
||
| 219 | |||
| 220 | public function testGetAbsoluteURL() { |
||
| 224 | |||
| 225 | public function testNameAndTitleGeneration() { |
||
| 226 | /* If objects are loaded into the system with just a Filename, then Name is generated but Title isn't */ |
||
| 227 | $file = $this->objFromFixture('File', 'asdf'); |
||
| 228 | $this->assertEquals('FileTest.txt', $file->Name); |
||
| 229 | $this->assertNull($file->Title); |
||
| 230 | |||
| 231 | /* However, if Name is set instead of Filename, then Title is set */ |
||
| 232 | $file = $this->objFromFixture('File', 'setfromname'); |
||
| 233 | $this->assertEquals(ASSETS_DIR . '/FileTest.png', $file->Filename); |
||
| 236 | |||
| 237 | public function testSizeAndAbsoluteSizeParameters() { |
||
| 245 | |||
| 246 | public function testFileType() { |
||
| 260 | |||
| 261 | /** |
||
| 262 | * Test the File::format_size() method |
||
| 263 | */ |
||
| 264 | public function testFormatSize() { |
||
| 278 | |||
| 279 | public function testDeleteDatabaseOnly() { |
||
| 291 | |||
| 292 | public function testRenameFolder() { |
||
| 328 | |||
| 329 | |||
| 330 | public function testGetClassForFileExtension() { |
||
| 353 | |||
| 354 | public function testFolderConstructChild() { |
||
| 365 | |||
| 366 | public function testSetsOwnerOnFirstWrite() { |
||
| 387 | |||
| 388 | public function testCanEdit() { |
||
| 415 | |||
| 416 | ///////////////////////////////////////////////////////////////////////////////////////////////////////////// |
||
| 417 | |||
| 418 | public function setUp() { |
||
| 449 | |||
| 450 | public function tearDown() { |
||
| 484 | |||
| 485 | } |
||
| 486 | |||
| 490 |
Loading history...
Since your code implements the magic setter
_set, this function will be called for any write access on an undefined variable. You can add the@propertyannotation to your class or interface to document the existence of this variable.Since the property has write access only, you can use the @property-write annotation instead.
Of course, you may also just have mistyped another name, in which case you should fix the error.
See also the PhpDoc documentation for @property.