[2.1] Form File Upload refactor #2439

Closed
wants to merge 16 commits into from

9 participants

@cgmartin

This PR refactors File Uploads to use Zend\Form's normal input filtering rather then using Zend\File\Transfer.
Ready to merge

Notes:

  • Zend\File\Transfer and its HTTP adapter is no longer used.
  • Filters and Validators are added to File's input filter, just like any other form input.
  • New Zend\InputFilter\FileInput will do validation first, then filtering.
  • InputFilter has been modified to detect if File Upload is missing, for isRequired/isEmpty tests.
  • New Zend\Filter\File\RenameUpload for securely moving uploaded files.
  • Refactored Zend\File\Transfer\Adapter\Http upload progress methods into Zend\ProgressBar\Upload handlers: Apc, UploadProgress, and (new) PHP5.4+ Session Upload Progress.
  • New Zend\Form\Element\File elements for Apc, UploadProgress, and Session upload progress tracking.
  • Multi-file uploads work successfully with Zend\Form\Element\Collection and when the HTML5 "multiple" attribute is set.
  • New Zend\Validator\File\Explode validator for "multiple" attribute validation.

Important to note: You must be cognizant to pass the $_FILES info into the Form:

$data = array_merge(
    $this->getRequest()->getPost()->toArray(),
    $this->getRequest()->getFiles()->toArray()
);
$form->setData($data);

Full usage example here:
https://gist.github.com/3797983

@cgmartin cgmartin commented on an outdated diff Sep 28, 2012
library/Zend/Form/Form.php
@@ -570,25 +564,27 @@ public function setInputFilter(InputFilterInterface $inputFilter)
*/
public function getInputFilter()
{
- if ($this->object instanceof InputFilterAwareInterface) {
- if (null == $this->baseFieldset) {
- $this->filter = $this->object->getInputFilter();
- } else {
- $name = $this->baseFieldset->getName();
- if (!$this->filter instanceof InputFilterInterface || !$this->filter->has($name)) {
- $filter = new InputFilter();
- $filter->add($this->object->getInputFilter(), $name);
- $this->filter = $filter;
+ if (null === $this->filter) {

Bugfix: dupe filters being added on multiple calls to getInputFilter().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cgmartin cgmartin and 1 other commented on an outdated diff Sep 28, 2012
library/Zend/InputFilter/Input.php
@@ -241,7 +241,7 @@ public function merge(InputInterface $input)
$this->setErrorMessage($input->getErrorMessage());
$this->setName($input->getName());
$this->setRequired($input->isRequired());
- $this->setValue($input->getValue());
+ $this->setValue($input->getRawValue());

Bugfix: Filters shouldn't be run during merge, right?

No. I added the merge function to allow to override/add some filters/validators, for instance for all HTML5 elements that had pre-defined filters/validators. This is called only when the input filters are created, so this occurs before any filtering.

Hmm, this isn't what I was experiencing in my testing. The $input->getValue() runs the filters on the object that is being merged from, and then a few lines below filters are merged... So when this Input's getValue() is called, the filters are run yet again.
Still seems to me that the actual raw value should be merged since the filters are always being run on getValue().

You're completely right. I actually made a big mistake there :/... Thanks for correcting it !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cgmartin cgmartin commented on an outdated diff Sep 28, 2012
library/Zend/InputFilter/BaseInputFilter.php
@@ -165,6 +165,7 @@ public function isValid()
if (!array_key_exists($name, $this->data)
|| (null === $this->data[$name])
|| (is_string($this->data[$name]) && strlen($this->data[$name]) === 0)
+ || (isset($this->data[$name]['error']) && $this->data[$name]['error'] === UPLOAD_ERR_NO_FILE)

Support empty uploads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bakura10 bakura10 commented on the diff Sep 28, 2012
library/Zend/Validator/File/Size.php
@@ -33,9 +33,9 @@ class Size extends AbstractValidator

We should speak to @weierophinney about that, but following the new conventions of ZF 2, I think those validators should be moved to Zend\File namespace (for instance, all the i18n view helpers were moved to I18n namespace, same for Form view helpers).

@weierophinney
Zend Framework member

We can't move them during the 2.X cycle at this point. If we'd done this prior to the stable release, sure -- but following stable, no. We should do this for v3, however. Reminder to myself: start a v3 TODO. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bakura10

Really good work ! This was really missing. I've quickly read the source, it seems good to me.

I wonder about one thing : when hydrated, what will the element contain ? the file name ?

@cgmartin

@bakura10 When hydrated, the element's value "should" contain a string of the file path. I need to test this though - thanks for bringing it up.

@weierophinney weierophinney was assigned Oct 1, 2012
@weierophinney
Zend Framework member

I really like this new approach. Having the FileInput as part of InputFilter makes a lot of sense to me, and I appreciate how much it simplifies the validators.

Looks like you may need to rebase off of develop, btw.

Keep me posted, and let me know when it's ready to review for merge!

@cgmartin

Thanks @weierophinney. I'll pick this back up and ping you when it's ready for a final review.

@cgmartin

Will this UploadFilter have the capability handle a succesfull upload, but other elements failed to validate use cases? Which would give the ability to just repost the (corrected) form without the file, but still have access to it?

I've been thinking about this too, but haven't come up with a generic/configurable/automatic way to do this with the form yet. There seemed to be several use cases one may want to do in this situation, so in an effort to get this out and tested, we'll leave it up to the developer to do their own custom handling for now.

File validations and filters will still run and process the file. But, it will be up to the developer to explicitly check that the file element is valid after postback and then modify the form/view to compensate, update the db, etc.. In some cases you might be able to separate out the file uploads into their own form to get around this, but not always.

@bakura10
@cgmartin

This is ready for a final review. Thanks!

@juriansluiman

It will be fantastic to have this merged for 2.1! I almost don't dare to ask, but what about docs? It would be nice when this gets merged, the docs can be merged directly too.

@bakura10

Wow... you need such an awesome work @cgmartin ! That's outstanding ! I can't wait to try this out !

@cgmartin

@juriansluiman yes, already planning to work on the docs after this gets a final approval. I wouldn't leave you hanging like that! ;) Currently working on usage examples to see if this could fit with the postRequestGet plugin somehow.

@bakura10 thanks!

@basz
@cgmartin

Found an issue when using the HTML5 "multiple" attribute with the file element. Working on a fix. Do not merge.

@cgmartin

OK this should be good for a final review again.

A "nice-to-have" would be an Explode Filter, akin to the Explode validators when the multiple attribute is set. IMO, I don't think that should hold up this PR, but I'll work on getting it in regardless.

@basz yep, working on it. We'll see how it goes.

@davidwindell

A few questions as we are currently using the existing implementation, will we still be able to achieve all the functionality as shown in the following gist? https://gist.github.com/83f582c74d45174e0538, for example;

  • Specifying the temporary destination
  • Getting the File Info data on the uploaded files for mimetype logging

Anything else you can see in there that might not work?

@weierophinney
Zend Framework member

@cgmartin Can you rebase against latest develop? There are evidently conflicts currently, and I don't want to mess up the merge.

Thanks!

@cgmartin

@davidwindell you'll still be able to achieve those things, though the implementation will be different, obviously. The FileSize, Extension, MimeType validators still exist; Count would now be managed by a Collection; and the destination can be specified via the RenameUpload filter. Getting the mime info for your attachment looks to be the only difference - You would likely use FileInfo (or similar) directly yourself for that, as the form processing will not save that information automatically. I haven't looked closely at it, but Zend\Mime may provide some automatic features for mail attachments.

@weierophinney, rebased. Thanks!

Now that ZendCon is over I'll be picking back up on the docs and the following enhancements in the following week:

Post-Redirect-Get plugin support
Discussed briefly with @weierophinney that we could potentially process the form's file elements (running validation and filtering) during the post, and save the error messages (and other state) for after the redirect. The form file elements must process on POST for is_uploaded_file security validation.

Multi-File Filtering
The Form will require a "Explode" filter (similar to the Explode validator) in order to process multiple files.

@cgmartin

Travis failed only on 5.3.3 tests due to network connectivity, see details. Latest commit should pass.

Added a new Post-Redirect-Get plugin for File upload forms:
https://github.com/zendframework/zf2/pull/2439/files#L21R25

It is fundamentally different than the original plugin as it will process the form on POST and save any form errors in the session when it comes back around on the GET. Usage is different, see example here: https://github.com/cgmartin/ZF2FileUploadExamples/blob/master/src/ZF2FileUploadExamples/Controller/Examples.php#L75

@weierophinney weierophinney added a commit that closed this pull request Nov 6, 2012
@weierophinney weierophinney Merge branch 'feature/form-file-validation' into develop
File validation for forms, with PRG support

Close #2439
0976470
@weierophinney
Zend Framework member

STELLAR work, @cgmartin !!!!

@bakura10

Can't wait to use that ! Thank you @cgmartin !

@juriansluiman

👍 👏 :metal: whohoo!

@cgmartin
@mohamedsharaf

here i think we can add option to keep old file extension or it may be added to rename filter

$uploaded_file = pathinfo($value['name']);

$uploaded_file_extension = $uploaded_file['extension'];

The $value that is passed into the file filters is only a filename string...the 'tmp_name' value in an upload situation. We unfortunately don't have the other upload info in this context (at the moment). This would need some deeper investigation.

@taiwen

The commit broke built-in calls for validators for extension, exclueextension, etc. by removing the second argument in isValid($value, $file).

@taiwen Thanks for reporting this, I'll take a stab at a fix.

Some background:

  • Second argument to isValid() was removed for the file validators to match ValidatorInterface.
  • Original assumption was that Zend\Transfer was to be removed when this refactor was implemented. Was added back in last minute before 2.1 release.
@mbn18

@cgmartin , is it possible to create an InputFilter for file upload using InputFactory?

I tried different variant like this. but all failed.


$factory = new InputFactory();

$this->add($factory->createInputFilter(array(
    'name'          => 'file',
    'required'      => true,
)));
@mbn18

@cgmartin , tried that with no success.

I think that type is not the problem as it is already being declared at:
https://github.com/zendframework/zf2/blob/master/library/Zend/InputFilter/Factory.php#L217

I tried that but got an error.

        $this->add($factory->createInputFilter(array(
            'name'          => 'file',
            'required'      => true,
            'type'          => 'Zend\InputFilter\FileInput',
        )));
File:
/srv/httpd/brainpop/dev/web/vendor/ZendFramework/library/Zend/InputFilter/Factory.php:231
Message:
InputFilter factory expects the "type" to be a class implementing Zend\InputFilter\InputFilterInterface; received "Zend\InputFilter\FileInput"

Maybe the class is not defined yet or not compatible?

@cgmartin

Ah, we both may have confused by the differences between InputFilter vs. Input.

Assuming $this is your form's InputFilter, try this:

        $this->add($factory->createInput(array(
            'name'          => 'file',
            'required'      => true,
            'type'          => 'Zend\InputFilter\FileInput',
        )));

The InputFilter acts as a parent collection of Input objects (and potentially other InputFilters). The Input objects themselves contain the required, filters, validators details for each form input.
http://framework.zend.com/manual/2.1/en/modules/zend.input-filter.intro.html

The File Element utilizes $factory->createInput() rather than $factory->createInputFilter(). Here is the correct place where that type is checked in the factory:
https://github.com/zendframework/zf2/blob/master/library/Zend/InputFilter/Factory.php#L109

The file elements need a special FileInput in order to do the validations and filters correctly for file uploads. Some details here about the differences:
http://framework.zend.com/manual/2.1/en/modules/zend.input-filter.file-input.html

Hope this helps, let us know if still encountering a problem.

@mbn18

Hi @cgmartin ,

Here is the full version ( without other fields )

<?php
namespace WhatEver\Form;

use Zend\InputFilter\InputFilter;
use Zend\InputFilter\Factory as InputFactory;

class AmazingWonderfulFilter extends InputFilter
{
    public function __construct()
    {
        $factory = new InputFactory();

        $this->add($factory->createInput(array(
            'name'          => 'file',
            'required'      => true,
            'type'          => 'Zend\InputFilter\FileInput',
        )));
    } // End of __construct
}

And the output ( I get the same error with and without declaring type )

An error occurred
An error occurred during execution; please try again later.
Additional information:
Zend\View\Exception\InvalidArgumentException
File:
/srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/Helper/Escaper/AbstractHelper.php:99
Message:
Array provided to Escape helper, but flags do not allow recursion
Stack trace:
#0 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Form/View/Helper/AbstractHelper.php(228): Zend\View\Helper\Escaper\AbstractHelper->__invoke(Array)
#1 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Form/View/Helper/FormInput.php(111): Zend\Form\View\Helper\AbstractHelper->createAttributesString(Array)
#2 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Form/View/Helper/FormInput.php(130): Zend\Form\View\Helper\FormInput->render(Object(Zend\Form\Element\File))
#3 [internal function]: Zend\Form\View\Helper\FormInput->__invoke(Object(Zend\Form\Element\File))
#4 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/Renderer/PhpRenderer.php(400): call_user_func_array(Object(Zend\Form\View\Helper\FormInput), Array)
#5 /srv/httpd/myproj/dev/web/module/ProjActivity/view/activity/view_revision.phtml(99): Zend\View\Renderer\PhpRenderer->__call('formInput', Array)
#6 /srv/httpd/myproj/dev/web/module/ProjActivity/view/activity/view_revision.phtml(99): Zend\View\Renderer\PhpRenderer->formInput(Object(Zend\Form\Element\File))
#7 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/Renderer/PhpRenderer.php(507): include('/srv/httpd/brai...')
#8 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/View.php(201): Zend\View\Renderer\PhpRenderer->render(Object(Zend\View\Model\ViewModel))
#9 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/View.php(229): Zend\View\View->render(Object(Zend\View\Model\ViewModel))
#10 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/View/View.php(194): Zend\View\View->renderChildren(Object(Zend\View\Model\ViewModel))
#11 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Mvc/View/Http/DefaultRenderingStrategy.php(126): Zend\View\View->render(Object(Zend\View\Model\ViewModel))
#12 [internal function]: Zend\Mvc\View\Http\DefaultRenderingStrategy->render(Object(Zend\Mvc\MvcEvent))
#13 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/EventManager/EventManager.php(460): call_user_func(Array, Object(Zend\Mvc\MvcEvent))
#14 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/EventManager/EventManager.php(204): Zend\EventManager\EventManager->triggerListeners('render', Object(Zend\Mvc\MvcEvent), Array)
#15 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Mvc/Application.php(332): Zend\EventManager\EventManager->trigger('render', Object(Zend\Mvc\MvcEvent))
#16 /srv/httpd/myproj/dev/web/vendor/ZendFramework/library/Zend/Mvc/Application.php(307): Zend\Mvc\Application->completeRequest(Object(Zend\Mvc\MvcEvent))
#17 /srv/httpd/myproj/dev/web/public/index.php(12): Zend\Mvc\Application->run()
#18 {main}
@davidwindell

@mbn18 what does your view script look like?

@mbn18

The view script is quite big. The relevant lines are:

<?php echo $this->formLabel($update_activity_rev_form->get('file')); ?>
<?php echo $this->formInput($update_activity_rev_form->get('file')->setAttributes(array('style'=>'margin-left: 10px;'))); ?>

But the error occur when I run an update controller (no view)

The form spec is:

<?php
namespace WhatEver\Form;

use Zend\Form\Factory as FormFactory;
use Zend\Form\Form;

class UpdateFooForm extends Form
{
    public function __construct()
    {
        parent::__construct($name);
        $factory = new FormFactory();

        $this->add($factory->createElement(array(
            'name' => 'file',
            'type' => 'Zend\Form\Element\File',
            'options' => array(
                'label' => 'Upload:',
            ),
        )));
    }
}
@weierophinney
Zend Framework member
@juriansluiman

/me thanks @weierophinney for suggesting a view helper I did not know about :-)

@davidwindell

@mbn18 @weierophinney Yep, that's why I wanted to see your view, just swap out the formInput with formFile or even formRow and you're good.

@mbn18

Yep, that was the problem. Thanks everyone.
Maybe it will be beneficial to add it to the docs? If so, I can do it. Where it is done?

@cgmartin

The docs I created back in the 2.1 release have some details:
http://framework.zend.com/manual/2.1/en/modules/zend.form.view.helpers.html#formfile
http://framework.zend.com/manual/2.1/en/modules/zend.form.file-upload.html

Feel free to open a PR against the ZF2 Docs repo if they could use any enhancements:
https://github.com/zendframework/zf2-documentation

@ghost Unknown pushed a commit that referenced this pull request Jul 14, 2013
@weierophinney weierophinney Merge branch 'feature/form-file-validation' into develop
File validation for forms, with PRG support

Close #2439
4bf1ebb
@weierophinney weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/form-file-validation' into develop
File validation for forms, with PRG support

Close zendframework/zendframework#2439
3615cdf
@weierophinney weierophinney added a commit to zendframework/zend-progressbar that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/form-file-validation' into develop
File validation for forms, with PRG support

Close zendframework/zendframework#2439
2cc8054
@weierophinney weierophinney added a commit to zendframework/zend-filter that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/form-file-validation' into develop
File validation for forms, with PRG support

Close zendframework/zendframework#2439
1bd0805
@weierophinney weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/form-file-validation' into develop
File validation for forms, with PRG support

Close zendframework/zendframework#2439
886d9b6
@weierophinney weierophinney added a commit to zendframework/zend-file that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/form-file-validation' into develop
File validation for forms, with PRG support

Close zendframework/zendframework#2439
5ec28c8
@yourithielen yourithielen referenced this pull request in zendframework/zend-file Mar 9, 2016
Closed

Purpose of Zend\File? #22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment