[Mime] Throw InvalidArgumentException on invalid form field type inside array #52033
Conversation
|
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:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| @@ -75,6 +75,10 @@ private function prepareFields(array $fields): array | |||
| return; | |||
| } | |||
|
|
|||
| if (!\is_string($item) && !$item instanceof TextPart) { | |||
There was a problem hiding this comment.
Maybe we can move this check to the constructor directly? That would be more consistent as we already check some of that there.
There was a problem hiding this comment.
I also considered this, but in my opinion it has two disadvantages:
- We have to traverse through the array recursively. In
prepareFieldsthis is already the case, but in the constructor we only traverse at top-level. - We don't have the field name in form of "foo[qux][quux]" in the constructor. Either we cannot include it in the error message, or we have to generate it redundantly.
Also we already have a check (regarding the form field values with integer keys) inside prepareFields, so I don't feel like adding another check there affects the consistency much.
How about we remove the check in the constructor altogether? All the checks could be done inside prepareFields and the constructor would look like this:
public function __construct(array $fields = [])
{
parent::__construct();
$this->fields = $fields;
// HTTP does not support \r\n in header values
$this->getHeaders()->setMaxLineLength(\PHP_INT_MAX);
}
There was a problem hiding this comment.
Having all the checks in prepareFields works for me.
90031d3 to
995c57b
Compare
995c57b to
08f5d7c
Compare
|
Thank you @l-naumann. |
This PR was merged into the 6.4 branch. Discussion ---------- [Mime] fix test | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | | License | MIT updates the test added in #57892 to reflect the change of behavior introduced with #52033 Commits ------- 2e8b9ce fix test
Example code:
Currently, when you use a disallowed type inside an array, a TypeError is thrown:
Symfony\Component\Mime\Part\Multipart\FormDataPart::configurePart(): Argument #2 ($part) must be of type Symfony\Component\Mime\Part\TextPart, int givenThis change adds a clearly understandable error message:
The value of the form field "foo[qux][quux]" can only be a string, an array, or an instance of TextPart ("int" given).