Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[Hotfix] FilePostRedirectGet plugin and form collections #5641

Closed
wants to merge 3 commits into from

4 participants

Stefano Torresi 2DivisionsByZero Matthew Weier O'Phinney dpommeranz
Stefano Torresi

This PR fixes a problem with the fileprg plugin when using form collections with either allow_add or allow_remove set to true.

FilePostRedirectGet invoked Form::getInputFilter() before Form::setData(), so any filter provided by collection elements did not run at all for dinamically added/removed ones, since the number of elements/filters may vary on the basis of the input data.

Edit:
I also added some more code to fix another problem that occurs in the plugin, indicated in #5381.

When a collection target element is a fieldset with both a file and normal input, the merge done by array_merge_recursive (used by FilePRG) leads to unpredictable behaviour due to colliding numeric keys in both $_POST and $_FILES.

#5244 (which in turn fixes #4974) adds the capability to preserve numeric keys in ArrayUtils::merge()

#5381 was supposed to test the new behaviour but it was somehow borked, so i started from @2DivisionsByZero work to provide a comprehensive PR.

dpommeranz and others added some commits
dpommeranz dpommeranz fixes zendframework/zf2#4974
Conflicts:
	library/Zend/Mvc/Controller/Plugin/FilePostRedirectGet.php
468860b
Stefano Torresi stefanotorresi fix fileprg tests by @2DivisionsByZero, close #5381 f6ba030
2DivisionsByZero

Multiple collection elements are still not working. See tests at stefanotorresi#2

Matthew Weier O'Phinney weierophinney added this to the 2.2.6 milestone
Matthew Weier O'Phinney weierophinney self-assigned this
Matthew Weier O'Phinney weierophinney referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney [#5641] CS fixes
- EOF ending
8a0fdf5
Matthew Weier O'Phinney weierophinney referenced this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/5641' into develop
Forward port #5641

Conflicts:
	library/Zend/Stdlib/ArrayUtils.php
41494eb
Stefano Torresi stefanotorresi deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 14, 2014
  1. Stefano Torresi
  2. dpommeranz Stefano Torresi

    fixes zendframework/zf2#4974

    dpommeranz authored stefanotorresi committed
    Conflicts:
    	library/Zend/Mvc/Controller/Plugin/FilePostRedirectGet.php
  3. Stefano Torresi
This page is out of date. Refresh to see the latest.
27 library/Zend/Mvc/Controller/Plugin/FilePostRedirectGet.php
View
@@ -17,6 +17,7 @@
use Zend\InputFilter\InputFilterInterface;
use Zend\Mvc\Exception\RuntimeException;
use Zend\Session\Container;
+use Zend\Stdlib\ArrayUtils;
use Zend\Validator\ValidatorChain;
/**
@@ -63,6 +64,13 @@ protected function handlePostRequest(FormInterface $form, $redirect, $redirectTo
$container = $this->getSessionContainer();
$request = $this->getController()->getRequest();
+ $postFiles = $request->getFiles()->toArray();
+ $postOther = $request->getPost()->toArray();
+ $post = ArrayUtils::merge($postOther, $postFiles, true);
+
+ // Fill form with the data first, collections may alter the form/filter structure
+ $form->setData($post);
+
// Change required flag to false for any previously uploaded files
$inputFilter = $form->getInputFilter();
$previousFiles = ($container->files) ?: array();
@@ -78,12 +86,6 @@ function ($input, $value) {
);
// Run the form validations/filters and retrieve any errors
- $postFiles = $request->getFiles()->toArray();
- $postOther = $request->getPost()->toArray();
- $post = array_merge_recursive($postOther, $postFiles);
-
- // Validate form, and capture data and errors
- $form->setData($post);
$isValid = $form->isValid();
$data = $form->getData(FormInterface::VALUES_AS_ARRAY);
$errors = (!$isValid) ? $form->getMessages() : null;
@@ -91,11 +93,12 @@ function ($input, $value) {
// Merge and replace previous files with new valid files
$prevFileData = $this->getEmptyUploadData($inputFilter, $previousFiles);
$newFileData = $this->getNonEmptyUploadData($inputFilter, $data);
- $postFiles = array_merge_recursive(
+ $postFiles = ArrayUtils::merge(
$prevFileData ?: array(),
- $newFileData ?: array()
+ $newFileData ?: array(),
+ true
);
- $post = array_merge_recursive($postOther, $postFiles);
+ $post = ArrayUtils::merge($postOther, $postFiles, true);
// Save form data in session
$container->setExpirationHops(1, array('post', 'errors', 'isValid'));
@@ -129,6 +132,9 @@ protected function handleGetRequest(FormInterface $form)
unset($container->errors);
unset($container->isValid);
+ // Fill form with the data first, collections may alter the form/filter structure
+ $form->setData($post);
+
// Remove File Input validators and filters on previously uploaded files
// in case $form->isValid() or $form->bindValues() is run
$inputFilter = $form->getInputFilter();
@@ -145,8 +151,7 @@ function ($input, $value) {
}
);
- // Fill form with previous info and state
- $form->setData($post);
+ // set previous state
$form->isValid(); // re-validate to bind values
if (null !== $errors) {
$form->setMessages($errors); // overwrite messages
14 library/Zend/Stdlib/ArrayUtils.php
View
@@ -245,23 +245,23 @@ public static function iteratorToArray($iterator, $recursive = true)
/**
* Merge two arrays together.
*
- * If an integer key exists in both arrays, the value from the second array
- * will be appended the the first array. If both values are arrays, they
- * are merged together, else the value of the second array overwrites the
- * one of the first array.
+ * If an integer key exists in both arrays and preserveNumericKeys is false, the value
+ * from the second array will be appended to the first array. If both values are arrays, they
+ * are merged together, else the value of the second array overwrites the one of the first array.
*
* @param array $a
* @param array $b
+ * @param bool $preserveNumericKeys
* @return array
*/
- public static function merge(array $a, array $b)
+ public static function merge(array $a, array $b, $preserveNumericKeys = false)
{
foreach ($b as $key => $value) {
if (array_key_exists($key, $a)) {
- if (is_int($key)) {
+ if (is_int($key) && !$preserveNumericKeys) {
$a[] = $value;
} elseif (is_array($value) && is_array($a[$key])) {
- $a[$key] = static::merge($a[$key], $value);
+ $a[$key] = static::merge($a[$key], $value, $preserveNumericKeys);
} else {
$a[$key] = $value;
}
137 tests/ZendTest/Mvc/Controller/Plugin/FilePostRedirectGetTest.php
View
@@ -22,6 +22,7 @@
use Zend\Mvc\Router\RouteMatch;
use Zend\Mvc\Router\SimpleRouteStack;
use Zend\Stdlib\Parameters;
+use Zend\Validator\NotEmpty;
use ZendTest\Mvc\Controller\TestAsset\SampleController;
use ZendTest\Session\TestAsset\TestManager as SessionManager;
@@ -262,4 +263,140 @@ public function testReuseMatchedParametersWithSegmentController()
$this->assertEquals($expects, $prgResultRoute->getHeaders()->get('Location')->getUri() , 'redirect to the same url');
$this->assertEquals(303, $prgResultRoute->getStatusCode());
}
+
+ public function testCollectionInputFilterIsInitializedBeforePluginRetrievesIt()
+ {
+ $fieldset = new TestAsset\InputFilterProviderFieldset();
+ $collectionSpec = array(
+ 'name' => 'test_collection',
+ 'type' => 'collection',
+ 'options' => array(
+ 'target_element' => $fieldset
+ ),
+ );
+
+ $form = new Form();
+ $form->add($collectionSpec);
+
+ $postData = array(
+ 'test_collection' => array(
+ array (
+ 'test_field' => 'foo'
+ ),
+ array (
+ 'test_field' => 'bar'
+ )
+ )
+ );
+
+ // test POST
+ $request = new Request();
+ $request->setMethod('POST');
+ $request->setPost(new Parameters($postData));
+ $this->controller->dispatch($request, $this->response);
+
+ $this->controller->fileprg($form, '/someurl', true);
+
+ $data = $form->getData();
+
+ $this->assertArrayHasKey(0, $data['test_collection']);
+ $this->assertArrayHasKey(1, $data['test_collection']);
+
+ $this->assertSame('FOO', $data['test_collection'][0]['test_field']);
+ $this->assertSame('BAR', $data['test_collection'][1]['test_field']);
+
+ // now test GET with a brand new form instance
+ $form = new Form();
+ $form->add($collectionSpec);
+
+ $request = new Request();
+ $this->controller->dispatch($request, $this->response);
+
+ $this->controller->fileprg($form, '/someurl', true);
+
+ $data = $form->getData();
+
+ $this->assertArrayHasKey(0, $data['test_collection']);
+ $this->assertArrayHasKey(1, $data['test_collection']);
+
+ $this->assertSame('FOO', $data['test_collection'][0]['test_field']);
+ $this->assertSame('BAR', $data['test_collection'][1]['test_field']);
+ }
+
+ public function testCorrectInputDataMerging()
+ {
+ require_once __DIR__ . '/TestAsset/DisablePhpUploadChecks.php';
+ require_once __DIR__ . '/TestAsset/DisablePhpMoveUploadedFileChecks.php';
+
+ $form = new Form();
+ $form->add(array(
+ 'name' => 'collection',
+ 'type' => 'collection',
+ 'options' => array(
+ 'target_element' => new TestAsset\TestFieldset('target'),
+ 'count' => 2,
+ )
+ ));
+
+ copy(__DIR__ . '/TestAsset/nullfile', __DIR__ . '/TestAsset/nullfile_copy');
+
+ $request = $this->request;
+ $request->setMethod('POST');
+ $request->setPost(new Parameters(array(
+ 'collection' => array(
+ 0 => array(
+ 'text' => 'testvalue1',
+ ),
+ 1 => array(
+ 'text' => '',
+ )
+ )
+ )));
+ $request->setFiles(new Parameters(array(
+ 'collection' => array(
+ 0 => array(
+ 'file' => array(
+ 'name' => 'test.jpg',
+ 'type' => 'image/jpeg',
+ 'size' => 20480,
+ 'tmp_name' => __DIR__ . '/TestAsset/nullfile_copy',
+ 'error' => UPLOAD_ERR_OK
+ ),
+ ),
+ )
+ )));
+
+ $this->controller->dispatch($this->request, $this->response);
+ $this->controller->fileprg($form, '/test/getPage', true);
+
+ $this->assertFalse($form->isValid());
+ $data = $form->getData();
+
+ $this->assertEquals(array(
+ 'collection' => array(
+ 0 => array(
+ 'text' => 'testvalue1',
+ 'file' => array(
+ 'name' => 'test.jpg',
+ 'type' => 'image/jpeg',
+ 'size' => 20480,
+ 'tmp_name' => __DIR__ . '/TestAsset/testfile.jpg',
+ 'error' => 0
+ ),
+ ),
+ 1 => array(
+ 'text' => null,
+ 'file' => null,
+ )
+ )
+ ), $data);
+
+ $this->assertFileExists($data['collection'][0]['file']['tmp_name']);
+
+ unlink($data['collection'][0]['file']['tmp_name']);
+
+ $messages = $form->getMessages();
+ $this->assertTrue(isset($messages['collection'][1]['text'][NotEmpty::IS_EMPTY]));
+ $this->assertTrue(isset($messages['collection'][1]['file'][NotEmpty::IS_EMPTY]));
+ }
}
8 tests/ZendTest/Mvc/Controller/Plugin/TestAsset/DisablePhpMoveUploadedFileChecks.php
View
@@ -0,0 +1,8 @@
+<?php
+
+namespace Zend\Filter\File;
+
+function move_uploaded_file($source, $dest)
+{
+ return rename($source, $dest);
+}
8 tests/ZendTest/Mvc/Controller/Plugin/TestAsset/DisablePhpUploadChecks.php
View
@@ -0,0 +1,8 @@
+<?php
+
+namespace Zend\Validator\File;
+
+function is_uploaded_file($filename)
+{
+ return true;
+}
37 tests/ZendTest/Mvc/Controller/Plugin/TestAsset/InputFilterProviderFieldset.php
View
@@ -0,0 +1,37 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace ZendTest\Mvc\Controller\Plugin\TestAsset;
+
+use Zend\Form\Element;
+use Zend\Form\Fieldset;
+use Zend\InputFilter\InputFilterProviderInterface;
+
+class InputFilterProviderFieldset extends Fieldset implements InputFilterProviderInterface
+{
+ public function __construct($name = null, $options = array())
+ {
+ parent::__construct($name, $options);
+
+ $this->add(array(
+ 'name' => 'test_field',
+ ));
+ }
+
+ public function getInputFilterSpecification()
+ {
+ return array(
+ 'test_field' => array(
+ 'filters' => array(
+ new \Zend\Filter\StringToUpper,
+ ),
+ ),
+ );
+ }
+}
45 tests/ZendTest/Mvc/Controller/Plugin/TestAsset/TestFieldset.php
View
@@ -0,0 +1,45 @@
+<?php
+
+namespace ZendTest\Mvc\Controller\Plugin\TestAsset;
+
+use Zend\Form\Fieldset;
+use Zend\InputFilter\InputFilterProviderInterface;
+
+class TestFieldset extends Fieldset implements InputFilterProviderInterface
+{
+ public function __construct($name = null, $options = array())
+ {
+ parent::__construct($name, $options);
+ $this->add(array(
+ 'name' => 'text',
+ 'type' => 'text',
+ ));
+
+ $this->add(array(
+ 'name' => 'file',
+ 'type' => 'file',
+ ));
+
+ }
+
+ public function getInputFilterSpecification()
+ {
+ return array(
+ 'text' => array(
+ 'required' => true,
+ ),
+ 'file' => array(
+ 'required' => true,
+ 'filters' => array(
+ array(
+ 'name' => 'filerenameupload',
+ 'options' => array(
+ 'target' => __DIR__ . '/testfile.jpg',
+ 'overwrite' => true,
+ )
+ )
+ ),
+ ),
+ );
+ }
+}
BIN  tests/ZendTest/Mvc/Controller/Plugin/TestAsset/nullfile
View
Binary file not shown
61 tests/ZendTest/Stdlib/ArrayUtilsTest.php
View
@@ -143,20 +143,67 @@ public static function invalidArrays()
public static function mergeArrays()
{
return array(
- 'merge-integer-and-string keys' => array(
+ 'merge-integer-and-string-keys' => array(
array(
'foo',
- 3 => 'bar',
- 'baz' => 'baz'
+ 3 => 'bar',
+ 'baz' => 'baz',
+ 4 => array(
+ 'a',
+ 1 => 'b',
+ 'c',
+ ),
),
array(
'baz',
+ 4 => array(
+ 'd' => 'd',
+ ),
),
+ false,
array(
0 => 'foo',
3 => 'bar',
'baz' => 'baz',
- 4 => 'baz'
+ 4 => array(
+ 'a',
+ 1 => 'b',
+ 'c',
+ ),
+ 5 => 'baz',
+ 6 => array(
+ 'd' => 'd',
+ ),
+ )
+ ),
+ 'merge-integer-and-string-keys-preserve-numeric' => array(
+ array(
+ 'foo',
+ 3 => 'bar',
+ 'baz' => 'baz',
+ 4 => array(
+ 'a',
+ 1 => 'b',
+ 'c',
+ ),
+ ),
+ array(
+ 'baz',
+ 4 => array(
+ 'd' => 'd',
+ ),
+ ),
+ true,
+ array(
+ 0 => 'baz',
+ 3 => 'bar',
+ 'baz' => 'baz',
+ 4 => array(
+ 'a',
+ 1 => 'b',
+ 'c',
+ 'd' => 'd',
+ ),
)
),
'merge-arrays-recursively' => array(
@@ -170,6 +217,7 @@ public static function mergeArrays()
'baz'
)
),
+ false,
array(
'foo' => array(
0 => 'baz',
@@ -186,6 +234,7 @@ public static function mergeArrays()
'foo' => 'baz',
'bar' => 'bat'
),
+ false,
array(
'foo' => 'baz',
'bar' => 'bat'
@@ -340,9 +389,9 @@ public function testEmptyArrayReturnsFalse()
/**
* @dataProvider mergeArrays
*/
- public function testMerge($a, $b, $expected)
+ public function testMerge($a, $b, $preserveNumericKeys, $expected)
{
- $this->assertEquals($expected, ArrayUtils::merge($a, $b));
+ $this->assertEquals($expected, ArrayUtils::merge($a, $b, $preserveNumericKeys));
}
/**
Something went wrong with that request. Please try again.