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

[10.x] Add withQueryParameters to the HTTP client #47297

Merged
merged 3 commits into from
Jun 22, 2023

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented May 31, 2023

Some APIs require some query parameters to be always set. For example auth token (even though this isn't a great practice).

With this new helper, we can create a macro with the query parameter always set.

Example:

        Http::macro('convertKit', fn() => Http::timeout(5)
            ->retry(3, 100)
            ->withOptions([
                'query' => [
                    'api_secret' => config('services.convertkit.api_secret'),
                ],
            ])
            ->baseUrl('https://api.convertkit.com/v3/')
            ->acceptJson());

can become:

        Http::macro('convertKit', fn() => Http::timeout(5)
            ->retry(3, 100)
            ->withQueryParameters([
                'api_secret' => config('services.convertkit.api_secret'),
            ])
            ->baseUrl('https://api.convertkit.com/v3/')
            ->acceptJson());

@GrahamCampbell GrahamCampbell changed the title Add withQueryParameters to the HTTP client [10.x] Add withQueryParameters to the HTTP client May 31, 2023
@cosmastech
Copy link
Contributor

Http::convertKit()->get('users', ['search' => 'Matthieu']);

Will this code overwrite the query specified in the macro?

Some APIs require some query parameters to be always set. For example auth token (even though this isn't a great practice).

With this new helper, we can create a macro with the query parameter always set.
@mnapoli
Copy link
Contributor Author

mnapoli commented Jun 1, 2023

@cosmastech good question, only one way to find out. I added a new test to the PR, the query parameters are merged together 👍

@taylorotwell
Copy link
Member

taylorotwell commented Jun 1, 2023

@mnapoli what about some tests to confirm behavior around arrays? For example:

$this->factory->withQuery(
    ['foo' => ['bar', 'baz']],
)->get('https://laravel.com');

And other related scenarios.

Can you mark as ready for review again when that is done? Thanks!

@taylorotwell taylorotwell marked this pull request as draft June 1, 2023 18:36
@driesvints
Copy link
Member

@mnapoli are you still working on this?

@mnapoli
Copy link
Contributor Author

mnapoli commented Jun 21, 2023

@driesvints apologies for the delay, I added more tests to cover these behaviors:

  • we can set query parameters with the new methods
  • on ->get(), we can set additional parameters or override previously set parameters

This is the behavior I would expect and I think makes sense, let me know if you think otherwise.

@mnapoli mnapoli marked this pull request as ready for review June 21, 2023 12:28
@taylorotwell taylorotwell merged commit 4226ef4 into laravel:10.x Jun 22, 2023
16 checks passed
taylorotwell added a commit to illuminate/http that referenced this pull request Jun 22, 2023
* Add `withQueryParameters` to the HTTP client

Some APIs require some query parameters to be always set. For example auth token (even though this isn't a great practice).

With this new helper, we can create a macro with the query parameter always set.

* Add more tests for `withQueryParameters` with arrays

See laravel/framework#47297 (comment)

* formatting

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>
@mnapoli mnapoli deleted the withQueryParameters branch June 22, 2023 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants