8000 [BrowserKit] HttpBrowser ignores GET-request body · Issue #37609 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[BrowserKit] HttpBrowser ignores GET-request body #37609

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.

Sign up for GitHub

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

Jump to bottom
Closed
Jeroeny opened this issue Jul 20, 2020 · 7 comments · Fixed by #38622
Closed

[BrowserKit] HttpBrowser ignores GET-request body #37609

Jeroeny opened this issue Jul 20, 2020 · 7 comments · Fixed by #38622
Labels
BrowserKit Bug Status: Needs Review

Comments

@Jeroeny
Copy link
Contributor
Jeroeny commented Jul 20, 2020

Symfony version(s) affected: >=4.3

Description
When using the BrowserKit to do functional tests, I've been trying to use the HttpBrowser to perform requests. Our API uses the request body to send data as JSON. The KernelBrowser doesn't seem to mind this and passes on the request to the kernel without modification. However, the HttpBrowser 'extracts' the body and does an early-return when the method is GET. This doesn't seem to comply with the RFC. And makes the use for HttpBrowser not possible for us.

How to reproduce
This is where it's filtered out: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/BrowserKit/HttpBrowser.php#L64

Possible Solution
Remove GET from the in_array check to allow bodies for GET-requests.

If that's agreed upon I'd gladly make a PR.
@Jeroeny Jeroeny added the Bug label Jul 20, 2020
@stof
Copy link
Member
stof commented Jul 23, 2020

Well, BrowserKit is meant to emulate browsers. And AFAIK, no browser is allowing to include a body in a GET request.

@Jeroeny
Copy link
Contributor Author
Jeroeny commented Aug 19, 2020

I guess that makes sense. Is there a standardised way to do functional tests on an API? Or could/should the HttpClient just be used on its own for this?

@nicolas-grekas
Copy link
Member

How does API Platform do? /cc @dunglas

@thiagomp
Copy link
Contributor

Hi @Jeroeny . Interesting to notice that RFC-7231 obsolets RFC-2616.
In the section chapter 4.3.1 of the RFC-7231, I think this is the interesting excerpt for your case:

A payload within a GET request message has no defined semantics;
sending a payload body on a GET request might cause some existing
implementations to reject the request.

Which for me shows that your API implementation can define it's own semantics.
But also, I think what @stof said makes sense because I would speculate that browsers that implemented RFC-2616 followed instructions below and flat-out ignore the body-message:

RFC-2616 section 4.3

...if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request.

RFC-2616 section 9.3

The GET method means retrieve whatever information ([...]) is identified by the Request-URI.

After all that, I'd suggest that, instead of this being a bug, perhaps we could suggest a way to tell BrowserKit to process the message-body on a GET request

@Jeroeny
Copy link
Contributor Author
Jeroeny commented Sep 22, 2020

@thiagomp Thanks for the thorough feedback. Explicitly telling BrowserKit about a message payload sounds good too. In that case I'd probably go with Content-Type. See also RFC7231 - 3.1.1.5:

A sender that generates a me 8000 ssage containing a payload body SHOULD generate a Content-Type header field in that message

@thiagomp thiagomp mentioned this issue Oct 4, 2020
Closed
@thiagomp
Copy link
Contributor
thiagomp commented Oct 4, 2020

@Jeroeny Do you want to take a look in the PR I've created? symfony/browser-kit#11

@thiagomp thiagomp mentioned this issue Oct 18, 2020
Merged
@Jeroeny
Copy link
Contributor Author
Jeroeny commented Oct 19, 2020

@thiagomp Yes something like that would be nice 👍.

nicolas-grekas added a commit that referenced this issue Dec 8, 2020
@nicolas-grekas
feature #38622 [BrowserKit] Allowing body content from GET with a con…
219d511 
…tent-type (thiagomp)

This PR was squashed before being merged into the 5.3-dev branch.

Discussion
----------

[BrowserKit] Allowing body content from GET with a content-type

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #37609
| License       | MIT
| Doc PR        | N/A

This allows a GET request to sent a payload if it specifies a content-type in the HTTP header as per our conversation in the ticket.

Commits
-------

93c2f5e [BrowserKit] Allowing body content from GET with a content-type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BrowserKit Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[BrowserKit] Allowing body content from GET with a content-type thiagomp/symfony
6 participants
@nicolas-grekas @stof @thiagomp @Jeroeny @xabbuh @carsonbot
0