[Form] removed the file upload temporary storage feature

The current implementation is not ready for inclusion in 2.0. It has several
known problems (security, not possible to disable it, not "cloud-compatible",
...) and it's not a must have feature anyway.

Some references:

 * Security issue in FileType: https://github.com/symfony/symfony/issues/1001
 * Validation fails on file, still stored in TemporaryStorage: https://github.com/symfony/symfony/issues/908
 * Add a size argument & ability to configure TemporaryStorage: https://github.com/symfony/symfony/pull/748

This feature should be reworked and discussed for inclusion in 2.1.

UPDATE.md

@@ -9,6 +9,8 @@ timeline closely anyway.
 beta4 to beta5
 --------------
 
+* The temporary storage for file uploads has been removed
+
 * The `Symfony\Component\HttpFoundation\File\File::getExtension()` and
   `guessExtension()` methods do not return the extension with a `.` anymore.
 

src/Symfony/Bridge/Twig/Resources/views/Form/div_layout.html.twig

@@ -247,9 +247,6 @@
 {% spaceless %}
     <div {{ block('container_attributes') }}>
         {{ form_widget(form.file) }}
-        {{ form_widget(form.token) }}
-        {{ form_widget(form.name) }}
-        {{ form_widget(form.originalName) }}
     </div>
 {% endspaceless %}
 {% endblock file_widget %}

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -156,14 +156,6 @@ class FrameworkExtension extends Extension
             $container->setParameter('form.type_extension.csrf.enabled', $config['csrf_protection']['enabled']);
             $container->setParameter('form.type_extension.csrf.field_name', $config['csrf_protection']['field_name']);
         }
-
-        if ($container->hasDefinition('session')) {
-            $container->removeDefinition('file.temporary_storage');
-            $container->setDefinition('file.temporary_storage', $container->getDefinition('file.temporary_storage.session'));
-            $container->removeDefinition('file.temporary_storage.session');
-        } else {
-            $container->removeDefinition('file.temporary_storage.session');
-        }
     }
 
     /**

src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

@@ -8,8 +8,6 @@
         <parameter key="form.extension.class">Symfony\Component\Form\Extension\DependencyInjection\DependencyInjectionExtension</parameter>
         <parameter key="form.factory.class">Symfony\Component\Form\FormFactory</parameter>
         <parameter key="form.type_guesser.validator.class">Symfony\Component\Form\Extension\Validator\ValidatorTypeGuesser</parameter>
-        <parameter key="file.temporary_storage.class">Symfony\Component\HttpFoundation\File\TemporaryStorage</parameter>
-        <parameter key="file.temporary_storage.session.class">Symfony\Component\HttpFoundation\File\SessionBasedTemporaryStorage</parameter>
     </parameters>
 
     <services>
@@ -51,18 +49,6 @@
             <argument type="service" id="validator.mapping.class_metadata_factory" />
         </service>
 
-        <!-- TemporaryStorage - where should we put this? -->
-        <service id="file.temporary_storage.session" class="%file.temporary_storage.session.class%">
-            <argument type="service" id="session" />
-            <argument>%kernel.secret%</argument>
-            <argument>%kernel.cache_dir%/upload</argument>
-        </service>
-
-        <service id="file.temporary_storage" class="%file.temporary_storage.class%">
-            <argument>%kernel.secret%</argument>
-            <argument>%kernel.cache_dir%/upload</argument>
-        </service>
-
         <!-- CoreExtension -->
         <service id="form.type.field" class="Symfony\Component\Form\Extension\Core\Type\FieldType">
             <tag name="form.type" alias="field" />
@@ -97,7 +83,6 @@
         </service>
         <service id="form.type.file" class="Symfony\Component\Form\Extension\Core\Type\FileType">
             <tag name="form.type" alias="file" />
-            <argument type="service" id="file.temporary_storage" />
         </service>
         <service id="form.type.hidden" class="Symfony\Component\Form\Extension\Core\Type\HiddenType">
             <tag name="form.type" alias="hidden" />

src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/file_widget.html.php

@@ -5,8 +5,4 @@
         <?php if ($form['file']->get('disabled')): ?>disabled="disabled"<?php endif ?>
         <?php if ($form['file']->get('required')): ?>required="required"<?php endif ?>
     />
-
-    <?php echo $view['form']->widget($form['token']) ?>
-    <?php echo $view['form']->widget($form['name']) ?>
-    <?php echo $view['form']->widget($form['originalName']) ?>
 </div>

src/Symfony/Component/Form/Extension/Core/CoreExtension.php

@@ -13,7 +13,6 @@ namespace Symfony\Component\Form\Extension\Core;
 
 use Symfony\Component\Form\Extension\Core\Type;
 use Symfony\Component\Form\AbstractExtension;
-use Symfony\Component\HttpFoundation\File\TemporaryStorage;
 
 /**
  * Represents the main form extension, which loads the core functionality.
@@ -22,13 +21,6 @@ use Symfony\Component\HttpFoundation\File\TemporaryStorage;
  */
 class CoreExtension extends AbstractExtension
 {
-    private $storage;
-
-    public function __construct(TemporaryStorage $storage)
-    {
-        $this->storage = $storage;
-    }
-
     protected function loadTypes()
     {
         return array(
@@ -58,7 +50,7 @@ class CoreExtension extends AbstractExtension
             new Type\TimeType(),
             new Type\TimezoneType(),
             new Type\UrlType(),
-            new Type\FileType($this->storage),
+            new Type\FileType(),
         );
     }
 }

src/Symfony/Component/Form/Extension/Core/EventListener/FixFileUploadListener.php

@@ -1,86 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Component\Form\Extension\Core\EventListener;
-
-use Symfony\Component\Form\FormEvents;
-use Symfony\Component\Form\Exception\UnexpectedTypeException;
-use Symfony\Component\Form\Event\FilterDataEvent;
-use Symfony\Component\EventDispatcher\EventSubscriberInterface;
-use Symfony\Component\HttpFoundation\File\UploadedFile;
-use Symfony\Component\HttpFoundation\File\TemporaryStorage;
-
-/**
- * Moves uploaded files to a temporary location
- *
- * @author Bernhard Schussek <bernhard.schussek@symfony-project.com>
- */
-class FixFileUploadListener implements EventSubscriberInterface
-{
-    private $storage;
-
-    public function __construct(TemporaryStorage $storage)
-    {
-        $this->storage = $storage;
-    }
-
-    public static function getSubscribedEvents()
-    {
-        return array(FormEvents::BIND_CLIENT_DATA => 'onBindClientData');
-    }
-
-    public function onBindClientData(FilterDataEvent $event)
-    {
-        $form = $event->getForm();
-        $data = $event->getData();
-
-        if (null === $data) {
-            $data = array();
-        }
-
-        if (!is_array($data)) {
-            throw new UnexpectedTypeException($data, 'array');
-        }
-
-        $data = array_replace(array(
-            'file' => '',
-            'token' => '',
-            'name' => '',
-            'originalName' => '',
-        ), $data);
-
-        // Newly uploaded file
-        if ($data['file'] instanceof UploadedFile && $data['file']->isValid()) {
-            $data['token'] = (string)rand(100000, 999999);
-            $directory = $this->storage->getTempDir($data['token']);
-            $data['file']->move($directory);
-            $data['name'] = $data['file']->getName();
-            $data['originalName'] = $data['file']->getOriginalName();
-        }
-
-        // Existing uploaded file
-        if (!$data['file'] && $data['token'] && $data['name']) {
-            $path = $this->storage->getTempDir($data['token']) . DIRECTORY_SEPARATOR . $data['name'];
-
-            if (file_exists($path)) {
-                $data['file'] = new UploadedFile($path, $data['originalName'], null, null, null, true);
-            }
-        }
-
-        // Clear other fields if we still don't have a file, but keep
-        // possible existing files of the field
-        if (!$data['file']) {
-            $data = $form->getNormData();
-        }
-
-        $event->setData($data);
-    }
-}

src/Symfony/Component/Form/Extension/Core/Type/FileType.php

@@ -14,22 +14,13 @@ namespace Symfony\Component\Form\Extension\Core\Type;
 use Symfony\Component\Form\AbstractType;
 use Symfony\Component\Form\FormInterface;
 use Symfony\Component\Form\FormBuilder;
-use Symfony\Component\Form\Extension\Core\EventListener\FixFileUploadListener;
 use Symfony\Component\Form\ReversedTransformer;
 use Symfony\Component\Form\Extension\Core\DataTransformer\FileToStringTransformer;
 use Symfony\Component\Form\Extension\Core\DataTransformer\FileToArrayTransformer;
 use Symfony\Component\Form\FormView;
-use Symfony\Component\HttpFoundation\File\TemporaryStorage;
 
 class FileType extends AbstractType
 {
-    private $storage;
-
-    public function __construct(TemporaryStorage $storage)
-    {
-        $this->storage = $storage;
-    }
-
     public function buildForm(FormBuilder $builder, array $options)
     {
         if ($options['type'] === 'string') {
@@ -40,11 +31,7 @@ class FileType extends AbstractType
 
         $builder
             ->appendNormTransformer(new FileToArrayTransformer())
-            ->addEventSubscriber(new FixFileUploadListener($this->storage), 10)
             ->add('file', 'field')
-            ->add('token', 'hidden')
-            ->add('name', 'hidden')
-            ->add('originalName', 'hidden')
         ;
     }
 

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

@@ -1,36 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Component\HttpFoundation\File;
-
-use Symfony\Component\HttpFoundation\Session;
-
-/**
- * @author Bernhard Schussek <bernhard.schussek@symfony-project.com>
- */
-class SessionBasedTemporaryStorage extends TemporaryStorage
-{
-    private $session;
-
-    public function __construct(Session $session, $secret, $directory)
-    {
-        parent::__construct($secret, $directory);
-
-        $this->session = $session;
-    }
-
-    protected function generateHashInfo($token)
-    {
-        $this->session->start();
-
-        return $this->session->getId().parent::generateHashInfo($token);
-    }
-}

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

@@ -1,61 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Component\HttpFoundation\File;
-
-use Symfony\Component\HttpFoundation\File\Exception\FileException;
-use Symfony\Component\HttpFoundation\File\Exception\UnexpectedTypeException;
-
-/**
- * @author Bernhard Schussek <bernhard.schussek@symfony-project.com>
- */
-class TemporaryStorage
-{
-    private $directory;
-    private $secret;
-
-    public function __construct($secret, $directory)
-    {
-        if (!file_exists($directory)) {
-            mkdir($directory, 0777, true);
-        }
-
-        $this->directory = realpath($directory);
-        $this->secret = $secret;
-    }
-
-    protected function generateHashInfo($token)
-    {
-        return $this->secret.$token;
-    }
-
-    protected function generateHash($token)
-    {
-        return md5($this->generateHashInfo($token));
-    }
-
-    public function getTempDir($token)
-    {
-        if (!is_string($token)) {
-            throw new UnexpectedTypeException($token, 'string');
-        }
-
-        $hash = $this->generateHash($token);
-
-        $directory = $this->directory.DIRECTORY_SEPARATOR.substr($hash, 0, 2).DIRECTORY_SEPARATOR.substr($hash, 2);
-
-        if (!file_exists($directory)) {
-            mkdir($directory, 0777, true);
-        }
-
-        return $directory;
-    }
-}

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

@@ -51,13 +51,6 @@ class UploadedFile extends File
      */
     protected $error;
 
-    /**
-     * Whether the uploaded file has already been moved.
-     *
-     * @var Boolean
-     */
-    protected $moved;
-
     /**
      * Accepts the information of the uploaded file as provided by the PHP global $_FILES.
      *
@@ -66,13 +59,11 @@ class UploadedFile extends File
      * @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
      *
      * @throws FileException         If file_uploads is disabled
      * @throws FileNotFoundException If the file does not exist
      */
-    public function __construct($path, $originalName, $mimeType = null,
-            $size = null, $error = null, $moved = false)
+    public function __construct($path, $originalName, $mimeType = null, $size = null, $error = null)
     {
         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')));
@@ -87,7 +78,6 @@ class UploadedFile extends File
         $this->mimeType = $mimeType ?: 'application/octet-stream';
         $this->size = $size;
         $this->error = $error ?: UPLOAD_ERR_OK;
-        $this->moved = (Boolean) $moved;
     }
 
     /**
@@ -111,10 +101,6 @@ class UploadedFile extends File
      */
     public function getExtension()
     {
-        if ($this->moved) {
-            return parent::getExtension();
-        }
-
         if ($ext = pathinfo($this->getOriginalName(), PATHINFO_EXTENSION)) {
             return $ext;
         }
@@ -164,10 +150,6 @@ class UploadedFile extends File
      */
     public function move($directory, $name = null)
     {
-        if ($this->moved) {
-            return parent::move($directory, $name);
-        }
-
         $newPath = $directory.DIRECTORY_SEPARATOR.(null === $name ? $this->getName() : $name);
 
         if (!@move_uploaded_file($this->getPath(), $newPath)) {
@@ -175,7 +157,6 @@ class UploadedFile extends File
             throw new FileException(sprintf('Could not move file %s to %s (%s)', $this->getPath(), $newPath, strip_tags($error['message'])));
         }
 
-        $this->moved = true;
         $this->path = realpath($newPath);
     }
 }

tests/Symfony/Tests/Component/Form/AbstractLayoutTest.php

@@ -30,10 +30,9 @@ abstract class AbstractLayoutTest extends \PHPUnit_Framework_TestCase
 
         $dispatcher = new EventDispatcher();
         $this->csrfProvider = $this->getMock('Symfony\Component\Form\Extension\Csrf\CsrfProvider\CsrfProviderInterface');
-        $storage = new \Symfony\Component\HttpFoundation\File\TemporaryStorage('foo', \sys_get_temp_dir());
 
         $this->factory = new FormFactory(array(
-            new CoreExtension($storage),
+            new CoreExtension(),
             new CsrfExtension($this->csrfProvider),
         ));
     }
@@ -757,11 +756,8 @@ abstract class AbstractLayoutTest extends \PHPUnit_Framework_TestCase
 '/div
     [
         ./input[@type="file"][@id="na&me_file"]
-        /following-sibling::input[@type="hidden"][@id="na&me_token"]
-        /following-sibling::input[@type="hidden"][@id="na&me_name"]
-        /following-sibling::input[@type="hidden"][@id="na&me_originalName"]
     ]
-    [count(./input)=4]
+    [count(./input)=1]
 '
         );
     }

tests/Symfony/Tests/Component/Form/Extension/Core/EventListener/FixFileUploadListenerTest.php

@@ -1,167 +0,0 @@
-<?php
-
-/*
- * This file is part of the Symfony package.
- *
- * (c) Fabien Potencier <fabien@symfony.com>
- *
- * For the full copyright and license information, please view the LICENSE
- * file that was distributed with this source code.
- */
-
-namespace Symfony\Tests\Component\Form\Extension\Core\EventListener;
-
-use Symfony\Component\Form\Event\FilterDataEvent;
-use Symfony\Component\Form\Extension\Core\EventListener\FixFileUploadListener;
-use Symfony\Component\HttpFoundation\File\UploadedFile;
-
-class FixFileUploadListenerTest extends \PHPUnit_Framework_TestCase
-{
-    private $storage;
-
-    private $destination;
-
-    public function setUp()
-    {
-        $this->storage = $this->getMockBuilder('Symfony\Component\HttpFoundation\File\TemporaryStorage')
-            ->disableOriginalConstructor()
-            ->getMock();
-    }
-
-    public function testValidNewlyUploadedFile()
-    {
-        $passedToken = null;
-
-        $this->storage->expects($this->any())
-            ->method('getTempDir')
-            ->will($this->returnCallback(function ($token) use (&$passedToken) {
-                $passedToken = $token;
-
-                return __DIR__.DIRECTORY_SEPARATOR.'tmp';
-            }));
-
-        $file = $this->createUploadedFileMock('randomhash', 'original.jpg', true);
-        $file->expects($this->once())
-            ->method('move')
-            ->with(__DIR__.DIRECTORY_SEPARATOR.'tmp');
-
-        $data = array(
-            'file' => $file,
-            'token' => '',
-            'name' => '',
-            'originalName' => '',
-        );
-
-        $form = $this->getMock('Symfony\Tests\Component\Form\FormInterface');
-        $event = new FilterDataEvent($form, $data);
-
-        $filter = new FixFileUploadListener($this->storage);
-        $filter->onBindClientData($event);
-
-        $this->assertEquals(array(
-            'file' => $file,
-            'name' => 'randomhash',
-            'originalName' => 'original.jpg',
-            'token' => $passedToken,
-        ), $event->getData());
-    }
-
-    public function testExistingUploadedFile()
-    {
-        $test = $this;
-
-        $this->storage->expects($this->any())
-            ->method('getTempDir')
-            ->will($this->returnCallback(function ($token) use ($test) {
-                $test->assertSame('abcdef', $token);
-
-                return __DIR__.DIRECTORY_SEPARATOR.'Fixtures';
-            }));
-
-        $data = array(
-            'file' => '',
-            'token' => 'abcdef',
-            'name' => 'randomhash',
-            'originalName' => 'original.jpg',
-        );
-
-        $form = $this->getMock('Symfony\Tests\Component\Form\FormInterface');
-        $event = new FilterDataEvent($form, $data);
-
-        $filter = new FixFileUploadListener($this->storage);
-        $filter->onBindClientData($event);
-
-        $this->assertEquals(array(
-            'file' => new UploadedFile(
-                __DIR__.DIRECTORY_SEPARATOR.'Fixtures'.DIRECTORY_SEPARATOR.'randomhash',
-                'original.jpg',
-                null,
-                null,
-                null,
-                true // already moved
-             ),
-            'name' => 'randomhash',
-            'originalName' => 'original.jpg',
-            'token' => 'abcdef',
-        ), $event->getData());
-    }
-
-    public function testNullAndExistingFile()
-    {
-        $existingData = array(
-            'file' => new UploadedFile(
-                __DIR__.DIRECTORY_SEPARATOR.'Fixtures'.DIRECTORY_SEPARATOR.'randomhash',
-                'original.jpg',
-                null,
-                null,
-                null,
-                true // already moved
-             ),
-            'name' => 'randomhash',
-            'originalName' => 'original.jpg',
-            'token' => 'abcdef',
-        );
-
-        $form = $this->getMock('Symfony\Tests\Component\Form\FormInterface');
-        $form->expects($this->any())
-            ->method('getNormData')
-            ->will($this->returnValue($existingData));
-
-        $event = new FilterDataEvent($form, null);
-
-        $filter = new FixFileUploadListener($this->storage);
-        $filter->onBindClientData($event);
-
-        $this->assertSame($existingData, $event->getData());
-    }
-
-    /**
-     * @expectedException Symfony\Component\Form\Exception\UnexpectedTypeException
-     */
-    public function testExpectNullOrArray()
-    {
-        $form = $this->getMock('Symfony\Tests\Component\Form\FormInterface');
-        $event = new FilterDataEvent($form, 'foobar');
-
-        $filter = new FixFileUploadListener($this->storage);
-        $filter->onBindClientData($event);
-    }
-
-    private function createUploadedFileMock($name, $originalName, $valid)
-    {
-        $file = $this->getMockBuilder('Symfony\Component\HttpFoundation\File\UploadedFile')
-            ->disableOriginalConstructor()
-            ->getMock();
-        $file->expects($this->any())
-            ->method('getName')
-            ->will($this->returnValue($name));
-        $file->expects($this->any())
-            ->method('getOriginalName')
-            ->will($this->returnValue($originalName));
-        $file->expects($this->any())
-            ->method('isValid')
-            ->will($this->returnValue($valid));
-
-        return $file;
-    }
-}

tests/Symfony/Tests/Component/Form/Extension/Core/Type/TypeTestCase.php

@@ -18,8 +18,6 @@ use Symfony\Component\EventDispatcher\EventDispatcher;
 
 abstract class TypeTestCase extends \PHPUnit_Framework_TestCase
 {
-    protected $storage;
-
     protected $factory;
 
     protected $builder;
@@ -31,9 +29,6 @@ abstract class TypeTestCase extends \PHPUnit_Framework_TestCase
     protected function setUp()
     {
         $this->dispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcherInterface');
-        $this->storage = $this->getMockBuilder('Symfony\Component\HttpFoundation\File\TemporaryStorage')
-            ->disableOriginalConstructor()
-            ->getMock();
         $this->factory = new FormFactory($this->getExtensions());
         $this->builder = new FormBuilder(null, $this->factory, $this->dispatcher);
     }
@@ -41,7 +36,7 @@ abstract class TypeTestCase extends \PHPUnit_Framework_TestCase
     protected function getExtensions()
     {
         return array(
-            new CoreExtension($this->storage),
+            new CoreExtension(),
         );
     }