NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Open

priyadi wants to merge13 commits intosymfony:5.4

base:5.4

Choose a base branch

frompriyadi:dev-fix-merge-options-form

Open

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

priyadi wants to merge13 commits intosymfony:5.4frompriyadi:dev-fix-merge-options-form

Conversation

Copy link

Contributor

priyadi commentedMar 18, 2024•
edited
Loading

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix#53359
License	MIT

This is an update to the PR#53353

This fixes another value-files merging bug involving collections of a form containing a file upload field and a checkbox.

Clément BOURREAUand others added2 commits

March 18, 2024 20:33

add failed test for partial data submitting with collections

597b6a3

refactor param & files merging

f943adb

priyadi requested review fromxabbuh andyceruto ascode owners

March 18, 2024 18:26

carsonbot added Status: Needs Review Bug Form labels

Mar 18, 2024

carsonbot added this to the5.4 milestone

Mar 18, 2024

Copy link

ContributorAuthor

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

ContributorAuthor

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

src/Symfony/Component/Form/Util/ParamFilesMerger.php OutdatedShow resolvedHide resolved

priyadi added5 commits

March 24, 2024 16:45

apply all suggestions

55deed0

cs

aec4b56

revert typehint removal

21fbb71

remove unneeded@params

60008f4

assorted changes

30c95b8

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Instead of introducing this new internal class the code should be moved into private methods inFormUtil as the class is only used there.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

The class represents a node in the tree. It is used by itself to traverse the entire tree. The task will be very complicated without a dedicated class.

https://github.com/symfony/symfony/pull/54324/files/30c95b8c9396624a98f6e44b0f83889b77112ab7#diff-5983ae1bcb319a7efa00aa62df7a2bcfaeaabd1894959ed7994d30a79f2281deR120

Copy link

Member

Choose a reason for hiding this comment

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

I have sent a PR against your fork which inlines the methods from theParamFilesMerger class.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I have merged your PR

xabbuhand others added2 commits

May 17, 2024 13:42

inline ParamFilesMerger methods

ac24610

cs fix

1cf2c5f

priyadi force-pushed thedev-fix-merge-options-form branch from4a6078e to1cf2c5fCompare

May 17, 2024 12:17

fabpot modified the milestones:5.4,6.4

Dec 14, 2024

Copy link

remyvanlerberghe commentedMar 20, 2025

Hi ! I noticed this PR has been open for about a year now. The bug fix you've provided here could really help us out! Would it be possible to get this moving forward again? Happy to help with anything that might be needed to get this merged

Copy link

ContributorAuthor

priyadi commentedMar 20, 2025

Hi ! I noticed this PR has been open for about a year now. The bug fix you've provided here could really help us out! Would it be possible to get this moving forward again? Happy to help with anything that might be needed to get this merged

I think the only thing left to do is for a maintainer to merge this PR. In the meantime, you can help by applying the fix to your project and verify if it resolves your problem.

stof requested changes

Mar 20, 2025

View reviewed changes

src/Symfony/Component/Form/Util/FormUtil.php

		}

		return $params;
		}

		/**
		* @param UploadedFile\|array $value

Copy link

Member

stofMar 20, 2025

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, theis_array check on line 122 would be totally useless btw)

src/Symfony/Component/Form/Tests/AbstractRequestHandlerTestCase.php

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

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

Copy link

Member

stofMar 20, 2025

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')

src/Symfony/Component/Form/Tests/AbstractRequestHandlerTestCase.php

		$this->assertArrayHasKey(1, $data);

		$this->assertEquals('test', $itemsForm->get('1')->get('item')->getData());
		$this->assertNotNull($itemsForm->get('0')->get('file'));

Copy link

Member

stofMar 20, 2025

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

stofMar 20, 2025

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.

src/Symfony/Component/Form/Util/FormUtil.php

		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

stofMar 20, 2025

Choose a reason for hiding this comment

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

Do we need this wholemergeAtPath 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.

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Mar 20, 2025

Labels

Bug Form Status: Needs Work

8 participants

		@@ -30,8 +30,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

Movatterモバイル変換

Uh oh!

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

priyadi commentedMar 18, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

priyadi commentedMar 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

priyadi commentedMay 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remyvanlerberghe commentedMar 20, 2025

Uh oh!

priyadi commentedMar 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

priyadi commentedMar 18, 2024•
edited
Loading