Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Mime] Throw InvalidArgumentException on invalid form field type inside array#52033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedOct 13, 2023
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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) insideprepareFields, 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 insideprepareFields 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Having all the checks in prepareFields works for me.
90031d3 to995c57bCompare995c57b to08f5d7cCompareThank 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 | MITupdates the test added in#57892 to reflect the change of behavior introduced with#52033Commits-------2e8b9ce fix test
Uh oh!
There was an error while loading.Please reload this page.
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).