[Mime] Allow multiple parts with the same name in FormDataPart #38323
Conversation
|
I think it would be better to replace the constructor argument and use a dedicated data structure instead. My proposal would be this one: $f1 = new FormDataPart([
['foo' => 'one'],
['foo' => 'two'],
]);the logic would be: when the main array has a numeric key, then enforce |
|
@nicolas-grekas The PR has been updated according to your suggestion. A limitation of this approach in comparison to the previous one is if the field name is actually meant to be an integer, eg the following will now throw an exception: $f1 = new FormDataPart([
'1' => [
'a' => 'one',
'b' => 'two',
],
]);The only way to achieve something like this now would be to manually name the fields appropriately: $f1 = new FormDataPart([
['1[a]' => 'one'],
['1[b]' => 'two'],
]);Since I doubt using integers as field names is common, this shouldn't be an issue, nonetheless I wanted to point it out just in case. |
There was a problem hiding this comment.
The only way to achieve something like this now would be to manually name the fields appropriately: [...]
Correct. I also think we can do the change without any practical BC impact. The good news is that if one still wants to use numeric names (who would, but anyway), then it's still possible.
|
@HypeMC could you please consider sending a PR to the doc to tell about this behavior? |
|
@nicolas-grekas Yep, sure, will do this week, tnx. |
|
This looks like a new feature to me btw. We should merge it in master. (now rebased for master) |
|
@nicolas-grekas This looks more like a bug to me since it's not possible to make a valid request with the current code. Also, when considering that the actual Without this fix, users using Sf4 won't be ably to make valid requests until they upgrade. Since some only use LTS versions in their production apps, it'll be some time till they get this fix. |
|
Unfortunately, that's a new feature and as such it must be merged in master. There is no such thing as "a couple of lines" and "no BC breaks" in semantic versioning. |
|
Thank you @HypeMC. |
|
@fabpot I guess I understand your side as well, I think it really boils down to whether the In any case, there is a workaround for Sf4 which I mentioned in #38258 so it's not a big deal. Just wanted to give you my 2 cents 😄 |
…gunApiTransport (starred-gijs) This PR was merged into the 6.1 branch. Discussion ---------- [Mailer] [Mailgun] Allow multiple TagHeaders with MailgunApiTransport | Q | A | ------------- | --- | Branch? | 6.1 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Mailgun allows you to set [3 tags per message](https://documentation.mailgun.com/en/latest/user_manual.html#tagging) but the current implementation in MailgunApiTransport, any consecutive TagHeader will override the previous one. Because it uses FormDataPart to build the payload, by using a integer key, it allows to set multiple parts with the same name #38323 The MailgunHttpTransport already supports multiple TagHeaders and I added a test to prove that. Commits ------- fa65173 [Mailer][Mailgun] Allow multiple tags with MailgunApiTransport
@HypeMC, meaning that these fields are no more valid? Why? <input type="hidden" name="bar[0][foo]" value="v1">
<input type="hidden" name="bar[0][baz]" value="v2">After I've updated a dependency that now requires
EDIT: Specifically, it happens in Behat tests and this is my chain of dependencies: |
|
@claudiu-cristea As mentioned in this PR, it is possible, just requires a different syntax: $f1 = new FormDataPart([
'bar[0]' => [
'foo' => 'v1',
'baz' => 'v2',
],
]);
var_dump($f1->toString()); |
|
@HypeMC, well this is transparent to me. I'm not creating that array, it's created under the hood. I'm only posting such a form. So, where should occur this change? |
|
@HypeMC, instead of throwing the exception, why not solving the issue? Wouldn't this change fix it? diff --git a/vendor/symfony/mime/Part/Multipart/FormDataPart.php b/vendor/symfony/mime/Part/Multipart/FormDataPart.php
--- a/vendor/symfony/mime/Part/Multipart/FormDataPart.php
+++ b/vendor/symfony/mime/Part/Multipart/FormDataPart.php (date 1662536878627)
@@ -59,12 +59,16 @@
$prepare = function ($item, $key, $root = null) use (&$values, &$prepare) {
if (\is_int($key) && \is_array($item)) {
- if (1 !== \count($item)) {
- throw new InvalidArgumentException(sprintf('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, %d provided.', \count($item)));
- }
-
- $key = key($item);
- $item = $item[$key];
+ if (1 === \count($item)) {
+ $key = key($item);
+ $item = $item[$key];
+ } else {
+ $items = [];
+ foreach ($item as $item_key => $itemValue) {
+ $items[$key . "[$item_key]"] = $itemValue;
+ }
+ array_walk($items, $prepare, $root);
+ }
}
$fieldName = null !== $root ? sprintf('%s[%s]', $root, $key) : $key; |
|
@HypeMC, yet another problem with this change: the name in form is |
Yes, that's the expected behavior for that particular data structure, eg: $f1 = new FormDataPart([
'qux' => [
['value' => 'v1'],
],
]);is suppose to produce: This was done so you could have multiple parts with the same name, eg: $f1 = new FormDataPart([
'qux' => [
['value' => 'v1'],
['value' => 'v2'],
],
]);Not sure what the best course of action would be here, perhaps the bug could be patched in the |
This is wrong, I'm expecting |
|
Yes, I understand that, I'm just explaining what the original idea was, for your case one would do: $f1 = new FormDataPart([
'qux[0]' => [
['value' => 'v1'],
['value' => 'v2'],
],
]);But since you're not using |
Added the possibility to choose whether multiple parts with the same name should be suffixed or not:
Only applies to numeric keys: