It parses "Content-Disposition: form-data" incorrectly for list arrays

[vedmant]

Following: https://github.com/brefphp/laravel-bridge/issues/189

It parses "Content-Disposition: form-data" incorrectly for list arrays, specifically in arrays like:

references[0][some_id]: 4390954279
references[0][url]: 
references[1][some_id]: 4313323164
references[1][url]: 
references[2][some_id]:
references[2][url]: https://someurl.com/node/745911

It parses into separate array objects:

{
  "references":
  [
    {
      "some_id": "4390954279"
    },
    {
      "url": ""
    },
    {
      "some_id": "4313323164"
    },
    {
      "url": ""
    },
    {
      "some_id": ""
    },
    {
      "url": "https://someurl.com/node/745911"
    }
  ]
}

But it should parse into:

{
  "references":
  [
    {
      "some_id": "4390954279",
      "url": ""
    },
    {
      "some_id": "4313323164",
      "url": ""
    },
    {
      "some_id": "",
      "url": "https://someurl.com/node/745911"
    }
  ],
} 

[mnapoli]

Thank you. Given how extensive/risky the changes are for this specific behavior, it would be great to have unit tests that cover all the different scenarios (in the sense that we have many branches of code and we'd want all of them covered).

Doing that via high level tests would be very verbose and complex, so I'd leave the tests that you've added to cover the main paths, they are great.

Could you add lower level unit tests in Psr7BridgeTest similar to this one: https://github.com/brefphp/bref/blob/30a3e4745859de6c710816df513a6789b89a42b5/tests/Event/Http/Psr7BridgeTest.php#L18 but focused on the new code?

[vedmant]

@mnapoli About the Psr7BridgeTest test, I'll just add the same test but manually creating HttpRequestEvent like:

$event = new HttpRequestEvent(json_decode(file_get_contents('can I still use fixture file?'), true, 512, JSON_THROW_ON_ERROR));
$request = Psr7Bridge::convertRequest($event, Context::fake());
$this->assertEquals([parsed body], $request->getParsedBody());

to avoid using abstractions? What about 2 event versions?

[mnapoli]

Yes exactly, but you wouldn't even have to use an external file (fixture file), you can have the sample data directly in the PHP test for these unit tests.

That allows to create many small (unit) tests to check each scenario. Ideally we'd like some code coverage of all the lines you are adding to the project, because these will be hard to maintain considering the complexity.

[vedmant]

@mnapoli I can add a unit test for that specific function mergeRecursivePreserveNumeric to test possible scenarios of merging, fully isolated.

[vedmant]

@mnapoli I added a separate test only for the mergeRecursivePreserveNumeric function, I think that handles most of the merge cases.

[mnapoli]

OK, we don't want a separate test class, and we don't want to use reflection to test the private method. Here's an example of what would work well:

    public function test xxx()
    {
        $event = new HttpRequestEvent([
          ...
        ]);
        $request = Psr7Bridge::convertRequest($event, Context::fake());
        $this->assertEquals([
          ...
        ], $request->getParsedBody());
    }

And we need enough tests like these to cover all the scenarios that the new code supports (e.g. using code coverage).

[vedmant]

@mnapoli Ok, I added more comprehensive test to Psr7BridgeTest