[HttpFoundation] Add support for parsing non-POST requests using request_parse_body() (PHP 8.4) #59358

rottifant · 2025-01-03T22:54:54Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #59331
License	MIT

Introduce initial support for parsing multipart/form-data in PUT/PATCH requests using PHP 8.4's request_parse_body() function.
This function can also be used for application/x-www-form-urlencoded.

Problems & Todos:

request_parse_body() is only available PHP >= 8.4.0 --> Backwards compatibility?
The existing tests fail (in environment with PHP 8.4) because application/x-www-form-urlencodedwill also use the new implementation. They should be adapted?
New tests for multipart/form-data should be implemented.
Check if request_parse_body() is good in createFromGlobals() or if it should be implemented somewhere else (too?)?
Define the behavior when an exception is throw by the function

Unfortunately I could not find a way to test the new implementation properly.
Looking forward to your ideas & feedback, thx.

…ateFromGlobals() Introduce initial support for parsing `multipart/form-data` in PUT/PATCH requests using PHP 8.4's `request_parse_body()` function. Known issue: Some tests fail due to missing Content-Type handling and lack of proper test setup for PUT/PATCH requests. These will be addressed in follow-up changes.

carsonbot · 2025-01-03T22:55:00Z

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:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.3 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

OskarStark · 2025-01-04T07:18:57Z

You need to add a check for the php version then if it is only available in 8.4

src/Symfony/Component/HttpFoundation/Request.php

rottifant · 2025-01-04T11:04:32Z

The problem is that the tests fail because they were designed for the old solution with $request->getContent(). Additionally, the existing tests only cover requests tests for application/x-www-form-urlencoded, not multipart/form-data.
The exception thrown originates from request_parse_body(): RequestParseBodyException: Request does not provide a content type because (as far as I understand) it is a test environment without a real request.
Setting $_SERVER['CONTENT_TYPE'] has no effect.

At the moment, I’m unsure how to properly adapt or rewrite the tests to handle this scenario. I would appreciate any suggestions or ideas on how to proceed with testing for the new implementation (and with multipart/form-data) in this context.

src/Symfony/Component/HttpFoundation/Request.php

rottifant · 2025-01-04T13:21:25Z

I did some "manual tests" with a simple controller to see what the output is.

The controller:

class TestController
{
    #[Route('/test')]
    public function test(Request $request): JsonResponse
    {
        return new JsonResponse([
            'method' => $request->getMethod(),
            'content_type' => $request->headers->get('content-type'),
            'request_all' => $request->request->all(),
            'request_get_content' => $request->getContent(),
            'payload_all' => $request->getPayload()->all(),
            'files_all' => $request->files->all(),
        ]);
    }
}

The requests (sent with cURL):

curl --location --request PUT 'http://127.0.0.1:8000/test' \
--header 'Content-Type: application/x-www-form-urlencoded' \
--data-urlencode 'foo=foovalue' \
--data-urlencode 'bar=barvalue'

curl --location --request PUT 'http://127.0.0.1:8000/test' \
--form 'foo="foovalue"' \
--form 'bar="barvalue"' \
--form 'baz=@"/Users/rott/textfile.txt"'

with textfile.txt:
Symfony is cool

Method	Content-Type	Conclusion	Result with old implementation	Result with new implementation
POST	`x-www-form-urlencoded`	nothing different	`{"method":"POST","content_type":"application\/x-www-form-urlencoded","request_all":{"foo":"foovalue","bar":"barvalue"},"request_get_content":"foo=foovalue\u0026bar=barvalue","payload_all":{"foo":"foovalue","bar":"barvalue"},"files_all":[]}`	`{"method":"POST","content_type":"application\/x-www-form-urlencoded","request_all":{"foo":"foovalue","bar":"barvalue"},"request_get_content":"foo=foovalue\u0026bar=barvalue","payload_all":{"foo":"foovalue","bar":"barvalue"},"files_all":[]}`
PUT	`x-www-form-urlencoded`	nothing different	`{"method":"PUT","content_type":"application\/x-www-form-urlencoded","request_all":{"foo":"foovalue","bar":"barvalue"},"request_get_content":"foo=foovalue\u0026bar=barvalue","payload_all":{"foo":"foovalue","bar":"barvalue"},"files_all":[]}`	`{"method":"PUT","content_type":"application\/x-www-form-urlencoded","request_all":{"foo":"foovalue","bar":"barvalue"},"request_get_content":"foo=foovalue\u0026bar=barvalue","payload_all":{"foo":"foovalue","bar":"barvalue"},"files_all":[]}`
POST	`form-data`	nothing different	`{{"method":"POST","content_type":"multipart\/form-data; boundary=--------------------------430970386763090088183934","request_all":{"foo":"foovalue","bar":"barvalue"},"request_get_content":"","payload_all":{"foo":"foovalue","bar":"barvalue"},"files_all":{"baz":{}}}`	`{"method":"POST","content_type":"multipart\/form-data; boundary=--------------------------907910549111218239990088","request_all":{"foo":"foovalue","bar":"barvalue"},"request_get_content":"","payload_all":{"foo":"foovalue","bar":"barvalue"},"files_all":{"baz":{}}}`
PUT	`form-data`	With the new implementation: 1. The body is parsed and files are available (while before the request was only available unparsed via `$request->getContent()`) 2. The usage of `$request->getPayload()->all()` is also possible and `Symfony\Component\HttpKernel\Exception\BadRequestHttpException` is also not thrown anymore.	Important: `$request->getPayload()->all()` was removed from the response in this case because it threw `Symfony\Component\HttpKernel\Exception\BadRequestHttpException: "Could not decode request body."` {"method":"PUT","content_type":"multipart\/form-data; boundary=--------------------------503204391586777501976026","request_all":[],"request_get_content":"----------------------------503204391586777501976026\r\nContent-Disposition: form-data; name=\u0022foo\u0022\r\n\r\nfoovalue\r\n----------------------------503204391586777501976026\r\nContent-Disposition: form-data; name=\u0022bar\u0022\r\n\r\nbarvalue\r\n----------------------------503204391586777501976026\r\nContent-Disposition: form-data; name=\u0022baz\u0022; filename=\u0022textfile.txt\u0022\r\nContent-Type: text\/plain\r\n\r\nSymfony is cool\r\n----------------------------503204391586777501976026--\r\n","files_all":[]}	`{"method":"PUT","content_type":"multipart\/form-data; boundary=--------------------------249741139464139291005574","request_all":{"foo":"foovalue","bar":"barvalue"},"request_get_content":"","payload_all":{"foo":"foovalue","bar":"barvalue"},"files_all":{"baz":{}}}`

MatTheCat · 2025-01-05T10:21:48Z

Setting $_SERVER['CONTENT_TYPE'] has no effect.

Indeed you’d need $_SERVER['HTTP_CONTENT_TYPE']: https://www.php.net/manual/en/reserved.variables.server.php#refsect1-reserved.variables.server-description

rottifant · 2025-01-05T11:31:06Z

Setting $_SERVER['CONTENT_TYPE'] has no effect.

Indeed you’d need $_SERVER['HTTP_CONTENT_TYPE']: https://www.php.net/manual/en/reserved.variables.server.php#refsect1-reserved.variables.server-description

Thanks for the notice, but unfortunately this also doesn't change it.
Even if I set $_SERVER['HTTP_CONTENT_TYPE'] right before calling request_parse_body() RequestParseBodyException: Request does not provide a content type is thrown.

I am a little bit confused because $_SERVER['CONTENT_TYPE'] is used in the test:

symfony/src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

Line 1353 in ae5843f

$_SERVER['CONTENT_TYPE'] = 'application/x-www-form-urlencoded';

MatTheCat · 2025-01-05T11:41:43Z

Woops indeed my bad I missed f16e206 😅

nicolas-grekas

request_parse_body() is only available PHP >= 8.4.0 --> Backwards compatibility?

No need for a polyfill IMHO, upgrading PHP is the way to go to get support for this.

The existing tests fail (in environment with PHP 8.4) because application/x-www-form-urlencodedwill also use the new implementation. They should be adapted?

We should split the failing test cases and skip them on PHP 8.4

New tests for multipart/form-data should be implemented.

Given the way the PHP 8.4 function is implemented, this should be done using a php -S subprocess

Check if request_parse_body() is good in createFromGlobals() or if it should be implemented somewhere else (too?)?

Looks good to me.

nicolas-grekas · 2025-01-05T21:04:07Z

src/Symfony/Component/HttpFoundation/Request.php

+        if (
+            \in_array($method, ['PUT', 'DELETE', 'PATCH'], true)
+            && (
+                str_starts_with($contentType, 'application/x-www-form-urlencoded')


I'd let this check on the content-type to the request_parse_body function, no need to implement it ourselves

What do you mean?
A content type check is required because if, for example, a request with content-type application/json is passed to the function, it will throw RequestParseBodyException: Content-Type "application/json" is not supported. Or are we going to catch and handle it? RequestParseBodyException is also thrown with invalid content, not just unsupported.

I mean the check for the content-type: we can catch the exception and deal with it instead of checking the content-type ourselves (and diverging from the native check).
We have to add logic to deal with the exception anyway, we cannot let it bubble down unhandled (createFromGlobals isn't expected to throw such exceptions).

Sorry, my bad. I overlooked the "on the content-type" in your message...
I checked this because RequestParseBodyException is also thrown with invalid content, not just unsupported Content-Type. Or are we going to differentiate this?

src/Symfony/Component/HttpFoundation/Request.php

nicolas-grekas · 2025-01-05T21:07:37Z

Oh, and we need to define the behavior when an exception is throw by the function!

rottifant · 2025-01-05T21:37:12Z

@nicolas-grekas Thank you for the input.
What would you suggest to do when an Exception is thrown? Just leave request and files empty?

nicolas-grekas · 2025-01-05T21:39:29Z

I guess so. We should behave as close as how PHP behaves with other methods.

MatTheCat · 2025-01-07T12:51:22Z

Just a thought I had: wouldn’t the Runtime component be a better place for request_parse_body? As its goal is allegedly to populate $_POST and $_FILES it feels like it should be called before Request::createFromGlobals?

rottifant · 2025-01-07T15:07:13Z

Just a thought I had: wouldn’t the Runtime component be a better place for request_parse_body? As its goal is allegedly to populate $_POST and $_FILES it feels like it should be called before Request::createFromGlobals?

Good point, but another thought from me: I originally came across this via Laravel, which is using Symfony as foundation for requests. For instance, Laravel relies on Symfony’s Request class for parsing requests. If this functionality were implemented in the Request class, Laravel and other frameworks leveraging Symfony’s components could benefit from it as well, given that they don’t rely on the Runtime component, as far as I understand

nicolas-grekas · 2025-01-17T12:47:49Z

Up to move this PR forward @rottifant ?

rottifant · 2025-01-17T13:20:16Z

Up to move this PR forward @rottifant ?

Yes, I will continue in the next days.

florian2peaches · 2025-03-26T09:23:20Z

What's the current problem? I would love to have this fix/feature, maybe I can help.

rottifant · 2025-03-26T09:33:11Z

@florian2peaches
I'm sorry. I am currently working on many projects and I don't have the time.

I would be very happy if you could take it over :-) The implementation itself seems to work. Test tests that cover this new function request_parse_body still need to be added. I can give you access to my fork if you want.

florian2peaches · 2025-03-26T12:18:56Z

The exception thrown originates from request_parse_body(): RequestParseBodyException: Request does not provide a content type because (as far as I understand) it is a test environment without a real request.
Setting $_SERVER['CONTENT_TYPE'] has no effect.

That's still turning CI red, any more info on that?

rottifant · 2025-03-26T13:23:11Z

The exception thrown originates from request_parse_body(): RequestParseBodyException: Request does not provide a content type because (as far as I understand) it is a test environment without a real request.
Setting $_SERVER['CONTENT_TYPE'] has no effect.

That's still turning CI red, any more info on that?

Yes. As far as I found out, there is currently no way to test and simulate request_parse_body without a HTTP real request.
I thought about starting a server (php -S) when testing and sending real HTTP Requests to it to test the implementation. But tbh I do not have much experience with PHPUnit, don't exactly know how this should be done and if it is even possible to start the server in the test – especially with different environments/OS.

florian2peaches · 2025-04-09T06:05:00Z

I'm on the same lane, I know php works that way so request_parse_body internally accesses the request, but isn't there a way to inject a request internally or some kind of polyfill for request_parse_body?

rottifant · 2025-04-09T17:26:23Z

Unfortunately that was also the point where I got stuck and had no other idea than the "real" http server. Maybe the Symfony team has an idea how to implement this in the tests?
As the function itself is relatively new, I could not find any information on how to test it properly.

carsonbot added Status: Needs Review Feature HttpFoundation labels Jan 3, 2025

carsonbot added this to the 7.3 milestone Jan 3, 2025

rottifant marked this pull request as draft January 3, 2025 22:55

symfony deleted a comment from carsonbot Jan 4, 2025

rottifant commented Jan 4, 2025

View reviewed changes

src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved

[HttpFoundation] Usage of PHP_VERSION_ID

b6ebc60

fabpot reviewed Jan 4, 2025

View reviewed changes

src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved

[HttpFoundation] CS Patch, no overwriting of globals

699e21e

nicolas-grekas reviewed Jan 5, 2025

View reviewed changes

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Jan 5, 2025

Merge branch 'symfony:7.3' into 7.3

2be5ac2

crynobone mentioned this pull request Mar 27, 2025

Non-POST request body with type multipart/form-data is not present laravel/framework#55174

Open

MatTheCat mentioned this pull request May 23, 2025

Add support for QUERY method #60521

Open

Uh oh!

[HttpFoundation] Add support for parsing non-POST requests using request_parse_body() (PHP 8.4) #59358

Are you sure you want to change the base?

[HttpFoundation] Add support for parsing non-POST requests using request_parse_body() (PHP 8.4) #59358

Uh oh!

Conversation

rottifant commented Jan 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carsonbot commented Jan 3, 2025

Uh oh!

OskarStark commented Jan 4, 2025

Uh oh!

Uh oh!

rottifant commented Jan 4, 2025

Uh oh!

Uh oh!

rottifant commented Jan 4, 2025

Uh oh!

MatTheCat commented Jan 5, 2025

Uh oh!

rottifant commented Jan 5, 2025

Uh oh!

MatTheCat commented Jan 5, 2025

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jan 5, 2025

Choose a reason for hiding this comment

Uh oh!

rottifant Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jan 5, 2025

Choose a reason for hiding this comment

Uh oh!

rottifant Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolas-grekas commented Jan 5, 2025

Uh oh!

rottifant commented Jan 5, 2025

Uh oh!

nicolas-grekas commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatTheCat commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rottifant commented Jan 7, 2025

Uh oh!

nicolas-grekas commented Jan 17, 2025

Uh oh!

rottifant commented Jan 17, 2025

Uh oh!

florian2peaches commented Mar 26, 2025

Uh oh!

rottifant commented Mar 26, 2025

Uh oh!

florian2peaches commented Mar 26, 2025

Uh oh!

rottifant commented Mar 26, 2025

Uh oh!

florian2peaches commented Apr 9, 2025

Uh oh!

rottifant commented Apr 9, 2025

Uh oh!

Uh oh!

rottifant commented Jan 3, 2025 •

edited

Loading

rottifant Jan 5, 2025 •

edited

Loading

rottifant Jan 5, 2025 •

edited

Loading

nicolas-grekas commented Jan 5, 2025 •

edited

Loading

MatTheCat commented Jan 7, 2025 •

edited

Loading