Merge pull request #6484 from open-sausages/pulls/unique-folder-enforced

Enforce unique Folder names, use AssetNameGenerator

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()`)
 
 #### <a name="overview-general-removed"></a>General and Core Removed API
 

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
+        $name = $this->getField('Name');
-        if (!$this->getField('Name')) {
+        $title = $this->getField('Title');
-            $this->Name ="new-" . strtolower($this->class);
+
+        $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,
+     * Get an asset renamer for the given filename.
-     * 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".
      *
-     * Does not change the filesystem itself, please use {@link write()} for this.
+     * @param string $filename Path name
-     *
+     * @return AssetNameGenerator
-     * @param string $name
-     * @return $this
      */
-    public function setName($name)
+    protected function getNameGenerator($filename)
     {
-        $oldName = $this->Name;
+        return Injector::inst()->createWithArgs('AssetNameGenerator', array($filename));
-
-        // 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;
     }
 
     /**

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;

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.
      *

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));
+    }
 }

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);

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 <!BANG!>';
         $this->assertEquals($newFolder->Name, $newFolder->Title);
+        $this->assertEquals($newFolder->Title, 'TestNameWithIllegalCharactersCopiedToTitle <!BANG!>');
 
         $newFolder->Title = 'TestTitleWithIllegalCharactersCopiedToName <!BANG!>';
         $this->assertEquals($newFolder->Name, $newFolder->Title);
+        $this->assertEquals($newFolder->Title, 'TestTitleWithIllegalCharactersCopiedToName <!BANG!>');
     }
 }

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]);
+    }
 }