[Form] Fix form value merging involving file upload, collection & checkbox #54324
Conversation
|
paging @nicolas-grekas |
76fef46 to
fdc3e01
Compare
| @@ -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 | |||
There was a problem hiding this comment.
fabbot insists that I remove it. I reverted the changes, but it won't pass the test.
|
@nicolas-grekas bump. Is there anything else I need to do before this PR can be merged? |
| * | ||
| * @internal | ||
| */ | ||
| class ParamFilesMerger |
There was a problem hiding this comment.
Instead of introducing this new internal class the code should be moved into private methods in FormUtil as the class is only used there.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have sent a PR against your fork which inlines the methods from the ParamFilesMerger class.
There was a problem hiding this comment.
I have merged your PR
4a6078e to
1cf2c5f
Compare
|
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. |
| } | ||
|
|
||
| return $params; | ||
| } | ||
|
|
||
| /** | ||
| * @param UploadedFile|array $value |
There was a problem hiding this comment.
this type is wrong. The value can be anything (otherwise, the is_array check on line 122 would be totally useless btw)
| $this->assertArrayHasKey(0, $data); | ||
| $this->assertArrayHasKey(1, $data); | ||
|
|
||
| $this->assertEquals('test', $itemsForm->get('1')->get('item')->getData()); |
There was a problem hiding this comment.
this misses assertions on $itemsForm->get('0')->get('name') and $itemsForm->get('1')->get('file')
| $this->assertArrayHasKey(1, $data); | ||
|
|
||
| $this->assertEquals('test', $itemsForm->get('1')->get('item')->getData()); | ||
| $this->assertNotNull($itemsForm->get('0')->get('file')); |
There was a problem hiding this comment.
shouldn't this assert the form data is not null instead of asserting that the Form instance is not null ?
There was a problem hiding this comment.
btw, testMergeZeroIndexedCollection seems to have the same issue.
| 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); |
There was a problem hiding this comment.
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.
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.