-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpClient] Allow arrays as query parameters #31219
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
[HttpClient] Allow arrays as query parameters #31219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do it because merging is not trivial at all, here are some hints.
Thanks.
Nicolas, I tried to address all the issues mentioned in your comments in the last commits. The AppVeyor and Travic CI builds failed but the errors are not HttpClient related though. |
Thanks @sleepyboy |
779435a
to
891fb1a
Compare
@sleepyboy if you're OK, we should now squash all commits in one, please advise :) |
I added missing query array validation — we lost it along the way. Here's an example:
Without proper check http_build_query will try to cast objects to array and will omit non-valid query parameters without raising an exception.
will produce "post[title]=Test&post[description]=Test2 We may raise an exception though and leave it up to developers to cast their objects. |
If you're okay with the last change and don't need casting objects to string, go ahead and squash the commits. |
I don't think the validation is needed actually. I would prefer following what |
Okay, I'll fix it this weekend then. |
I cannot recommend this approach. There are alot of different ways to serialize list of values in a query, e.g. comma separated or repeating the query param. |
d9351a4
to
a19ac0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this PR, for 4.3 would be great. I understand what you mean @Tobion but on the other side, the "query" option already exists and is the one that is the most useful since it's the only one that is standard.
Any other type of query string is easily implemented outside of the client itself.
a19ac0e
to
1cbefd7
Compare
Thank you @sleepyboy. |
This PR was submitted for the master branch but it was merged into the 4.3 branch instead (closes #31219). Discussion ---------- [HttpClient] Allow arrays as query parameters | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A This PR allows passing arrays as query parameters. For instance, if I pass $options['query']['category'] = ['news', 'sport', 'culture'] to my GET request, I expect this to be transformed to URL?category[]=news&category[]=sport&category[]=culture. I added two tests to cover this functionality. I also fixed one of the tests where "+" wasn't encoded as %20 in the final URL. Commits ------- 1cbefd7 [HttpClient] Allow arrays as query parameters
You're welcome, Nicolas! Thank you for all the comments and suggestions! |
This PR allows passing arrays as query parameters.
For instance, if I pass $options['query']['category'] = ['news', 'sport', 'culture'] to my GET request, I expect this to be transformed to URL?category[]=news&category[]=sport&category[]=culture.
I added two tests to cover this functionality. I also fixed one of the tests where "+" wasn't encoded as %20 in the final URL.