Skip to content

[Form] Fix form value merging involving file upload, collection & checkbox #54324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: 5.4
Choose a base branch
from
+136 −15
Open

[Form] Fix form value merging involving file upload, collection & checkbox #54324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
597b6a3
add failed test for partial data submitting with collections
Jan 2, 2024
f943adb
refactor param & files merging
priyadi Mar 18, 2024
fdc3e01
cs fix
priyadi Mar 18, 2024
79fdbaa
type hints, etc
priyadi Mar 19, 2024
0c9b3d7
simplify
priyadi Mar 19, 2024
f3b5efd
cs
priyadi Mar 19, 2024
55deed0
apply all suggestions
priyadi Mar 24, 2024
aec4b56
cs
priyadi Mar 24, 2024
21fbb71
revert typehint removal
priyadi Mar 24, 2024
60008f4
remove unneeded @params
priyadi Mar 24, 2024
30c95b8
assorted changes
priyadi Mar 24, 2024
ac24610
inline ParamFilesMerger methods
xabbuh May 15, 2024
1cf2c5f
cs fix
priyadi May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions src/Symfony/Component/Form/Tests/AbstractRequestHandlerTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,53 @@ public function testMergeZeroIndexedCollection()
$this->assertNotNull($itemsForm->get('0')->get('file'));
}

public function testMergePartialDataFromCollection()
{
$form = $this->createForm('root', 'POST', true);
$form->add('items', CollectionType::class, [
'entry_type' => ItemFileType::class,
'allow_add' => true,
]);

$file = $this->getUploadedFile();
$file2 = $this->getUploadedFile();

$this->setRequestData('POST', [
'root' => [
'items' => [
1 => [
'item' => 'test',
],
],
],
], [
'root' => [
'items' => [
0 => [
'file' => $file,
],
1 => [
'file' => $file2,
],
],
],
]);

$this->requestHandler->handleRequest($form, $this->request);

$itemsForm = $form->get('items');
$data = $itemsForm->getData();
$this->assertTrue($form->isSubmitted());
$this->assertTrue($form->isValid());

$this->assertCount(2, $data);
$this->assertArrayHasKey(0, $data);
$this->assertArrayHasKey(1, $data);

$this->assertEquals('test', $itemsForm->get('1')->get('item')->getData());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this misses assertions on $itemsForm->get('0')->get('name') and $itemsForm->get('1')->get('file')

$this->assertNotNull($itemsForm->get('0')->get('file'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this assert the form data is not null instead of asserting that the Form instance is not null ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, testMergeZeroIndexedCollection seems to have the same issue.

}

/**
* @dataProvider methodExceptGetProvider
*/
Expand Down
104 changes: 89 additions & 15 deletions src/Symfony/Component/Form/Util/FormUtil.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\Form\Util;

use Symfony\Component\HttpFoundation\File\UploadedFile;

/**
* @author Bernhard Schussek <bschussek@gmail.com>
*/
Expand All @@ -30,8 +32,6 @@ private function __construct()
* a form and needs to be consistent. PHP keyword `empty` cannot
* be used as it also considers 0 and "0" to be empty.
*
* @param mixed $data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fabbot insists that I remove it. I reverted the changes, but it won't pass the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the change has not been reverted

*
* @return bool
*/
public static function isEmpty($data)
Expand All @@ -43,30 +43,104 @@ public static function isEmpty($data)
}

/**
* Recursively replaces or appends elements of the first array with elements
* of second array. If the key is an integer, the values will be appended to
* the new array; otherwise, the value from the second array will replace
* the one from the first array.
* Merges query string or post parameters with uploaded files.
*/
public static function mergeParamsAndFiles(array $params, array $files): array
{
$isFilesList = array_is_list($files);
return self::mergeAtPath($params, $files);
}

private static function mergeAtPath(array $params, array $files, array $path = [])
{
$paramsValue = self::getValueAtPath($params, $path);
$filesValue = self::getValueAtPath($files, $path);

if (null === $paramsValue) {
return $filesValue;
}

if (\is_array($paramsValue) && self::isFileUpload($filesValue)) {
return $filesValue; // if the array is a file upload field, it has the precedence
}

if (\is_array($paramsValue) && \is_array($filesValue)) {
// if both are lists and both does not contain array, then merge them and return
if (array_is_list($paramsValue) && self::doesNotContainNonFileUploadArray($paramsValue) && array_is_list($filesValue) && self::doesNotContainNonFileUploadArray($filesValue)) {
return array_merge($paramsValue, $filesValue);
}

// heuristics to preserve order, the bigger array wins
if (\count($filesValue) > \count($paramsValue)) {
$keys = array_unique(array_merge(array_keys($filesValue), array_keys($paramsValue)));
} else {
$keys = array_unique(array_merge(array_keys($paramsValue), array_keys($filesValue)));
}

$result = [];

foreach ($keys as $key) {
$subPath = $path;
$subPath[] = $key;

foreach ($params as $key => $value) {
if (\is_array($value) && \is_array($files[$key] ?? null)) {
$params[$key] = self::mergeParamsAndFiles($value, $files[$key]);
unset($files[$key]);
$result[$key] = self::mergeAtPath($params, $files, $subPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this whole mergeAtPath here which then involves reading the data at a nested path, making the whole logic more complex ?

We could be looping over all keys (which is what seemed missing before) while still merging values by passing the values at that level instead.

}

return $result;
}

if (\is_array($paramsValue)) {
return $paramsValue; // params has the precedence
}

if (!$isFilesList) {
return array_replace($params, $files);
if (self::isFileUpload($filesValue)) {
return $filesValue; // if the array is a file upload field, it has the precedence
}

foreach ($files as $value) {
$params[] = $value;
return $paramsValue;
}

private static function getValueAtPath(array $params, array $path)
{
foreach ($path as $key) {
if (null === $params = $params[$key] ?? null) {
return null;
}
}

return $params;
}

/**
* @param UploadedFile|array $value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type is wrong. The value can be anything (otherwise, the is_array check on line 122 would be totally useless btw)

*/
private static function isFileUpload($value): bool
{
if ($value instanceof UploadedFile) {
return true;
}

if (!\is_array($value) || !\in_array(\count($value), [5, 6], true)) {
return false;
}

if (\array_key_exists('full_path', $value)) {
unset($value['full_path']);
}

$keys = array_keys($value);
sort($keys);

return ['error', 'name', 'size', 'tmp_name', 'type'] === $keys;
}

private static function doesNotContainNonFileUploadArray(array $array): bool
{
foreach ($array as $value) {
if (\is_array($value) && !self::isFileUpload($value)) {
return false;
}
}

return true;
}
}
Loading