Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[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

Merged
merged 1 commit into from
May 21, 2019

Conversation

sleepyboy
Copy link
Contributor

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.

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.

src/Symfony/Component/HttpClient/HttpClientTrait.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpClient/HttpClientTrait.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpClient/HttpClientTrait.php Outdated Show resolved Hide resolved
@sleepyboy
Copy link
Contributor Author

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
Copy link
Member

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 fix-http-client-query-option branch from 779435a to 891fb1a Compare May 1, 2019 09:00
@nicolas-grekas
Copy link
Member

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

@sleepyboy
Copy link
Contributor Author

sleepyboy 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
Copy link
Contributor Author

@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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

Tobion 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 fix-http-client-query-option branch from d9351a4 to a19ac0e Compare May 13, 2019 11:05
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.3 May 21, 2019 16:51
@nicolas-grekas nicolas-grekas force-pushed the fix-http-client-query-option branch from a19ac0e to 1cbefd7 Compare May 21, 2019 16:52
@nicolas-grekas
Copy link
Member

Thank you @sleepyboy.

@nicolas-grekas nicolas-grekas merged commit 1cbefd7 into symfony:4.3 May 21, 2019
nicolas-grekas added a commit that referenced this pull request May 21, 2019
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
Copy link
Contributor Author

Thank you @sleepyboy.

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

@fabpot fabpot mentioned this pull request May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.