[7.x] Http client query string support

[irazasyed]

This is the PR for the issue reported earlier here: laravel/ideas#2137

This doesn't break any existing usage and should work just fine since Guzzle itself supports query params in URI and there is no need to parse and send them as an array like it was being done previously.

You can refer Guzzle docs for query request option.

There is one point to note here, tho.

Guzzle's default behavior is that, if we pass the query request option with either a string or array (empty or with values), it will overwrite "all query string values supplied in the URI of a request"

So as per Guzzle (And I tested this case), this is what happens:

// Send a GET request to /get?foo=bar
Http::get('https://example.com/get?abc=123', ['foo' => 'bar']);

// Guzzle will automatically replace and turn the above URI to:
// https://example.com/get?foo=bar

While it's Guzzle that's doing this, I'm just putting this out here to highlight its behavior so it doesn't cause a confusion.

Examples

This PR will support the following variations of a GET request.

Http::get('https://example.com/get');
// URL: https://example.com/get

Http::get('https://example.com/get?abc=123');
// URL: https://example.com/get?abc=123

Http::get('https://example.com/get', ['foo' => 'bar']);
// URL: https://example.com/get?foo=bar

Http::get('https://example.com/get', 'foo=bar');
// URL: https://example.com/get?foo=bar

Http::get('https://example.com/get?abc;foo;bar;1;10;2&p=2');
// URL: https://example.com/get?abc;foo;bar;1;10;2&p=2

Http::get('https://example.com/get', 'abc;foo;bar;1;10;2&p=2');
// URL: https://example.com/get?abc;foo;bar;1;10;2&p=2

Http::get('https://example.com/get', ['abc;foo;1;10;2' => 'bar', 'p' => 2]);
// URL: https://example.com/get?abc%3Bfoo%3B1%3B10%3B2=bar&p=2

// Notice how Guzzle handles the encoding part if the query is an array, and it has these chars that need encoding, otherwise it just passes as is giving us more control.

This also resolves two old issues reported in Zttp (Similar to mine, the same encoding issue).

// Issue: https://github.com/kitetail/zttp/issues/77
Http::get('https://example.com/get?api.version=1');
// URL: https://example.com/get?api.version=1

// Issue: https://github.com/kitetail/zttp/issues/42
Http::get('http://localhost:8002/monitoring/stats?mount=/test.mp3');
// URL: http://localhost:8002/monitoring/stats?mount=/test.mp3

[GrahamCampbell]

Could you add some tests please?

[irazasyed]

Sure!

[irazasyed]

Added @GrahamCampbell

[deleugpn]

Feels like Guzzle breaks the principle of Least Astonishment and by exposing that here, Laravel gets the downside without the responsibility. I'm sure there's reasons for it, but it just feels wrong to have a request sent to ('foo.bar?some=var', ['var' => 1]) and have it remove the parameters from the query string. If there's query strings in the URL, doesn't that mean it takes precedence as it's already prepared and established?

[irazasyed]

I don't think Laravel is responsible for how Guzzle tackles these things since this is merely a UX/DX layer.

Also, why would the former take precedence over the latter? We all know how the variable assignment works. It's always the latter that takes precedence (example).

And generally, any dev that's working on this would most likely either pass a params array or an already prepared URL with no additions required. That case is extremely rare and not sure why anyone would even want to do that? Like there is really no point of passing a prepared URL + Query params both in parts.

That's how it has always been for years with Guzzle and if there's anyone that needs to fix (Although, I still don't see what's there to fix), then it has to be done at the core level, i.e. Guzzle.

And anyway, they've made it this way to give more control to us, so we can either send a prepared URL or let Guzzle prepare one for us using the params we give to it.

The point of highlighting how the method works is so devs don't get confused.

[deleugpn]

I don't think Laravel is responsible for how Guzzle tackles these things since this is merely a UX/DX layer.

Precisely. This subject is much more about DX than anything else.

Also, why would the former take precedence over the latter? We all know how the variable assignment works. It's always the latter that takes precedence (example).

I would agree about variable assignment, but this isn't it. First, you're not just overriding previous variables with new values, but rather erasing all previous variables and setting up new ones. Secondly, the query parameters in the URL has been parsed and prepared. Undoing that work is not comparable with merging variables and overriding it.

And generally, any dev that's working on this would most likely either pass a params array or an already prepared URL with no additions required. That case is extremely rare and not sure why anyone would even want to do that? Like there is really no point of passing a prepared URL + Query params both in parts.

I can agree with this, but I can't agree with a behavior that decides to erase work that has been done. Regardless of the reason, if an endpoint is defined as https://foo.bar?source=my-domain, I would never expect any library to modify and erase what I defined as my endpoint.

That's how it has always been for years with Guzzle and if there's anyone that needs to fix (Although, I still don't see what's there to fix), then it has to be done at the core level, i.e. Guzzle.

Using the past as an argument to defend behavior isn't a strong argument. Women didn't have the right to vote for decades, doesn't mean it was the right thing. I can imagine it could be too big of a breaking change for Guzzle to fix and not worth it.

And anyway, they've made it this way to give more control to us, so we can either send a prepared URL or let Guzzle prepare one for us using the params we give to it.

I think it's the opposite. They took control away by forcing one way or another onto users, specially by deleting query parameters already present if an array is sent.

[irazasyed]

How about you come up with a smarter way to handle all the cases I've mentioned along with a solution to an issue you're having rather than agreeing/disagreeing?

We can parse and merge both but that'll introduce a whole lot of other issues I foresee and is unnecessary to tackle something that isn't really broken or needed.

[devfrey]

Related to #31918

[nickturrietta]

I would agree about variable assignment, but this isn't it. First, you're not just overriding previous variables with new values, but rather erasing all previous variables and setting up new ones. Secondly, the query parameters in the URL has been parsed and prepared. Undoing that work is not comparable with merging variables and overriding it.

Yeah, this bit me today. Seems really odd behavior to unset already specified query parameters.

This HTTP Client was based on Adam Wathan's ZTTP client. The ZTTP client doesn't override variables already set in the URL string: https://github.com/kitetail/zttp/blob/master/tests/ZttpTest.php (see query_parameters_in_urls_can_be_combined_with_array_parameters method)

[iSWORD]

It would be nice to be able to get query params using a method. 🤔 should I do it? @taylorotwell

[iSWORD]

I realized it can be done with $request->data()['foo'] == 'bar'. My bad.