diff --git a/docs/en/04_Changelogs/4.0.0.md b/docs/en/04_Changelogs/4.0.0.md index dc5953aa0..b8e83871d 100644 --- a/docs/en/04_Changelogs/4.0.0.md +++ b/docs/en/04_Changelogs/4.0.0.md @@ -808,6 +808,7 @@ specific functions. * `Injector::load` given a `src` parameter will no longer guess the service name from the filename. Now the service name will either by the array key, or the `class` parameter value. +* Uniqueness checks for `File.Name` is performed on write only (not in `setName()`) #### General and Core Removed API diff --git a/src/Assets/File.php b/src/Assets/File.php index 8ca2bcb5a..904f9dd7a 100644 --- a/src/Assets/File.php +++ b/src/Assets/File.php @@ -10,6 +10,7 @@ use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Core\Convert; use SilverStripe\Control\Director; use SilverStripe\Control\Controller; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Dev\Deprecation; use SilverStripe\Forms\DatetimeField; use SilverStripe\Forms\FieldList; @@ -639,12 +640,65 @@ class File extends DataObject implements ShortcodeHandler, AssetContainer, Thumb $this->OwnerID = Member::currentUserID(); } - // Set default name - if (!$this->getField('Name')) { - $this->Name ="new-" . strtolower($this->class); + $name = $this->getField('Name'); + $title = $this->getField('Title'); + + $changed = $this->isChanged('Name'); + + // Name can't be blank, default to Title + if (!$name) { + $changed = true; + $name = $title; } - // Propegate changes to the AssetStore and update the DBFile field + $filter = FileNameFilter::create(); + if ($name) { + // Fix illegal characters + $name = $filter->filter($name); + } else { + // Default to file name + $changed = true; + $name = $this->i18n_singular_name(); + $name = $filter->filter($name); + } + + // Check for duplicates when the name has changed (or is set for the first time) + if ($changed) { + $nameGenerator = $this->getNameGenerator($name); + // Defaults to returning the original filename on first iteration + foreach ($nameGenerator as $newName) { + // This logic is also used in the Folder subclass, but we're querying + // for duplicates on the File base class here (including the Folder subclass). + + // TODO Add read lock to avoid other processes creating files with the same name + // before this process has a chance to persist in the database. + $existingFile = File::get()->filter(array( + 'Name' => $newName, + 'ParentID' => (int) $this->ParentID + ))->exclude(array( + 'ID' => $this->ID + ))->first(); + if (!$existingFile) { + $name = $newName; + break; + } + } + } + + // Update actual field value + $this->setField('Name', $name); + + // Update title + if (!$title) { + // Generate a readable title, dashes and underscores replaced by whitespace, + // and any file extensions removed. + $this->setField( + 'Title', + str_replace(array('-','_'), ' ', preg_replace('/\.[^.]+$/', '', $name)) + ); + } + + // Propagate changes to the AssetStore and update the DBFile field $this->updateFilesystem(); parent::onBeforeWrite(); @@ -715,62 +769,14 @@ class File extends DataObject implements ShortcodeHandler, AssetContainer, Thumb } /** - * Setter function for Name. Automatically sets a default title, - * and removes characters that might be invalid on the filesystem. - * Also adds a suffix to the name if the filename already exists - * on the filesystem, and is associated to a different {@link File} database record - * in the same folder. This means "myfile.jpg" might become "myfile-1.jpg". + * Get an asset renamer for the given filename. * - * Does not change the filesystem itself, please use {@link write()} for this. - * - * @param string $name - * @return $this + * @param string $filename Path name + * @return AssetNameGenerator */ - public function setName($name) + protected function getNameGenerator($filename) { - $oldName = $this->Name; - - // It can't be blank, default to Title - if (!$name) { - $name = $this->Title; - } - - // Fix illegal characters - $filter = FileNameFilter::create(); - $name = $filter->filter($name); - - // We might have just turned it blank, so check again. - if (!$name) { - $name = 'new-folder'; - } - - // If it's changed, check for duplicates - if ($oldName && $oldName != $name) { - $base = pathinfo($name, PATHINFO_FILENAME); - $ext = self::get_file_extension($name); - $suffix = 1; - - while (File::get()->filter(array( - 'Name' => $name, - 'ParentID' => (int) $this->ParentID - ))->exclude(array( - 'ID' => $this->ID - ))->first() - ) { - $suffix++; - $name = "$base-$suffix.$ext"; - } - } - - // Update actual field value - $this->setField('Name', $name); - - // Update title - if (!$this->Title) { - $this->Title = str_replace(array('-','_'), ' ', preg_replace('/\.[^.]+$/', '', $name)); - } - - return $this; + return Injector::inst()->createWithArgs('AssetNameGenerator', array($filename)); } /** diff --git a/src/Assets/FileNameFilter.php b/src/Assets/FileNameFilter.php index 756b4e5ea..c73572e7d 100644 --- a/src/Assets/FileNameFilter.php +++ b/src/Assets/FileNameFilter.php @@ -77,7 +77,8 @@ class FileNameFilter extends Object // Safeguard against empty file names $nameWithoutExt = pathinfo($name, PATHINFO_FILENAME); if (empty($nameWithoutExt)) { - $name = $this->getDefaultName() . '.' . $ext; + $name = $this->getDefaultName(); + $name .= $ext ? '.' . $ext : ''; } return $name; diff --git a/src/Assets/Folder.php b/src/Assets/Folder.php index 8502464c8..9bd0b4e2b 100644 --- a/src/Assets/Folder.php +++ b/src/Assets/Folder.php @@ -129,7 +129,9 @@ class Folder extends File */ public function setTitle($title) { - $this->setName($title); + $this->setField('Title', $title); + $this->setField('Name', $title); + return $this; } @@ -143,21 +145,6 @@ class Folder extends File return $this->Name; } - /** - * Override setting the Title of Folders to that Name and Title are always in sync. - * Note that this is not appropriate for files, because someone might want to create a human-readable name - * of a file that is different from its name on disk. But folders should always match their name on disk. - * - * @param string $name - * @return $this - */ - public function setName($name) - { - parent::setName($name); - $this->setField('Title', $this->Name); - return $this; - } - /** * A folder doesn't have a (meaningful) file size. * diff --git a/tests/php/Assets/FileNameFilterTest.php b/tests/php/Assets/FileNameFilterTest.php index 04689ce40..bd4261f27 100644 --- a/tests/php/Assets/FileNameFilterTest.php +++ b/tests/php/Assets/FileNameFilterTest.php @@ -142,4 +142,11 @@ class FileNameFilterTest extends SapphireTest $filter = new FileNameFilter(); $this->assertEquals('test-document.txt', $filter->filter($name)); } + + public function testDoesntAddExtensionWhenMissing() + { + $name = 'no-extension'; + $filter = new FileNameFilter(); + $this->assertEquals('no-extension', $filter->filter($name)); + } } diff --git a/tests/php/Assets/FileTest.php b/tests/php/Assets/FileTest.php index dfb5a021a..b38e6770c 100644 --- a/tests/php/Assets/FileTest.php +++ b/tests/php/Assets/FileTest.php @@ -535,6 +535,88 @@ class FileTest extends SapphireTest ); } + public function testRenamesDuplicateFilesInSameFolder() + { + $original = new File(); + $original->update([ + 'Name' => 'file1.txt', + 'ParentID' => 0 + ]); + $original->write(); + + $duplicate = new File(); + $duplicate->update([ + 'Name' => 'file1.txt', + 'ParentID' => 0 + ]); + $duplicate->write(); + + $original = File::get()->byID($original->ID); + + $this->assertEquals($original->Name, 'file1.txt'); + $this->assertEquals($original->Title, 'file1'); + $this->assertEquals($duplicate->Name, 'file1-v2.txt'); + $this->assertEquals($duplicate->Title, 'file1 v2'); + } + + public function testSetsEmptyTitleToNameWithoutExtensionAndSpecialCharacters() + { + $fileWithTitle = new File(); + $fileWithTitle->update([ + 'Name' => 'file1-with-title.txt', + 'Title' => 'Some Title' + ]); + $fileWithTitle->write(); + + $this->assertEquals($fileWithTitle->Name, 'file1-with-title.txt'); + $this->assertEquals($fileWithTitle->Title, 'Some Title'); + + $fileWithoutTitle = new File(); + $fileWithoutTitle->update([ + 'Name' => 'file1-without-title.txt', + ]); + $fileWithoutTitle->write(); + + $this->assertEquals($fileWithoutTitle->Name, 'file1-without-title.txt'); + $this->assertEquals($fileWithoutTitle->Title, 'file1 without title'); + } + + public function testSetsEmptyNameToSingularNameWithoutTitle() + { + $fileWithTitle = new File(); + $fileWithTitle->update([ + 'Name' => '', + 'Title' => 'Some Title', + ]); + $fileWithTitle->write(); + + $this->assertEquals($fileWithTitle->Name, 'Some-Title'); + $this->assertEquals($fileWithTitle->Title, 'Some Title'); + + $fileWithoutTitle = new File(); + $fileWithoutTitle->update([ + 'Name' => '', + 'Title' => '', + ]); + $fileWithoutTitle->write(); + + $this->assertEquals($fileWithoutTitle->Name, $fileWithoutTitle->i18n_singular_name()); + $this->assertEquals($fileWithoutTitle->Title, $fileWithoutTitle->i18n_singular_name()); + } + + public function testSetsEmptyNameToTitleIfPresent() + { + $file = new File(); + $file->update([ + 'Name' => '', + 'Title' => 'file1', + ]); + $file->write(); + + $this->assertEquals($file->Name, 'file1'); + $this->assertEquals($file->Title, 'file1'); + } + public function testSetsOwnerOnFirstWrite() { Session::set('loggedInAs', null); diff --git a/tests/php/Assets/FolderTest.php b/tests/php/Assets/FolderTest.php index 8becbc83d..ef1aedc2f 100644 --- a/tests/php/Assets/FolderTest.php +++ b/tests/php/Assets/FolderTest.php @@ -76,6 +76,30 @@ class FolderTest extends SapphireTest $this->assertEquals($folder1->Filename . 'CreateFromNameAndParentID/', $newFolder->Filename); } + public function testRenamesDuplicateFolders() + { + $original = new Folder(); + $original->update([ + 'Name' => 'folder1', + 'ParentID' => 0 + ]); + $original->write(); + + $duplicate = new Folder(); + $duplicate->update([ + 'Name' => 'folder1', + 'ParentID' => 0 + ]); + $duplicate->write(); + + $original = Folder::get()->byID($original->ID); + + $this->assertEquals($original->Name, 'folder1'); + $this->assertEquals($original->Title, 'folder1'); + $this->assertEquals($duplicate->Name, 'folder1-v2'); + $this->assertEquals($duplicate->Title, 'folder1-v2'); + } + public function testAllChildrenIncludesFolders() { $folder1 = $this->objFromFixture(Folder::class, 'folder1'); @@ -255,14 +279,18 @@ class FolderTest extends SapphireTest $newFolder->Name = 'TestNameCopiedToTitle'; $this->assertEquals($newFolder->Name, $newFolder->Title); + $this->assertEquals($newFolder->Title, 'TestNameCopiedToTitle'); $newFolder->Title = 'TestTitleCopiedToName'; $this->assertEquals($newFolder->Name, $newFolder->Title); + $this->assertEquals($newFolder->Title, 'TestTitleCopiedToName'); $newFolder->Name = 'TestNameWithIllegalCharactersCopiedToTitle '; $this->assertEquals($newFolder->Name, $newFolder->Title); + $this->assertEquals($newFolder->Title, 'TestNameWithIllegalCharactersCopiedToTitle '); $newFolder->Title = 'TestTitleWithIllegalCharactersCopiedToName '; $this->assertEquals($newFolder->Name, $newFolder->Title); + $this->assertEquals($newFolder->Title, 'TestTitleWithIllegalCharactersCopiedToName '); } } diff --git a/tests/php/Assets/Storage/DefaultAssetNameGeneratorTest.php b/tests/php/Assets/Storage/DefaultAssetNameGeneratorTest.php index d3abda367..2427064ed 100644 --- a/tests/php/Assets/Storage/DefaultAssetNameGeneratorTest.php +++ b/tests/php/Assets/Storage/DefaultAssetNameGeneratorTest.php @@ -104,4 +104,32 @@ class DefaultAssetNameGeneratorTest extends SapphireTest $this->assertEquals('folder/MyFile-v99.jpg', $suggestions[98]); $this->assertNotEquals('folder/MyFile-v100.jpg', $suggestions[99]); } + + public function testFolderWithoutDefaultPrefix() + { + Config::inst()->update(DefaultAssetNameGenerator::class, 'version_prefix', ''); + $generator = new DefaultAssetNameGenerator('folder/subfolder'); + $suggestions = iterator_to_array($generator); + + // Expect 100 suggestions + $this->assertEquals(100, count($suggestions)); + + // First item is always the same as input + $this->assertEquals('folder/subfolder', $suggestions[0]); + $this->assertEquals('folder/subfolder2', $suggestions[1]); + } + + public function testFolderWithDefaultPrefix() + { + Config::inst()->update(DefaultAssetNameGenerator::class, 'version_prefix', '-v'); + $generator = new DefaultAssetNameGenerator('folder/subfolder'); + $suggestions = iterator_to_array($generator); + + // Expect 100 suggestions + $this->assertEquals(100, count($suggestions)); + + // First item is always the same as input + $this->assertEquals('folder/subfolder', $suggestions[0]); + $this->assertEquals('folder/subfolder-v2', $suggestions[1]); + } }