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

[7.x] Http client query string support #31996

Merged
merged 3 commits into from Mar 17, 2020
Merged

Conversation

@irazasyed
Copy link
Contributor

irazasyed commented Mar 17, 2020

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
irazasyed added 2 commits Mar 16, 2020
This gives more control over the URL query params just the way Guzzle is designed and supports.
@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Mar 17, 2020

Could you add some tests please?

@irazasyed

This comment has been minimized.

Copy link
Contributor Author

irazasyed commented Mar 17, 2020

Sure!

@irazasyed

This comment has been minimized.

Copy link
Contributor Author

irazasyed commented Mar 17, 2020

@deleugpn

This comment has been minimized.

Copy link
Contributor

deleugpn commented Mar 17, 2020

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

This comment has been minimized.

Copy link
Contributor Author

irazasyed commented Mar 17, 2020

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

This comment has been minimized.

Copy link
Contributor

deleugpn commented Mar 17, 2020

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

This comment has been minimized.

Copy link
Contributor Author

irazasyed commented Mar 17, 2020

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

This comment has been minimized.

Copy link

devfrey commented Mar 17, 2020

Related to #31918

@taylorotwell taylorotwell merged commit 2c8182c into laravel:7.x Mar 17, 2020
7 checks passed
7 checks passed
P7.2 - Sprefer-lowest
Details
P7.2 - Sprefer-stable
Details
P7.3 - Sprefer-lowest
Details
P7.3 - Sprefer-stable
Details
P7.4 - Sprefer-lowest
Details
P7.4 - Sprefer-stable
Details
continuous-integration/styleci/pr The analysis has passed
Details
@irazasyed irazasyed deleted the irazasyed:http-client-query-string-support branch Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.