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

[HttpClient] Allow arrays as query parameters #31219

Merged
merged 1 commit into from May 21, 2019

Conversation

Projects
None yet
5 participants
@sleepyboy
Copy link

commented Apr 24, 2019

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.

@nicolas-grekas
Copy link
Member

left a comment

I didn't do it because merging is not trivial at all, here are some hints.
Thanks.

@sleepyboy

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

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.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Thanks @sleepyboy
I pushed a 6th commit on your branch to show what I mean. TL;DR: we shouldn't make the logic specific to the way PHP parses URLs. There is nothing standard there. See updated tests.

@nicolas-grekas nicolas-grekas force-pushed the sleepyboy:fix-http-client-query-option branch from 779435a to 891fb1a May 1, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 5, 2019

@sleepyboy if you're OK, we should now squash all commits in one, please advise :)

@sleepyboy

This comment has been minimized.

Copy link
Author

commented May 5, 2019

@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.
Also, should we try to cast objects to string before raising an exception? For instance, if they implement __toString() method?

Here's an example:

class Post
{
    protected $title;

    public function __toString(): string
    {
        return $this->title;
    }
}

$queryArray = ['title' => $post];

Without proper check http_build_query will try to cast objects to array and will omit non-valid query parameters without raising an exception.

class Post
{
    public $title = 'Test';
    public $description = 'Test2';
}
$post = new Post();
$queryString = http_build_query(['post' => $post], '', '&', PHP_QUERY_RFC3986);

will produce "post[title]=Test&post[description]=Test2

We may raise an exception though and leave it up to developers to cast their objects.

@sleepyboy

This comment has been minimized.

Copy link
Author

commented May 5, 2019

@sleepyboy if you're OK, we should now squash all commits in one, please advise :)

If you're okay with the last change and don't need casting objects to string, go ahead and squash the commits.

@fabpot fabpot modified the milestones: 4.3, next May 6, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 11, 2019

I don't think the validation is needed actually. I would prefer following what http_build_query does. All these are PHP idiomatisms anyway.

@sleepyboy

This comment has been minimized.

Copy link
Author

commented May 11, 2019

I don't think the validation is needed actually. I would prefer following what http_build_query does. All these are PHP idiomatisms anyway.

Okay, I'll fix it this weekend then.

@Tobion

This comment has been minimized.

Copy link
Member

commented May 11, 2019

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.
So I would instead provide a query builder where you can configure the behavior and not build it into the http client directly.

@nicolas-grekas nicolas-grekas force-pushed the sleepyboy:fix-http-client-query-option branch from d9351a4 to a19ac0e May 13, 2019

@nicolas-grekas
Copy link
Member

left a comment

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.

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 May 21, 2019

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.3 May 21, 2019

@nicolas-grekas nicolas-grekas force-pushed the sleepyboy:fix-http-client-query-option branch from a19ac0e to 1cbefd7 May 21, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 21, 2019

Thank you @sleepyboy.

@nicolas-grekas nicolas-grekas merged commit 1cbefd7 into symfony:4.3 May 21, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request May 21, 2019

bug #31219 [HttpClient] Allow arrays as query parameters (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
@sleepyboy

This comment has been minimized.

Copy link
Author

commented May 21, 2019

Thank you @sleepyboy.

You're welcome, Nicolas! Thank you for all the comments and suggestions!

@fabpot fabpot referenced this pull request May 22, 2019

Merged

Release v4.3.0-BETA2 #31577

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.