Skip to content
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

[RFC] $request->body->get #47646

Closed
ro0NL opened this issue Sep 21, 2022 · 14 comments · Fixed by #49614
Closed

[RFC] $request->body->get #47646

ro0NL opened this issue Sep 21, 2022 · 14 comments · Fixed by #49614
Assignees
Labels
HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@ro0NL
Copy link
Contributor

ro0NL commented Sep 21, 2022

Description

Hi,

I'd like to propose $request->body->get() (an InputBag) to provide uniform access to a structured request body payload, regardless of content-type.

$request->request always looked weird to me, and is bound to $_POST, thus application/x-form-urlencoded:

public static function createFromGlobals(): static

Then for application/json one needs toArray():

public function toArray(): array

Example

$request->body = new InputBag($_POST ?: $request->toArray());
@carsonbot carsonbot added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Sep 21, 2022
@ro0NL
Copy link
Contributor Author

ro0NL commented Sep 21, 2022

this is not about adding support for XML, YAML, etc. though i'd fundamentally agree it should be possible to add later

ideally the parsing happens in a lazy manner, thus a LazyInputBag or so

@rvanlaak
Copy link
Contributor

I also remember this being quite confusing when I started working with Symfony back then, especially as the variable itself most often is named $request already.

@ro0NL ro0NL closed this as completed Oct 19, 2022
@kbond kbond self-assigned this Oct 19, 2022
@kbond kbond reopened this Oct 19, 2022
@kbond
Copy link
Member

kbond commented Oct 20, 2022

@symfony/mergers any thoughts on this. I really like the idea. I think it's common to want to get the array of data from a request and not care if it's a form request or json.

My only hesitation is the name body - it doesn't necessarily imply this will be a structured array.

One idea @ro0NL had is to perhaps deprecate Request::getContent() in favour of Request::getRawBody(). I've often looked for a Request::getBody() to get the raw body before finding it's actually Request::getContent(). Additionally, maybe Request::$request could be deprecated in favour of Request::$form?

@welcoMattic
Copy link
Member

I would love to clarify the $request->request thing, it was confusing to me when I started to code with Symfony too 👍

@chalasr
Copy link
Member

chalasr commented Oct 20, 2022

I agree as well that it’s worth renaming.

@OskarStark
Copy link
Contributor

I agree as well that it’s worth renaming.

Me too 👍

@maxbeckers
Copy link
Contributor

maxbeckers commented Oct 20, 2022

I like the idea of renaming $request->request. But why form? This sounds to me as well confusing, because its maybe formdata, but not the form. I'd prefer to name it body or maybe data, because it's the posted data of the request.

@kbond
Copy link
Member

kbond commented Oct 20, 2022

This issue has sort of morphed into three things.

  1. A new property ($body) that represents either the $_POST data or json request body (the initial part of this issue).
  2. Renaming the $request property (which is a wrapper for $_POST) to something else ([HttpFoundation] [WIP] rename Request::$request to $post #47938). I chose $form because this is what $_POST represents, the form data. I'm not completely convinced that $form is the best term (it's better than $request IMO) - maybe $post would be better?
  3. Renaming Request::getContent() to getRawBody().

@maxbeckers
Copy link
Contributor

maxbeckers commented Oct 21, 2022

Yes, you are absolutely right. There are exactly these 3 topics here in this issue. Thanks for clarifying.
And yes you are right, from the naming point of view body doesn't really fit well then either. $post(Data) also sounds a bit strange to me, but yeah, maybe it's best to name it that way.
Better than request it is in any case.

@chalasr
Copy link
Member

chalasr commented Oct 21, 2022

I'm not completely convinced that $form is the best term (it's better than $request IMO) - maybe $post would be better?

My preference goes for body, I think it's the the most consistent with PHP and HTTP terms:

@kbond
Copy link
Member

kbond commented Oct 21, 2022

My preference goes for body, I think it's the the most consistent with PHP and HTTP terms:

Now, do you think $body should be new InputBag($_POST ?: $this->toArray()) as initially described by this issue or a straight up rename of $request?

@GromNaN
Copy link
Member

GromNaN commented Oct 22, 2022

Now, do you think $body should be new InputBag($_POST ?: $this->toArray()) as initially described by this issue or a straight up rename of $request?

I would love if $request->body could be populated equally from x-www-form-urlencoded or application/json requests. Adding a new property is the right opportunity to do so.

For backward-compatibility, we cannot do the same for $request->request. Thus, they needs to be distinct objects.

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 3, 2022

i understand if this is too disruptive

also i realized for us the better path is to deserialize requests into dedicated (message) DTOs, and dispatch from there

@ro0NL ro0NL closed this as completed Nov 3, 2022
@stof stof closed this as not planned Won't fix, can't repro, duplicate, stale Nov 3, 2022
@wouterj
Copy link
Member

wouterj commented Nov 3, 2022

I think we should keep this issue open, or at least close as completed.

We have an open PR about this (#47938) that we should continue, we can do this in a non disruptive way.

@wouterj wouterj closed this as completed Nov 3, 2022
fabpot added a commit that referenced this issue Apr 20, 2023
This PR was merged into the 6.3 branch.

Discussion
----------

[HttpFoundation] add `Request::getPayload()`

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #47646
| License       | MIT
| Doc PR        | todo

Alternative for #47938 based on feedback there.

```php
/** `@var` InputBag $input wrapping either Request::$request or Request::toArray() */
$input = $request->getPayload();
```

Commits
-------

4a0a439 [HttpFoundation] add `Request::getPayload()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.