Skip to content

[HttpFoundation] Map multipart/form-data as form Content-Type #43017

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 19, 2021

Conversation

keichinger
Copy link
Contributor

@keichinger keichinger commented Sep 14, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets #34240
License MIT
Doc PR

As per RFC 2045 and RFC 2388 (see https://www.w3.org/TR/html401/interact/forms.html#h-17.13.4.2), the multipart/form-data Content-Type should be used when submitting a mixed forms containing files, binary data and non-ASCII data.
Currently, the same logic that would run in regular, non-mixed form scenarios can't run for mixed forms, as soon as it relies on the result of Request::getContentType() or anything else that might rely on Request::$formats without going low-level and avoiding the implemented abstraction. This PR fixes that.

Resolves #34240

@keichinger
Copy link
Contributor Author

I couldn't find many tests Request::$formats. The only test for Request::getContentType() is a null test for the default value. Other than that, I've extended the Request::getMimeType(s) test, which also rely on Request::$formats, with a test-case for the form MIME types.

If there's any other test that I've missed, please let me know and I'll update it accordingly.

@keichinger
Copy link
Contributor Author

The failing test seems unrelated to this PR.

Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks correct, but I'm unsure about the target.

  • If we file this as a bugfix, we should target 4.4
  • If we file this as an improvement, we should target 5.4

@wouterj
Copy link
Member

wouterj commented Sep 14, 2021

We're trying to be on the more cautious side to judge whether something is a bug or a feature since some time. So I would say this "bug fixing feature" should target 5.4.

As per RFC 2045  and RFC 2388, the `multipart/form-data` Content-Type should be used when submitting a mixed form containing files, binary data and non-ASCII data.

This commit helps with infrastructure that is directly checking against the `Request::getContentType()` method in scenarios, where a mixed form has been submitted.

Resolves symfony#34240
@keichinger
Copy link
Contributor Author

@wouterj I've rebased the PR to 5.4 as requested.

Thanks for the feedback to both of you ☺️

@fabpot fabpot modified the milestones: 5.3, 5.4 Sep 19, 2021
@fabpot
Copy link
Member

fabpot commented Sep 19, 2021

Thank you @keichinger.

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

Successfully merging this pull request may close these issues.

[HttpFoundation] Request::getContentType doesn't support multipart/form-data
6 participants