[Form] Mismatched index during merge of params/files after submit form with files/collections/checkbox

[Clement-B]

Symfony version(s) affected

5.4

Description

Since PR #52255 (or #52347), merge of files/params after submitting a form seems to not result in a correct form data when using collection of files AND fields that are not POST for each element.

How to reproduce

I created a PR with a unit test to try to explain at best how we can reproduce this edge case. See #53353

We could certainty found a more concise example, but here is a form configuration where you could reproduce the bug:

1. Use a formType as bellow (field names are added to help) :

• CollectionType (collection)
  • CheckboxType (checkbox)
  • CollectionType (files)
    • FileType (file)

2. Have a root collection with 3 elements
3. Make sure to have a file uploaded in each FileType
4. Have all checkbox unchecked
5. Check the checkbox of item n°2
6. Submit

We got the error "This form should not contain extra fields.".

PS: I didnt get time to create a small project to reproduce issue, sorry.

Possible Solution / Root cause

I dig in into this error, and here is what I found.

Merge of files data and params data are done by static function FormUtil::mergeParamsAndFiles.

Params

When we submit our form with only item n°2 checked, params will only contains thoses values:

[
  "collection" => [
    1 => [
      "checkbox" => "1"
    ]
  ]
]

Very important note here (and maybe the unique cause ?), fields from collections items number 1 and 3 are not submitted here. It's explained as checkbox are never submitted in form when they are unchecked.

Extracted from W3C :

If any of the following conditions are met, then skip these substeps for this element:
[...]
The field element is an input element whose type attribute is in the Checkbox state and whose checkedness is false.

Files

Then we have our files, where each item collection have at least one, so our data when submitting ($_FILES) will look like this:

[
  "collection" => [
    0 => [
      "files" => [
      	0 => FileObject
      ]
    ],
    1 => [
      "files" => [
      	0 => FileObject
      ]
    ],
    2 => [
      "files" => [
      	0 => FileObject
      ]
    ]
  ]
]

Nothing anormal here.

Merge

Here it comes...

During the merge with the new code of FormUtil function, we will got a result like this:

[
  "collection" => [
    1 => [
      "checkbox" => "1",
      "files" => [
      	0 => FileObject
      ]
    ],
    2 => [
      "files" => [
      	0 => FileObject
      ]
    ],
    3 => [
      "files" => [
      	0 => FileObject
      ]
    ],
  ]
]

😮

As you can see, index for root collection have changed. We lost index "0" and we have a new index "3".
Obviously, we will be in trouble here as Symfony will lost data from collection item with index 0 and will try to add a new item.

If we didn't authorize it, we will then get the error about extra fields.

Expected result was:

[
  "collection" => [
    0 => [
      "files" => [
      	0 => FileObject
      ]
    ],
    1 => [
      "checkbox" => "1",
      "files" => [
      	0 => FileObject
      ]
    ],
    2 => [
      "files" => [
      	0 => FileObject
      ]
    ],
  ]
]

Workaround ?

For my use case, it has been simple to get a workaround by just adding a hidden field in the root collection. This way, my params array is always complete with all index and merge is done successfully.

Fix ?

I have tried a quick fix by changing mergeParamsAndFiles methods like this:

   public static function mergeParamsAndFiles(array $params, array $files): array
    {
        $isFilesList = array_is_list($files);

        foreach ($params as $key => $value) {
            if (\is_array($value) && \is_array($files[$key] ?? null)) {
                $params[$key] = self::mergeParamsAndFiles($value, $files[$key]);
                unset($files[$key]);
            }
        }

        if (!$isFilesList) {
            return array_replace($params, $files);
        }

-        foreach ($files as $value) {
-            $params[] = $value;
+        foreach ($files as $key => $value) {
+            $params[$key] = $value;
        }

        return $params;
    }

It work fine in my use case but unfortunately, it cause regressions in the test function testMergeParamsAndFilesMultiple.

What to do next ?

So here is what I see for now:

1. We could revert previous PR [Form] Skip merging params & files if there are no files in the first place #52255, but it seems backtracking to me.
2. We could try to find a better way to fix this but it could be hard as it's difficult to differenciate integer index of existing items and integer index of new added items ?
3. Do nothing as it is easy to use a workaround and seems to be an edge case ?
4. Anything else we could do ?

Who can fix it ?

With some help I could produce a fix as I already created a unit test to reproduce the issue in this PR #53353.

Is this test seems a good start point for you ?
Is this issue enough impacting to trying to create a fix ?
Do you see some more use case where this issue could occur ?

Appreciate your feedbacks 🙂

[alexandre-daubois]

I think that it has been considered as a feature, the keep_as_list option has been added to CollectionType in 7.1: https://symfony.com/doc/7.1/reference/forms/types/collection.html#keep-as-list

[Clement-B]

I think that it has been considered as a feature, the keep_as_list option has been added to CollectionType in 7.1: https://symfony.com/doc/7.1/reference/forms/types/collection.html#keep-as-list

From my understanding and what I see in the feature code, I will say that is not related.

The feature keep_as_list make sure to re-index non consecutive index from a collection where some items were removed.

Whereas the issue here is about a collection of items is partially submitted due to checkboxs being unchecked while collection contains files that are all well submitted.