[HttpFoundation] cleaned up comments and coding standards, added better exception messages when filesystem functions throw errors, added "moved" arg to UploadedFile constructor, added FileNotFoundException to UploadedFile per the parent constructor

src/Symfony/Component/HttpFoundation/File/File.php

@@ -16,14 +16,15 @@ use Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException;
 use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser;
 
 /**
- * A file in the file system
+ * A file in the file system.
  *
  * @author Bernhard Schussek <bernhard.schussek@symfony.com>
  */
 class File
 {
     /**
-     * Assignment of mime types to their default extensions
+     * A map of mime types and their default extensions.
+     *
      * @var array
      */
     static protected $defaultExtensions = array(
@@ -440,33 +441,35 @@ class File
     );
 
     /**
-     * Stores the absolute path to the document root directory
+     * Stores the absolute path to the document root directory.
+     *
      * @var string
      */
     static protected $documentRoot;
 
     /**
-     * The absolute path to the file without dots
+     * The absolute path to the file without dots.
+     *
      * @var string
      */
     protected $path;
 
     /**
-     * Sets the path t the document root directory
+     * Sets the path to the document root directory.
      *
      * @param string $documentRoot
      */
     static public function setDocumentRoot($documentRoot)
     {
         if (!is_dir($documentRoot)) {
-            throw new \LogicException($documentRoot . ' is no directory');
+            throw new \LogicException($documentRoot . ' is not a directory.');
         }
 
         self::$documentRoot = realpath($documentRoot);
     }
 
     /**
-     * Returns the path to the document root directory
+     * Returns the path to the document root directory.
      *
      * @return string
      */
@@ -478,8 +481,9 @@ class File
     /**
      * Constructs a new file from the given path.
      *
-     * @param  string $path           The path to the file
-     * @throws FileNotFoundException  If the given path is no file
+     * @param string $path The path to the file
+     *
+     * @throws FileNotFoundException If the given path is not a file
      */
     public function __construct($path)
     {
@@ -491,7 +495,7 @@ class File
     }
 
     /**
-     * Alias for getPath()
+     * Alias for getPath().
      *
      * @return string
      */
@@ -501,7 +505,7 @@ class File
     }
 
     /**
-     * Returns the file name
+     * Returns the file name.
      *
      * @return string
      */
@@ -511,13 +515,13 @@ class File
     }
 
     /**
-     * Returns the file extension (with dot)
+     * Returns the file extension (with dot).
      *
      * @return string
      */
     public function getExtension()
     {
-        if ($ext = pathinfo($this->getName(), \PATHINFO_EXTENSION)) {
+        if ($ext = pathinfo($this->getName(), PATHINFO_EXTENSION)) {
             return '.' . $ext;
         }
 
@@ -525,7 +529,7 @@ class File
     }
 
     /**
-     * Returns the extension based on the mime type (with dot)
+     * Returns the extension based on the mime type (with dot).
      *
      * If the mime type is unknown, the actual extension is returned instead.
      *
@@ -543,7 +547,7 @@ class File
     }
 
     /**
-     * Returns the directory of the file
+     * Returns the directory of the file.
      *
      * @return string
      */
@@ -553,9 +557,9 @@ class File
     }
 
     /**
-     * Returns the absolute file path without dots
+     * Returns the absolute file path, without dots.
      *
-     * @returns string  The file path
+     * @return string
      */
     public function getPath()
     {
@@ -563,19 +567,19 @@ class File
     }
 
     /**
-     * Returns the path relative to the document root
+     * Returns the path relative to the document root.
      *
      * You can set the document root using the static method setDocumentRoot().
      * If the file is outside of the document root, this method returns an
      * empty string.
      *
-     * @return string  The relative file path
+     * @return string The relative file path
      */
     public function getWebPath()
     {
         $root = self::$documentRoot;
 
-        if (strpos($this->path, $root) === false) {
+        if (false === strpos($this->path, $root)) {
             return '';
         }
 
@@ -589,7 +593,7 @@ class File
      * and the system binary "file" (in this order), depending on which of those
      * is available on the current operating system.
      *
-     * @returns string  The guessed mime type, e.g. "application/pdf"
+     * @return string The guessed mime type (i.e. "application/pdf")
      */
     public function getMimeType()
     {
@@ -599,14 +603,15 @@ class File
     }
 
     /**
-     * Returns the size of this file
+     * Returns the size of this file.
      *
-     * @return integer  The file size in bytes
+     * @return integer The file size in bytes
      */
-    public function size()
+    public function getSize()
     {
-        if (false === ($size = @filesize($this->getPath()))) {
-            throw new FileException(sprintf('Could not read file size of %s', $this->getPath()));
+        if (false === $size = @filesize($this->getPath())) {
+            $error = error_get_last();
+            throw new FileException(sprintf('Could not read file size of %s (%s)', $this->getPath(), $error['message']));
         }
 
         return $size;
@@ -615,16 +620,18 @@ class File
     /**
      * Moves the file to a new directory and gives it a new filename
      *
-     * @param  string $directory   The new directory
-     * @param  string $filename    The new file name
-     * @throws FileException       When the file could not be moved
+     * @param string $directory The new directory
+     * @param string $filename  The new file name
+     *
+     * @throws FileException When the file could not be moved
      */
     protected function doMove($directory, $filename)
     {
         $newPath = $directory . DIRECTORY_SEPARATOR . $filename;
 
         if (!@rename($this->getPath(), $newPath)) {
-            throw new FileException(sprintf('Could not move file %s to %s', $this->getPath(), $newPath));
+            $error = error_get_last();
+            throw new FileException(sprintf('Could not move file %s to %s (%s)', $this->getPath(), $newPath, $error['message']));
         }
 
         $this->path = realpath($newPath);
@@ -633,8 +640,8 @@ class File
     /**
      * Moves the file to a new location.
      *
-     * @param string $directory   The destination folder
-     * @param string $name        The new file name
+     * @param string $directory The destination folder
+     * @param string $name      The new file name
      */
     public function move($directory, $name = null)
     {
@@ -646,12 +653,12 @@ class File
     }
 
     /**
-     * Renames the file
+     * Renames the file.
      *
-     * @param string $name  The new file name
+     * @param string $name The new file name
      */
     public function rename($name)
     {
         $this->doMove($this->getDirectory(), $name);
     }
-}
+}

src/Symfony/Component/HttpFoundation/File/UploadedFile.php

@@ -12,78 +12,80 @@
 namespace Symfony\Component\HttpFoundation\File;
 
 use Symfony\Component\HttpFoundation\File\Exception\FileException;
+use Symfony\Component\HttpFoundation\File\Exception\FileNotFoundException;
 
 /**
  * A file uploaded through a form.
  *
- * @author     Bernhard Schussek <bernhard.schussek@symfony.com>
- * @author     Florian Eckerstorfer <florian@eckerstorfer.org>
+ * @author Bernhard Schussek <bernhard.schussek@symfony.com>
+ * @author Florian Eckerstorfer <florian@eckerstorfer.org>
  */
 class UploadedFile extends File
 {
     /**
-     * The original name of the uploaded file
+     * The original name of the uploaded file.
+     *
      * @var string
      */
     protected $originalName;
 
     /**
-     * The mime type provided by the uploader
+     * The mime type provided by the uploader.
+     *
      * @var string
      */
     protected $mimeType;
 
     /**
-     * The file size provided by the uploader
+     * The file size provided by the uploader.
+     *
      * @var integer
      */
     protected $size;
 
     /**
-     * The UPLOAD_ERR_XXX constant provided by the uploader
+     * The UPLOAD_ERR_XXX constant provided by the uploader.
+     *
      * @var integer
      */
     protected $error;
 
     /**
-     * Whether the uploaded file has already been moved
-     * @var boolean
+     * Whether the uploaded file has already been moved.
+     *
+     * @var Boolean
      */
-    protected $moved = false;
+    protected $moved;
 
     /**
-     * Accepts the information of the uploaded file as provided by the PHP
-     * global $_FILES.
+     * Accepts the information of the uploaded file as provided by the PHP global $_FILES.
+     *
+     * @param string  $path         The full temporary path to the file
+     * @param string  $originalName The original file name
+     * @param string  $mimeType     The type of the file as provided by PHP
+     * @param integer $size         The file size
+     * @param integer $error        The error constant of the upload (one of PHP's UPLOAD_ERR_XXX constants)
+     * @param Boolean $moved        Whether the file has been moved from its original location
      *
-     * @param string  $tmpName  The full temporary path to the file
-     * @param string  $name     The original file name
-     * @param string  $type     The type of the file as provided by PHP
-     * @param integer $size     The file size
-     * @param string  $error    The error constant of the upload. Should be
-     *                          one of PHP's UPLOAD_ERR_XXX constants.
+     * @throws FileException         If file_uploads is disabled
+     * @throws FileNotFoundException If the file does not exist
      */
-    public function __construct($path, $originalName, $mimeType, $size, $error)
+    public function __construct($path, $originalName, $mimeType, $size, $error, $moved = false)
     {
         if (!ini_get('file_uploads')) {
             throw new FileException(sprintf('Unable to create UploadedFile because "file_uploads" is disabled in your php.ini file (%s)', get_cfg_var('cfg_file_path')));
         }
 
-        if (is_file($path)) {
-            $this->path = realpath($path);
+        if (!is_file($path)) {
+            throw new FileNotFoundException($path);
         }
 
-        if (null === $error) {
-            $error = UPLOAD_ERR_OK;
-        }
-
-        if (null === $mimeType) {
-            $mimeType = 'application/octet-stream';
-        }
-
-        $this->originalName = (string)$originalName;
-        $this->mimeType = $mimeType;
+        $this->path = realpath($path);
+        $this->originalName = $originalName;
+        $this->mimeType = $mimeType ?: 'application/octet-stream';
         $this->size = $size;
-        $this->error = $error;
+        $this->error = $error ?: UPLOAD_ERR_OK;
+        $this->moved = (Boolean) $moved;
     }
 
     /**
@@ -91,28 +93,25 @@ class UploadedFile extends File
      */
     public function getMimeType()
     {
-        $mimeType = parent::getMimeType();
-
-        if (null === $mimeType) {
-            $mimeType = $this->mimeType;
-        }
+        return parent::getMimeType() ?: $this->mimeType;
+    }
 
-        return $mimeType;
+    /**
+     * @inheritDoc
+     */
+    public function getSize()
+    {
+        return null === $this->size ? parent::getSize() : $this->size;
     }
 
     /**
-     * Returns the absolute file name without dots
-     *
-     * Until the uploaded file is moved, it will return the name of the temporary file
+     * Returns the absolute file name without dots.
      *
-     * @returns string  The file path
+     * @return string The file path
      */
     public function getName()
     {
-        if (!$this->moved) {
-            return $this->originalName;
-        }
-        return parent::getName();
+        return $this->moved ? parent::getName() : $this->originalName;
     }
 
     /**
@@ -121,7 +120,7 @@ class UploadedFile extends File
      * If the upload was successful, the constant UPLOAD_ERR_OK is returned.
      * Otherwise one of the other UPLOAD_ERR_XXX constants is returned.
      *
-     * @returns integer  The upload error
+     * @return integer The upload error
      */
     public function getError()
     {
@@ -133,18 +132,18 @@ class UploadedFile extends File
      */
     protected function doMove($directory, $filename)
     {
-        if (!$this->moved) {
-            $newPath = $directory . DIRECTORY_SEPARATOR . $filename;
+        if ($this->moved) {
+            return parent::doMove($directory, $filename);
+        }
 
-            if (!move_uploaded_file($this->getPath(), $newPath)) {
-                throw new FileException(sprintf('Could not move file %s to %s', $this->getPath(), $newPath));
-            }
+        $newPath = $directory . DIRECTORY_SEPARATOR . $filename;
 
-            $this->moved = true;
-            $this->path = realpath($newPath);
-        } else {
-            parent::doMove($directory, $filename);
+        if (!move_uploaded_file($this->getPath(), $newPath)) {
+            throw new FileException(sprintf('Could not move file %s to %s', $this->getPath(), $newPath));
         }
+
+        $this->moved = true;
+        $this->path = realpath($newPath);
     }
 
     /**
@@ -152,14 +151,14 @@ class UploadedFile extends File
      */
     public function move($directory, $name = null)
     {
-        if (!$this->moved) {
-            $this->doMove($directory, $this->originalName);
-
-            if (null !== $name) {
-                $this->rename($name);
-            }
-        } else {
-            parent::move($directory, $name);
+        if ($this->moved) {
+            return parent::move($directory, $name);
+        }
+
+        $this->doMove($directory, $this->originalName);
+
+        if (null !== $name) {
+            $this->rename($name);
         }
     }
-}
+}

src/Symfony/Component/HttpFoundation/FileBag.php

@@ -21,7 +21,7 @@ use Symfony\Component\HttpFoundation\File\UploadedFile;
  */
 class FileBag extends ParameterBag
 {
-    private $fileKeys = array('error', 'name', 'size', 'tmp_name', 'type');
+    static private $fileKeys = array('error', 'name', 'size', 'tmp_name', 'type');
 
     /**
      * Constructor.
@@ -77,24 +77,23 @@ class FileBag extends ParameterBag
         if ($file instanceof UploadedFile) {
             return $file;
         }
+
         $file = $this->fixPhpFilesArray($file);
         if (is_array($file)) {
             $keys = array_keys($file);
             sort($keys);
-            if ($keys == $this->fileKeys) {
-                $file['error'] = (int) $file['error'];
-            }
-            if ($keys != $this->fileKeys) {
-                $file = array_map(array($this, 'convertFileInformation'), $file);
-            } else {
-                if ($file['error'] === UPLOAD_ERR_NO_FILE) {
+
+            if ($keys == self::$fileKeys) {
+                if (UPLOAD_ERR_NO_FILE == $file['error']) {
                     $file = null;
                 } else {
-                    $file = new UploadedFile($file['tmp_name'], $file['name'],
-                    $file['type'], $file['size'], $file['error']);
+                    $file = new UploadedFile($file['tmp_name'], $file['name'], $file['type'], $file['size'], $file['error']);
                 }
+            } else {
+                $file = array_map(array($this, 'convertFileInformation'), $file);
             }
         }
+
         return $file;
     }
 
@@ -115,19 +114,22 @@ class FileBag extends ParameterBag
      */
     protected function fixPhpFilesArray($data)
     {
-        if (! is_array($data)) {
+        if (!is_array($data)) {
             return $data;
         }
+
         $keys = array_keys($data);
         sort($keys);
-        if ($this->fileKeys != $keys || ! isset($data['name']) ||
-         ! is_array($data['name'])) {
+
+        if (self::$fileKeys != $keys || !isset($data['name']) || !is_array($data['name'])) {
             return $data;
         }
+
         $files = $data;
-        foreach ($this->fileKeys as $k) {
+        foreach (self::$fileKeys as $k) {
             unset($files[$k]);
         }
+
         foreach (array_keys($data['name']) as $key) {
             $files[$key] = $this->fixPhpFilesArray(array(
                 'error'    => $data['error'][$key],
@@ -136,6 +138,7 @@ class FileBag extends ParameterBag
                 'size'     => $data['size'][$key]
             ));
         }
+
         return $files;
     }
 }

tests/Symfony/Tests/Component/HttpFoundation/File/FileTest.php

@@ -111,11 +111,13 @@ class FileTest extends \PHPUnit_Framework_TestCase
 
     public function testSizeReturnsFileSize()
     {
-        $this->assertEquals(filesize($this->file->getPath()), $this->file->size());
+        $this->assertEquals(filesize($this->file->getPath()), $this->file->getSize());
     }
 
     public function testSizeFailing()
     {
+        $this->setExpectedException('Symfony\Component\HttpFoundation\File\Exception\FileException');
+
         $dir = __DIR__.DIRECTORY_SEPARATOR.'Fixtures'.DIRECTORY_SEPARATOR.'directory';
         $path = $dir.DIRECTORY_SEPARATOR.'test.copy.gif';
         @unlink($path);
@@ -123,10 +125,7 @@ class FileTest extends \PHPUnit_Framework_TestCase
 
         $file = new File($path);
         @unlink($path);
-
-        $this->setExpectedException('Symfony\Component\HttpFoundation\File\Exception\FileException');
-        $file->size($path);
-
+        $file->getSize();
     }
 
     public function testMove()

tests/Symfony/Tests/Component/HttpFoundation/File/UploadedFileTest.php

@@ -15,87 +15,64 @@ use Symfony\Component\HttpFoundation\File\UploadedFile;
 
 class UploadedFileTest extends \PHPUnit_Framework_TestCase
 {
-
-    public function testFileUploadsMustBeEnabled()
+    protected function setUp()
     {
-        // we can't change this setting without modifying php.ini :(
         if (!ini_get('file_uploads')) {
-            $this->setExpectedException('Symfony\Component\HttpFoundation\File\Exception\FileException');
-
-            new UploadedFile(
-                __DIR__.'/Fixtures/test.gif',
-                'original.gif',
-                'image/gif',
-                filesize(__DIR__.'/Fixtures/test.gif'),
-                UPLOAD_ERR_OK
-            );
+            $this->markTestSkipped('file_uploads is disabled in php.ini');
         }
     }
 
     public function testFileUploadsWithNoMimeType()
     {
-        // we can't change this setting without modifying php.ini :(
-        if (ini_get('file_uploads')) {
-
-            $file = new UploadedFile(
-                __DIR__.'/Fixtures/test.gif',
-                'original.gif',
-                null,
-                filesize(__DIR__.'/Fixtures/test.gif'),
-                UPLOAD_ERR_OK
-            );
+        $file = new UploadedFile(
+            __DIR__.'/Fixtures/test.gif',
+            'original.gif',
+            null,
+            filesize(__DIR__.'/Fixtures/test.gif'),
+            UPLOAD_ERR_OK
+        );
 
-            $this->assertAttributeEquals('application/octet-stream', 'mimeType', $file);
-            $this->assertEquals('image/gif', $file->getMimeType());
-        }
+        $this->assertAttributeEquals('application/octet-stream', 'mimeType', $file);
+        $this->assertEquals('image/gif', $file->getMimeType());
     }
 
     public function testFileUploadsWithUnknownMimeType()
     {
-        // we can't change this setting without modifying php.ini :(
-        if (ini_get('file_uploads')) {
-
-            $file = new UploadedFile(
-                __DIR__.'/Fixtures/.unknownextension',
-                'original.gif',
-                null,
-                filesize(__DIR__.'/Fixtures/.unknownextension'),
-                UPLOAD_ERR_OK
-            );
+        $file = new UploadedFile(
+            __DIR__.'/Fixtures/.unknownextension',
+            'original.gif',
+            null,
+            filesize(__DIR__.'/Fixtures/.unknownextension'),
+            UPLOAD_ERR_OK
+        );
 
-            $this->assertAttributeEquals('application/octet-stream', 'mimeType', $file);
-            $this->assertEquals('application/octet-stream', $file->getMimeType());
-        }
+        $this->assertAttributeEquals('application/octet-stream', 'mimeType', $file);
+        $this->assertEquals('application/octet-stream', $file->getMimeType());
     }
 
     public function testErrorIsOkByDefault()
     {
-        // we can't change this setting without modifying php.ini :(
-        if (ini_get('file_uploads')) {
-            $file = new UploadedFile(
-                __DIR__.'/Fixtures/test.gif',
-                'original.gif',
-                'image/gif',
-                filesize(__DIR__.'/Fixtures/test.gif'),
-                null
-            );
+        $file = new UploadedFile(
+            __DIR__.'/Fixtures/test.gif',
+            'original.gif',
+            'image/gif',
+            filesize(__DIR__.'/Fixtures/test.gif'),
+            null
+        );
 
-            $this->assertEquals(UPLOAD_ERR_OK, $file->getError());
-        }
+        $this->assertEquals(UPLOAD_ERR_OK, $file->getError());
     }
+
     public function testGetOriginalName()
     {
-        // we can't change this setting without modifying php.ini :(
-        if (ini_get('file_uploads')) {
-            $file = new UploadedFile(
-                __DIR__.'/Fixtures/test.gif',
-                'original.gif',
-                'image/gif',
-                filesize(__DIR__.'/Fixtures/test.gif'),
-                null
-            );
+        $file = new UploadedFile(
+            __DIR__.'/Fixtures/test.gif',
+            'original.gif',
+            'image/gif',
+            filesize(__DIR__.'/Fixtures/test.gif'),
+            null
+        );
 
-            $this->assertEquals('original.gif', $file->getName());
-        }
+        $this->assertEquals('original.gif', $file->getName());
     }
-}
+}