[BUGFIX] LocalDriver: Recursive file listing is broken 81/17881/11
authorAndreas Wolf <andreas.wolf@typo3.org>
Tue, 29 Jan 2013 14:11:24 +0000 (15:11 +0100)
committerSteffen Ritter <info@rs-websystems.de>
Thu, 4 Jul 2013 19:18:14 +0000 (21:18 +0200)
The recursive file listing, introduced as part of the public API in
the fix for #43249, is currently broken.

One problem is that the file information retrieval is incomplete: The
filename of files in subfolders also contains the path to this file from
the current folder, while the identifier is missing that information.
The identifier is thus wrong and the filename contains too much
information (more than the filename).

Additionally, the method getDirectoryItemList() returns the file list
with the filenames as key, which will fail when a file name exists twice
in different folders. Therefore, this patch changes the keys to numeric
values when a recursive folder list is requested.

Change-Id: Iaebd862327d2dfc849044236474f6da2444cd4f5
Resolves: #44910
Releases: 6.1, 6.0
Reviewed-on: https://review.typo3.org/17881
Reviewed-by: Steffen Ritter
Tested-by: Steffen Ritter
typo3/sysext/core/Classes/Resource/Driver/AbstractDriver.php
typo3/sysext/core/Classes/Resource/Driver/LocalDriver.php
typo3/sysext/core/Classes/Resource/Folder.php
typo3/sysext/core/Classes/Resource/ResourceStorage.php
typo3/sysext/core/Classes/Utility/ResourceUtility.php [new file with mode: 0644]
typo3/sysext/core/Tests/Unit/Resource/Driver/LocalDriverTest.php
typo3/sysext/core/Tests/Unit/Utility/ResourceUtilityTest.php [new file with mode: 0644]

index ce3276e..59d3c6d 100644 (file)
@@ -164,7 +164,7 @@ abstract class AbstractDriver {
         * Generic handler method for directory listings - gluing together the
         * listing items is done
         *
-        * @param string $path
+        * @param string $basePath
         * @param integer $start
         * @param integer $numberOfItems
         * @param array $filterMethods The filter methods used to filter the directory items
@@ -173,7 +173,7 @@ abstract class AbstractDriver {
         * @param boolean $recursive
         * @return array
         */
-       protected function getDirectoryItemList($path, $start, $numberOfItems, array $filterMethods, $itemHandlerMethod, $itemRows = array(), $recursive = FALSE) {
+       protected function getDirectoryItemList($basePath, $start, $numberOfItems, array $filterMethods, $itemHandlerMethod, $itemRows = array(), $recursive = FALSE) {
 
        }
 
index efffe40..a933362 100644 (file)
@@ -295,7 +295,7 @@ class LocalDriver extends \TYPO3\CMS\Core\Resource\Driver\AbstractDriver {
         * Generic wrapper for extracting a list of items from a path. The
         * extraction itself is done by the given handler method
         *
-        * @param string $path
+        * @param string $basePath
         * @param integer $start The position to start the listing; if not set, start from the beginning
         * @param integer $numberOfItems The number of items to list; if set to zero, all items are returned
         * @param array $filterMethods The filter methods used to filter the directory items
@@ -305,47 +305,65 @@ class LocalDriver extends \TYPO3\CMS\Core\Resource\Driver\AbstractDriver {
         * @return array
         */
        // TODO add unit tests
-       protected function getDirectoryItemList($path, $start, $numberOfItems, array $filterMethods, $itemHandlerMethod, $itemRows = array(), $recursive = FALSE) {
-               $realPath = rtrim(($this->absoluteBasePath . trim($path, '/')), '/') . '/';
+       protected function getDirectoryItemList($basePath, $start, $numberOfItems, array $filterMethods, $itemHandlerMethod, $itemRows = array(), $recursive = FALSE) {
+               $realPath = rtrim(($this->absoluteBasePath . trim($basePath, '/')), '/') . '/';
                if (!is_dir($realPath)) {
-                       throw new \InvalidArgumentException('Cannot list items in directory ' . $path . ' - does not exist or is no directory', 1314349666);
+                       throw new \InvalidArgumentException('Cannot list items in directory ' . $basePath . ' - does not exist or is no directory', 1314349666);
                }
+
                if ($start > 0) {
                        $start--;
                }
+
                // Fetch the files and folders and sort them by name; we have to do
                // this here because the directory iterator does return them in
                // an arbitrary order
                $items = $this->getFileAndFoldernamesInPath($realPath, $recursive);
-               natcasesort($items);
+               uksort(
+                       $items,
+                       array('\\TYPO3\\CMS\\Core\\Utility\\ResourceUtility', 'recursiveFileListSortingHelper')
+               );
+
                $iterator = new \ArrayIterator($items);
                if ($iterator->count() == 0) {
                        return array();
                }
                $iterator->seek($start);
-               if ($path !== '' && $path !== '/') {
-                       $path = '/' . trim($path, '/') . '/';
+
+               if ($basePath !== '' && $basePath !== '/') {
+                       $basePath = '/' . trim($basePath, '/') . '/';
                }
+
                // $c is the counter for how many items we still have to fetch (-1 is unlimited)
                $c = $numberOfItems > 0 ? $numberOfItems : -1;
                $items = array();
                while ($iterator->valid() && ($numberOfItems == 0 || $c > 0)) {
                        // $iteratorItem is the file or folder name
                        $iteratorItem = $iterator->current();
+
                        // go on to the next iterator item now as we might skip this one early
                        $iterator->next();
-                       $identifier = $path . $iteratorItem;
-                       if ($this->applyFilterMethodsToDirectoryItem($filterMethods, $iteratorItem, $identifier, $path) === FALSE) {
+                       $identifier = $basePath . $iteratorItem['path'];
+
+                       if ($this->applyFilterMethodsToDirectoryItem($filterMethods, $iteratorItem['name'], $identifier, dirname($identifier) . '/', isset($itemRows[$identifier]) ? array('indexData' => $itemRows[$identifier]) : array()) === FALSE) {
                                continue;
                        }
+
+                       // dirname returns "/" when called with "/" as the argument, so strip trailing slashes here to be sure
+                       $path = rtrim(dirname($identifier), '/') . '/';
                        if (isset($itemRows[$identifier])) {
-                               list($key, $item) = $this->{$itemHandlerMethod}($iteratorItem, $path, $itemRows[$identifier]);
+                               list($key, $item) = $this->{$itemHandlerMethod}($iteratorItem['name'], $path, $itemRows[$identifier]);
                        } else {
-                               list($key, $item) = $this->{$itemHandlerMethod}($iteratorItem, $path);
+                               list($key, $item) = $this->{$itemHandlerMethod}($iteratorItem['name'], $path);
                        }
+
                        if (empty($item)) {
                                continue;
                        }
+                       if ($recursive) {
+                               $key = $iteratorItem['path'];
+                       }
+
                        $items[$key] = $item;
                        // Decrement item counter to make sure we only return $numberOfItems
                        // we cannot do this earlier in the method (unlike moving the iterator forward) because we only add the
@@ -368,6 +386,7 @@ class LocalDriver extends \TYPO3\CMS\Core\Resource\Driver\AbstractDriver {
                if (!is_file($filePath)) {
                        return array('', array());
                }
+
                // TODO add unit test for existing file row case
                if (!empty($fileRow) && filemtime($filePath) <= $fileRow['modification_date']) {
                        return array($fileName, $fileRow);
@@ -386,15 +405,19 @@ class LocalDriver extends \TYPO3\CMS\Core\Resource\Driver\AbstractDriver {
         */
        protected function getFolderList_itemCallback($folderName, $parentPath, array $folderRow = array()) {
                $folderPath = $this->getAbsolutePath($parentPath . $folderName);
+
                if (!is_dir($folderPath)) {
                        return array('', array());
                }
+
                // also don't show hidden files
                if ($folderName === '..' || $folderName === '.' || $folderName === '') {
                        return array('', array());
                }
+
                // remove the trailing slash from the folder name (the trailing slash comes from the DirectoryIterator)
                $folderName = substr($folderName, 0, -1);
+
                return array($folderName, $this->extractFolderInformation($folderPath, $parentPath));
        }
 
@@ -412,6 +435,7 @@ class LocalDriver extends \TYPO3\CMS\Core\Resource\Driver\AbstractDriver {
                } else {
                        $iterator = new \RecursiveDirectoryIterator($path, \FilesystemIterator::CURRENT_AS_FILEINFO);
                }
+
                $directoryEntries = array();
                while ($iterator->valid()) {
                        /** @var $entry \SplFileInfo */
@@ -426,11 +450,19 @@ class LocalDriver extends \TYPO3\CMS\Core\Resource\Driver\AbstractDriver {
                                $iterator->next();
                                continue;
                        }
-                       $entryPath = GeneralUtility::fixWindowsFilePath(substr($entry->getPathname(), strlen($path)));
+                       $entryPath = substr($entry->getPathname(), strlen($path));
+                       $entryPath = GeneralUtility::fixWindowsFilePath($entryPath);
+                       $entryName = PathUtility::basename(basename($entryPath));
                        if ($entry->isDir()) {
                                $entryPath .= '/';
+                               $entryName .= '/';
                        }
-                       $directoryEntries[] = $entryPath;
+                       $entry = array(
+                               'path' => $entryPath,
+                               'name' => $entryName,
+                               'type' => $entry->isDir() ? 'dir' : 'file'
+                       );
+                       $directoryEntries[$entryPath] = $entry;
                        $iterator->next();
                }
                return $directoryEntries;
@@ -804,9 +836,9 @@ class LocalDriver extends \TYPO3\CMS\Core\Resource\Driver\AbstractDriver {
        protected function createIdentifierMap(array $filesAndFolders, $relativeSourcePath, $relativeTargetPath) {
                $identifierMap = array();
                $identifierMap[$relativeSourcePath] = $relativeTargetPath;
-               foreach ($filesAndFolders as $oldSubIdentifier) {
-                       $oldIdentifier = $relativeSourcePath . $oldSubIdentifier;
-                       $newIdentifier = $relativeTargetPath . $oldSubIdentifier;
+               foreach ($filesAndFolders as $oldItem) {
+                       $oldIdentifier = $relativeSourcePath . $oldItem['path'];
+                       $newIdentifier = $relativeTargetPath . $oldItem['path'];
                        if (!$this->resourceExists($newIdentifier)) {
                                throw new \TYPO3\CMS\Core\Resource\Exception\FileOperationErrorException(sprintf('File "%1$s" was not found (should have been copied/moved from "%2$s").', $newIdentifier, $oldIdentifier), 1330119453);
                        }
index 8f6532d..c747b4f 100644 (file)
@@ -198,7 +198,14 @@ class Folder implements FolderInterface {
                $fileArray = $this->storage->getFileList($this->identifier, $start, $numberOfItems, $useFilters, TRUE, $recursive);
                $fileObjects = array();
                foreach ($fileArray as $fileInfo) {
-                       $fileObjects[$fileInfo['name']] = $factory->createFileObject($fileInfo);
+                       $fileObject = $factory->createFileObject($fileInfo);
+
+                       // we might have duplicate filenames when fetching a recursive list, so don't use the filename as array key
+                       if ($recursive == TRUE) {
+                               $fileObjects[] = $fileObject;
+                       } else {
+                               $fileObjects[$fileInfo['name']] = $fileObject;
+                       }
                }
 
                $this->restoreBackedUpFiltersInStorage($backedUpFilters);
index f96987c..4b7d636 100644 (file)
@@ -879,7 +879,12 @@ class ResourceStorage {
                }
                $filters = $useFilters == TRUE ? $this->fileAndFolderNameFilters : array();
                $items = $this->driver->getFileList($path, $start, $numberOfItems, $filters, $rows, $recursive);
-               uksort($items, 'strnatcasecmp');
+
+               // We should not sort when fetching a recursive list, as these are indexed numerically
+               if ($recursive === FALSE) {
+                       uksort($items, 'strnatcasecmp');
+               }
+
                return $items;
        }
 
diff --git a/typo3/sysext/core/Classes/Utility/ResourceUtility.php b/typo3/sysext/core/Classes/Utility/ResourceUtility.php
new file mode 100644 (file)
index 0000000..5b1f925
--- /dev/null
@@ -0,0 +1,75 @@
+<?php
+namespace TYPO3\CMS\Core\Utility;
+
+/***************************************************************
+ *  Copyright notice
+ *
+ *  (c) 2013 Andreas Wolf <andreas.wolf@typo3.org>
+ *  All rights reserved
+ *
+ *  This script is part of the TYPO3 project. The TYPO3 project is
+ *  free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  The GNU General Public License can be found at
+ *  http://www.gnu.org/copyleft/gpl.html.
+ *
+ *  This script is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  This copyright notice MUST APPEAR in all copies of the script!
+ ***************************************************************/
+
+/**
+ * Utility class for the File Abstraction Layer (aka subpackage Resource in EXT:core)
+ */
+class ResourceUtility {
+       /**
+        * This is a helper method that can be used with u?sort methods to sort a list of (relative) file paths, e.g.
+        * array("someDir/fileA", "fileA", "fileB", "anotherDir/fileA").
+        *
+        * Directories are sorted first in the lists, with the deepest structures first (while every level is sorted
+        * alphabetically)
+        *
+        * @param string $elementA
+        * @param string $elementB
+        * @return int
+        */
+       public static function recursiveFileListSortingHelper($elementA, $elementB) {
+               if (strpos($elementA, '/') === FALSE) {
+                       // first element is a file
+                       if (strpos($elementB, '/') === FALSE) {
+                               $result = strnatcasecmp($elementA, $elementB);
+                       } else {
+                               // second element is a directory => always sort it first
+                               $result = 1;
+                       }
+               } else {
+                       // first element is a directory
+                       if (strpos($elementB, '/') === FALSE)  {
+                               // second element is a file => always sort it last
+                               $result = -1;
+                       } else {
+                               // both elements are directories => we have to recursively sort here
+                               list($pathPartA, $elementA) = explode('/', $elementA, 2);
+                               list($pathPartB, $elementB) = explode('/', $elementB, 2);
+
+                               if ($pathPartA === $pathPartB) {
+                                       // same directory => sort by subpaths
+                                       $result = self::recursiveFileListSortingHelper($elementA, $elementB);
+                               } else {
+                                       // different directories => sort by current directories
+                                       $result = strnatcasecmp($pathPartA, $pathPartB);
+                               }
+                       }
+               }
+
+               return $result;
+       }
+}
+
+?>
\ No newline at end of file
index 57ef049..5bf86a4 100644 (file)
@@ -741,6 +741,31 @@ class LocalDriverTest extends \TYPO3\CMS\Core\Tests\Unit\Resource\BaseTestCase {
        /**
         * @test
         */
+       public function getFileListReturnsAllFilesInSubdirectoryIfRecursiveParameterIsSet() {
+               $dirStructure = array(
+                       'aDir' => array(
+                               'file3' => 'asdfgh',
+                               'subdir' => array(
+                                       'file4' => 'asklfjklasjkl'
+                               )
+                       ),
+                       'file1' => 'asdfg',
+                       'file2' => 'fdsa'
+               );
+               $this->addToMount($dirStructure);
+               $fixture = $this->createDriverFixture(
+                       array('basePath' => $this->getMountRootUrl()),
+                       NULL,
+                               // Mocked because finfo() can not deal with vfs streams and throws warnings
+                       array('getMimeTypeOfFile')
+               );
+               $fileList = $fixture->getFileList('/', 0, 0, array(), array(), TRUE);
+               $this->assertEquals(array('aDir/subdir/file4', 'aDir/file3', 'file1', 'file2'), array_keys($fileList));
+       }
+
+       /**
+        * @test
+        */
        public function getFileListFailsIfDirectoryDoesNotExist() {
                $this->setExpectedException('InvalidArgumentException', '', 1314349666);
                $this->addToMount(array('somefile' => ''));
@@ -843,7 +868,9 @@ class LocalDriverTest extends \TYPO3\CMS\Core\Tests\Unit\Resource\BaseTestCase {
                $fixture = $this->createDriverFixture(array(
                        'basePath' => $this->getMountRootUrl()
                ));
+
                $fileList = $fixture->getFolderList('/');
+
                $this->assertEquals(array('.someHiddenDir', 'aDir'), array_keys($fileList));
        }
 
@@ -859,10 +886,10 @@ class LocalDriverTest extends \TYPO3\CMS\Core\Tests\Unit\Resource\BaseTestCase {
                $fixture = $this->createDriverFixture(array(
                        'basePath' => $this->getMountRootUrl()
                ));
-               $FolderList = $fixture->getFolderList('/');
-               $this->assertEquals('/dir1/', $FolderList['dir1']['identifier']);
-               $FolderList = $fixture->getFolderList('/dir1/');
-               $this->assertEquals('/dir1/subdir1/', $FolderList['subdir1']['identifier']);
+               $folderList = $fixture->getFolderList('/');
+               $this->assertEquals('/dir1/', $folderList['dir1']['identifier']);
+               $folderList = $fixture->getFolderList('/dir1/');
+               $this->assertEquals('/dir1/subdir1/', $folderList['subdir1']['identifier']);
        }
 
        /**
diff --git a/typo3/sysext/core/Tests/Unit/Utility/ResourceUtilityTest.php b/typo3/sysext/core/Tests/Unit/Utility/ResourceUtilityTest.php
new file mode 100644 (file)
index 0000000..0ce2545
--- /dev/null
@@ -0,0 +1,85 @@
+<?php
+namespace TYPO3\CMS\Core\Tests\Unit\Utility;
+
+/***************************************************************
+ * Copyright notice
+ *
+ * (c) 2013 Andreas Wolf <andreas.wolf@typo3.org>
+ * All rights reserved
+ *
+ * This script is part of the TYPO3 project. The TYPO3 project is
+ * free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * The GNU General Public License can be found at
+ * http://www.gnu.org/copyleft/gpl.html.
+ * A copy is found in the textfile GPL.txt and important notices to the license
+ * from the author is found in LICENSE.txt distributed with these scripts.
+ *
+ *
+ * This script is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This copyright notice MUST APPEAR in all copies of the script!
+ ***************************************************************/
+
+use TYPO3\CMS\Core\Utility\ResourceUtility;
+
+/**
+ * Testcase for class \TYPO3\CMS\Core\Utility\ResourceUtility
+ */
+class ResourceUtilityTest extends \TYPO3\CMS\Core\Tests\UnitTestCase {
+
+       public function recursiveFileListSortingHelperTestDataProvider() {
+               return array(
+                       'normal file list' => array(
+                               array('fileB', 'fileA', 'someFile'),
+                               array('fileA', 'fileB', 'someFile')
+                       ),
+                       'already in correct order' => array(
+                               array('fileA', 'fileB', 'someFile'),
+                               array('fileA', 'fileB', 'someFile')
+                       ),
+                       'hidden file' => array(
+                               array('someFile', '.hiddenFile'),
+                               array('.hiddenFile', 'someFile')
+                       ),
+                       'mixed capitalization' => array(
+                               array('alllower', 'allCAPS', 'ALLcaps', 'mIxedinanotherway', 'ALLCAPS', 'MiXeDcApItAlIzAtIoN'),
+                               array('ALLCAPS', 'ALLcaps', 'allCAPS', 'alllower', 'MiXeDcApItAlIzAtIoN', 'mIxedinanotherway')
+                       ),
+                       'recursive list with one sublevel' => array(
+                               array('fileA', 'fileB', 'anotherDir/someFile', 'someDir/someFile', 'anotherDir/anotherFile'),
+                               array('anotherDir/anotherFile', 'anotherDir/someFile', 'someDir/someFile', 'fileA', 'fileB')
+                       ),
+                       'recursive list with two sub-levels' => array(
+                               array('file', 'someDir/someFile', 'someDir/subdir/file', 'someDir/subdir/somefile', 'someDir/anotherDir/somefile', 'anotherDir/someFile'),
+                               array('anotherDir/someFile', 'someDir/anotherDir/somefile', 'someDir/subdir/file', 'someDir/subdir/somefile', 'someDir/someFile', 'file')
+                       ),
+                       'recursive list with three sub-levels' => array(
+                               array('someDir/someSubdir/file', 'someDir/someSubdir/someSubsubdir/someFile', 'someDir/someSubdir/someSubsubdir/anotherFile'),
+                               array('someDir/someSubdir/someSubsubdir/anotherFile', 'someDir/someSubdir/someSubsubdir/someFile', 'someDir/someSubdir/file')
+                       )
+               );
+       }
+
+       /**
+        * @dataProvider recursiveFileListSortingHelperTestDataProvider
+        * @test
+        */
+       public function recursiveFileListSortingHelperCorrectlySorts($unsortedList, $expectedList) {
+               $result = $unsortedList;
+               usort(
+                       $result,
+                       array('\\TYPO3\\CMS\\Core\\Utility\\ResourceUtility', 'recursiveFileListSortingHelper')
+               );
+
+               $this->assertEquals($expectedList, $result);
+       }
+}
+
+?>
\ No newline at end of file