Skip to content

Correctly handle associative array keys in HTTP client multipart requests#59996

Closed
RomainMazB wants to merge 2 commits intolaravel:13.xfrom
RomainMazB:array-expand-without-data-loss
Closed

Correctly handle associative array keys in HTTP client multipart requests#59996
RomainMazB wants to merge 2 commits intolaravel:13.xfrom
RomainMazB:array-expand-without-data-loss

Conversation

@RomainMazB
Copy link
Copy Markdown
Contributor

Description

This PR fixes an issue where associative array keys may be lost when sending nested array values as multipart data through the HTTP client.

For example, the following payload:

Http::asMultipart()->post('http://foo.com/multipart', [
  'user' => ['name' => 'foo'],
  'admin' => ['name' => 'bar'],
]);

should preserve the nested keys and generate multipart field names such as:

user[name]
admin[name]

Instead, the current Laravel-side multipart expansion may flatten these values using [], which causes the original array keys to be lost. In some cases, when multiple nested arrays contain the same inner key, this can also result in data being overwritten or dropped.

Fixes #59992.

Changes

This PR updates parseMultipartBodyFormat so that keyed array values are expanded using their associative keys instead of being converted to unkeyed array fields.

It also adds a regression test covering the case where two root fields contain the same nested key:

[
  'user' => ['name' => 'foo'],
  'admin' => ['name' => 'bar'],
]

The expected multipart fields are now preserved as:

user[name] = foo
admin[name] = bar

Note about the preferred fix

While this PR fixes the reported issue within Laravel's current multipart parsing logic, I still believe #59984 would be the better long-term solution.

The reason is that this PR adds more Laravel-side logic around multipart array expansion, even though this behavior is now handled directly by Guzzle's MultipartStream in guzzlehttp/psr7 ^2.9.

In other words, this PR demonstrates that the current Laravel implementation does not fully cover the same behavior as Guzzle. However, instead of continuing to duplicate and maintain multipart array expansion logic inside the framework, it would likely be cleaner and more robust to rely on Guzzle for this responsibility.

So although this PR fixes #59992 directly, I would personally prefer reconsidering #59984, which removes the duplicated framework logic and delegates nested multipart array expansion to the underlying HTTP library.

@taylorotwell
Copy link
Copy Markdown
Member

Has conflicts.

@crynobone
Copy link
Copy Markdown
Member

@RomainMazB Can you resend a new PR with resolved merge conflict

@RomainMazB
Copy link
Copy Markdown
Contributor Author

@RomainMazB Can you resend a new PR with resolved merge conflict

It's unnecessary, now that #59984 was reopened and merged, this issue doesn't exist anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multipart array index and data loss in HTTP client

3 participants