From b1d5e97a3d71f31a04fcbccef49912b6d1243c16 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Wed, 11 Jan 2017 16:58:59 +1300 Subject: [PATCH 1/2] Enforce unique Folder names, use AssetNameGenerator --- docs/en/04_Changelogs/4.0.0.md | 1 + src/Assets/File.php | 120 +++++++++--------- src/Assets/Folder.php | 19 +-- tests/php/Assets/FileTest.php | 82 ++++++++++++ tests/php/Assets/FolderTest.php | 28 ++++ .../Storage/DefaultAssetNameGeneratorTest.php | 28 ++++ 6 files changed, 205 insertions(+), 73 deletions(-) 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/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/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]); + } } From 3b06e30558cab4f4d732db38702ed26999acd3a8 Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Fri, 13 Jan 2017 10:40:44 +1300 Subject: [PATCH 2/2] =?UTF-8?q?Don=E2=80=99t=20add=20extension=20dot=20in?= =?UTF-8?q?=20FileNameFilter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit File names are generally valid without an extension (although they might be disallowed by upload constraints), so the filter should deal gracefully with them (“myfile” should return “myfile”, not “myfile.”) --- src/Assets/FileNameFilter.php | 3 ++- tests/php/Assets/FileNameFilterTest.php | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-) 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/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)); + } }