Make WordPress Core

Opened 7 years ago

Last modified 6 years ago

#39043 assigned enhancement

HTTP API :: Its Not Possible To Send Data In Body For GET Requests

Reported by: lots0logs's profile lots.0.logs Owned by: rmccue's profile rmccue
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: HTTP API Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

Currently there is no way to send data in the body of GET requests using the HTTP API (it gets sent as query args instead). While it may not be a very common use-case, its a valid one nevertheless. I've stumbled upon this while writing an integration for a 3rd-party API which does not accept data as query args like most APIs typically do.

#37456 is relevant to this issue.

Patch incoming...

Attachments (1)

30943.patch (2.3 KB) - added by lots.0.logs 7 years ago.

Download all attachments as: .zip

Change History (13)

@lots.0.logs
7 years ago

#1 @lots.0.logs
7 years ago

  • Keywords has-patch added

#2 @johnbillion
7 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Thanks for the report, @lots.0.logs.

This is a duplicate of #37364, which is marked as wontfix. Passing body parameters in a GET request is not technically against the HTTP specification but it's certainly not recommended. Most HTTP proxies, for example, will strip the body of GET requests.

There's nothing inherent in the SendinBlue API docs which suggest that the request cannot use a POST request.

#3 @lots.0.logs
7 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Whether or not you or I, or anyone else for that matter, would recommend it matters not. The only thing that does matter is this:

Passing body parameters in a GET request is not technically against the HTTP specification

There is no harm in allowing this to be configurable as an option. I mean seriously, the greatest thing about WordPress is how extensible it is. Your reasoning for dismissing this as a wontfix goes against the very spirit WordPress.

#4 @lots.0.logs
7 years ago

  • Keywords 2nd-opinion added

#5 @dd32
7 years ago

Although technically supported, I have little want to support it. Just because it can be done, doesn't mean we should encourage it to be done.
I'll defer to @rmccue however, who maintains Requests and who also seems to not want to support it (#37364)

#6 @dmsnell
7 years ago

My vote also goes against this proposal. It's still possible to support APIs like this (which are very non-standard) by creating a plugin for it. Opening this up in WordPress however would seem to do more to legitimize this behavior when it's very much at risk of breaking things for people in subtle and hard-to-debug ways (such as the mentioned issue with proxies stripping out this data) and it's very non-standard. Would not most people be very suspicious of an API that uses this means for passing data?

#7 @lots.0.logs
7 years ago

Requests supports this out of the box. We shouldn't make something that is an option in Requests impossible to use without extending the WP_Http class and overriding its request() method. Again, all we have here are a bunch of opinions about disliking HTTP GETrequests being made with a data payload. If you dislike it, then obviously don't do it. The patch I submitted simply makes it possible to use an option which is already supported in the Requests library. It does not change the default behavior of the WP HTTP API in any way.

Opening this up in WordPress however would seem to do more to legitimize this behavior

This "behavior" is already legitimate and it will be until such time the HTTP spec says otherwise.

when it's very much at risk of breaking things for people in subtle and hard-to-debug ways (such as the mentioned issue with proxies stripping out this data) and it's very non-standard.

Again, nothing is changing with the default behavior of the WP HTTP API. There is no risk of breaking anything. I hate to say it but your remark is nothing more than FUD.

Even though I personally think this should be an option in the $args array that is passed in to the wp_remote_*() functions (as is done in the patch I submitted), I'm not an unreasonable person. A filter on the $options array that is passed to Requests::request() would achieve the same goal. I'd be happy to revise my patch if you guys think a filter is more appropriate.

Last edited 7 years ago by lots.0.logs (previous) (diff)

#8 follow-up: @rmccue
7 years ago

Per RFC 7231, "A payload within a GET request message has no defined semantics". Handling something with undefined semantics is basically impossible. The reason for this is that until the newest RFCs (723x, aka httpbis), the body wasn't allowed at all (RFC 2616, 4.3):

A message-body MUST NOT be included in
a request if the specification of the request method (section 5.1.1)
does not allow sending an entity-body in requests.

Both Requests and WP_HTTP were designed for HTTP 1.1 as published in RFC 2616, not the newer 723x. Since there's no defined semantics for GET with a payload, supporting it is tough: would we build data arrays into a body or query strings? Should we send a content-type header? Content-length? etc. Requests doesn't allow it so much as it doesn't disallow it.

The thing here that I think is more relevant is that you can't control the query parameters separately to the body parameters, and the choice is made for you. Requests specifically includes the data_format option to allow overriding this. This option was originally added for the reverse case (setting $data to build a query), but is also useful here.

The main reason I didn't do this is because the parameter is specifically called body, which makes me think it should never be a query string. In hindsight, I think that was wrong, since body is actually used for both.

tl;dr we should probably support data_format

@dd32 Agree?

#9 in reply to: ↑ 8 @dd32
7 years ago

Replying to rmccue:

tl;dr we should probably support data_format

I'm willing to agree and go with that, if you believe it should be specified by the caller.

I disagree that we should support a body parameter for GET requests though, but I'm willing to allow explicit calls to force it to happen - although I fear it'll cause compatibility issues in the future if we explicitly allow this, and want to move to another library in the future.

#10 @lots.0.logs
7 years ago

I disagree that we should support a body parameter for GET requests though

Just want to clarify that supporting an explicit body parameter (eg. $args['body']) for GET requests is not and was never the purpose of this enhancement request.

Last edited 7 years ago by lots.0.logs (previous) (diff)

#11 @dd32
7 years ago

  • Milestone set to Future Release
  • Owner set to rmccue
  • Status changed from reopened to assigned

#12 @lots.0.logs
6 years ago

Hi @rmccue, what's the status on this?

Note: See TracTickets for help on using tickets.