Form field values with integer keys not resolved correctly

[claudiu-cristea]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #47504
License	MIT
Doc PR

This should be legit:

new FormDataPart([
    'qux' => [
        [
            'foo' => 'v1',
            'baz' => 'v2',
        ],
        [
            'foo' => 'v3',
            'baz' => 'v4',
    	],
    ],
]);

Fixes #47504

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[claudiu-cristea]

The appveyor failed test seems so unrelated...

[claudiu-cristea]

Regarding the appveyor feature, I see is failing in may other PRs and in 6.2 HEAD as well

[claudiu-cristea]

@HypeMC thank you for your feedback in #38323 and, true, let's move the discussion here. I think I've managed to find a solution that would fix the qux[0][foo] > qux[foo] bug but also the limitation that throws an exception.

[HypeMC]

@HypeMC thank you for your feedback in #38323 and, true, let's move the discussion here. I think I've managed to find a solution that would fix the qux[0][foo] > qux[foo] bug but also the limitation that throws an exception.

@claudiu-cristea The problem with this approach is that it undoes the original PRs intentions. I've created an alternative PR which tries to solve the problem in the HttpBrowser instead.

[claudiu-cristea]

@HypeMC Thank you for following on this.

@claudiu-cristea The problem with this approach is that it undoes the original PRs intentions. I've created an alternative PR which tries to solve the problem in the HttpBrowser instead.

Could you, please, elaborate, what exactly is undoing? Could you give an example of valid data part(s) that is no more possible with this MR? I think having two distinct foo[0], with two different values, in the same time is not valid. That's because of the numeric index. But, yes, having two parts named foo it should be OK.

I'm gonna take a look at #47618

[HypeMC]

Could you, please, elaborate, what exactly is undoing? Could you give an example of valid data part(s) that is no more possible with this MR? I think having two distinct foo[0], with two different values, in the same time is not valid. That's because of the numeric index. But, yes, having two parts named foo it should be OK.

@claudiu-cristea Actually, it depends on how the application you are browsing works. In the past, I have worked with some apis that support having the same form filed multiple times with different values, or even the same query string param multiple times. An example of a valid data part that doesn't work any more:

        $parts[] = $p14 = clone $p1;
-       $p14->setName('quuz2[corge]');
+       $p14->setName('quuz2[0][corge]');
        $parts[] = $p15 = clone $p1;
-       $p15->setName('quuz2[corge]');
+       $p15->setName('quuz2[1][corge]');

[claudiu-cristea]

@HypeMC thank you.

I was looking to RFC 2388, section 3. Definition of multipart/form-data, where it states:

Within a given form, the names are unique.

This seems to me an application of the broader concept of multipart MIME data streams, where multiple parts with the same name are allowed.

But in this class it's about form data, right? So, I think, we should follow RFC 2388.

I did a very simple test with a form:

<form method="post" enctype="multipart/form-data" action="a.php">
  <input type="hidden" name="foo[0]" value="1">
  <input type="hidden" name="foo[0]" value="2">
  <input type="submit">
</form>

and a.php is:

<?php
print_r($_POST);

Submitting this form it shows:

Array ( [foo] => Array ( [0] => 2 ) )

Meaning, in a form, fields with the same name are overriding the previous one. However, looking to the POST request's, I see:

------WebKitFormBoundaryBJfLz3fwSKwJpHAr
Content-Disposition: form-data; name="foo[0]"

1
------WebKitFormBoundaryBJfLz3fwSKwJpHAr
Content-Disposition: form-data; name="foo[0]"

2
------WebKitFormBoundaryBJfLz3fwSKwJpHAr--

But here's about forms, right?

[nicrodgers]

I stumbled upon this PR after encountering many Form field values with integer keys can only have one array element, the key being the field name and the value being the field value, 2 provided. errors from our behat test suite on a Drupal site after running composer update. Thanks for the fix @claudiu-cristea

[claudiu-cristea]

@nicrodgers unfortunately it seems there’s no interest in this issue even it’s a very clear bug. I’ve ended up by using BrowserKit with Guzzle as client. I wonder how it was possible to merge #38323 with such an assumption

[nicolas-grekas]

I've updated the PR to keep support for duplicate keys but only at the root level. The convention is that if at the root level a key is an integer, then we enforce a [$key => $value] structure. This allows building any form with any repeated keys, with one exception: it's not possible to use integers as root-level keys. I think this limitation is not a limiting one in practice.

This changes the behavior introduced in #38323, which was way too disruptive, and unneeded to achieve the target goal.

[nicolas-grekas]

Thank you @claudiu-cristea.