Skip to content

[HttpClient] Allow enabling buffering conditionally with a Closure #32565

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 9, 2019
Merged

[HttpClient] Allow enabling buffering conditionally with a Closure #32565

merged 1 commit into from
Sep 9, 2019

Conversation

rjwebdev
Copy link
Contributor

@rjwebdev rjwebdev commented Jul 16, 2019 •
edited by nicolas-grekas
Loading

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31883
License MIT
Doc PR symfony/symfony-docs#12043

With this PR, responses can be buffered automatically from a closure passed to the buffer option.

$resp = $client->request('GET', $url, [
    'buffer' => function (array $headers): bool { return true/false; },
]);

When no option is provided, buffering is now enabled only for json, xml and text/* content types.

@rjwebdev rjwebdev changed the base branch from 4.4 to master July 16, 2019 19:17
@nicolas-grekas nicolas-grekas added this to the next milestone Jul 17, 2019
nicolas-grekas
nicolas-grekas reviewed Jul 17, 2019
View reviewed changes
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.

Thanks for working on this :)

@rjwebdev rjwebdev force-pushed the http-client-auto-buffer branch from c915a31 to f4ddd10 Compare July 18, 2019 19:53
@rjwebdev
Copy link
Contributor Author

@nicolas-grekas

Could you please give me some tips to make sure the tests pass? I'm not sure how to fix them

nicolas-grekas
nicolas-grekas reviewed Jul 18, 2019
View reviewed changes
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.

Thanks :)
We also need to validate the regex and fail properly when it is not valid, can you add a test case ensuring so?

@rjwebdev rjwebdev force-pushed the http-client-auto-buffer branch from ec3e7bb to 0452bf6 Compare July 20, 2019 16:40
nicolas-grekas
nicolas-grekas reviewed Jul 23, 2019
View reviewed changes
@rjwebdev rjwebdev force-pushed the http-client-auto-buffer branch from e2500ff to f1e77b1 Compare July 23, 2019 19:11
nicolas-grekas
nicolas-grekas reviewed Jul 24, 2019
View reviewed changes
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.

(no need for the return-type annotation on createBufferForContentType IMHO)

@rjwebdev rjwebdev force-pushed the http-client-auto-buffer branch from 66c4079 to 917a8d9 Compare July 25, 2019 20:44
@nicolas-grekas
Copy link
Member

Thanks, LGTM for the component.
Note that the PR should target 4.4, can you rebase+retarget?
If not, don't worry we can do it while merging.

Could you please have a look at the configuration for the component in FrameworkBundle?
I think the option "buffer" is defined as boolean there, so we should relax this now, and add a regexp check to ensure it is valid at compile time.

@rjwebdev rjwebdev force-pushed the http-client-auto-buffer branch from 917a8d9 to 7b33c42 Compare July 25, 2019 21:13
@rjwebdev rjwebdev changed the base branch from master to 4.4 July 25, 2019 21:14
@rjwebdev
Copy link
Contributor Author

Right now the default value of the buffer option is true, defined in Symfony\Contracts\HttpClient\HttpClientInterface. There's no buffer option configured in the frameworkbundle yet.

Should I add this option in the configuration (both default_options and scoped_clients?) and validate it in the configuration?

@nicolas-grekas
Copy link
Member

I'd say yes :)

@rjwebdev rjwebdev changed the title Http client auto buffer [HttpClient] Auto buffer Jul 26, 2019
@rjwebdev
Copy link
Contributor Author

I added the buffer configuration option for both the default_options as the scoped_clients.

Also updated the documentation in a PR

@rjwebdev rjwebdev mentioned this pull request Jul 27, 2019
Merged
@nicolas-grekas nicolas-grekas changed the title [HttpClient] Auto buffer [HttpClient] Allow enabling buffering based on the content-type of the response Jul 31, 2019
@rjwebdev rjwebdev force-pushed the http-client-auto-buffer branch from f56174f to 4eabdf4 Compare August 1, 2019 13:19
@nicolas-grekas nicolas-grekas force-pushed the http-client-auto-buffer branch from 96e1076 to 492d0d9 Compare August 8, 2019 18:03
@fabpot
Copy link
Member

fabpot commented Aug 9, 2019

I would not make the regex configurable (as I don't see the use cases but I can see how one can mess up with changing the regex). The buffer option should be true, false, auto IMHO.

@rjwebdev
Copy link
Contributor Author

rjwebdev commented Aug 9, 2019

Ok. So what should happen with this PR?

@fabpot
Copy link
Member

fabpot commented Aug 9, 2019

We are on the right track. The configuration option should only allow true, false, auto (which is going to use you regex).

@rjwebdev
Copy link
Contributor Author

Ok. Any idea which content types we'll allow to be buffered automatically? I need this one to be able to build a regex

@OskarStark
Copy link
Contributor

OskarStark commented Aug 20, 2019 •
edited
Loading

Why not the following solution:

buffer: true
buffer_regex: '/([a-z]+\/json)/'

buffer regex is containing the default one which could be overridden and is used if buffer is true... 🤔 The type mixing (bool/string) is not so the best option IMHO

@nicolas-grekas nicolas-grekas force-pushed the http-client-auto-buffer branch 2 times, most recently from 2c1cc5c to 8486526 Compare September 1, 2019 13:58
nicolas-grekas
nicolas-grekas approved these changes Sep 1, 2019
View reviewed changes
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 updated this PR to take @fabpot's comment into account. The option "buffer" is now removed from FrameworkBundle.
Implementations in the component (not the contracts) support a string for the "buffer" option.
This string should be a regular expression that enables buffering when the content-type of the response matches it.
It defaults to #^(?:text/|application/(?:.+\+)?(?:json|xml)$)#i, which covers all common content-types that should be buffered to me.

@nicolas-grekas nicolas-grekas changed the title [HttpClient] Allow enabling buffering based on the content-type of the response [HttpClient] Allow enabling buffering conditionally with a Closure Sep 1, 2019
@nicolas-grekas nicolas-grekas force-pushed the http-client-auto-buffer branch from 8486526 to db9b15a Compare September 1, 2019 14:48
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 1, 2019 •
edited
Loading

I figured out we could make this much more powerful by accepting a Closure to enable/disable buffering based on response headers. So there we are, PR updated.

@rjwebdev could you please sync the PR description and the documentation PR?

@nicolas-grekas nicolas-grekas force-pushed the http-client-auto-buffer branch from db9b15a to f705ac9 Compare September 1, 2019 14:50
@rjwebdev
Copy link
Contributor Author

rjwebdev commented Sep 7, 2019

@nicolas-grekas

Since you removed the buffer option from the Framework bundle, shouldn't the PR for the docs be closed?

@nicolas-grekas
Copy link
Member

We'd need a new PR, to explain the option, in the doc for the component: components/http_client.rst

@nicolas-grekas
Copy link
Member

Thank you @rjwebdev.

nicolas-grekas added a commit that referenced this pull request Sep 9, 2019
@nicolas-grekas
feature #32565 [HttpClient] Allow enabling buffering conditionally wi…
e9f524a 
…th a Closure (rjwebdev)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] Allow enabling buffering conditionally with a Closure

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31883
| License       | MIT
| Doc PR        | symfony/symfony-docs#12043

With this PR, responses can be buffered automatically from a closure passed to the `buffer` option.

```php
$resp = $client->request('GET', $url, [
    'buffer' => function (array $headers): bool { return true/false; },
]);
```

When no option is provided, buffering is now enabled only for json, xml and text/* content types.

Commits
-------

f705ac9 [HttpClient] Allow enabling buffering conditionally with a Closure
@nicolas-grekas nicolas-grekas merged commit f705ac9 into symfony:4.4 Sep 9, 2019
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Sep 18, 2019
@javiereguiluz
minor symfony#12043 [HttpClient] Add buffer configuration option to h…
57c0337 
…ttp_client (rjwebdev)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpClient] Add buffer configuration option to http_client

In PR symfony/symfony#32565, a regex value was added as a check to automatically buffer the content of a call. Since this is a new feature, the documentation of the http_client configuration true frameworkbundle is updated
<!--

If your pull request fixes a BUG, use the oldest maintained branch that contains
the bug (see https://symfony.com/roadmap for the list of maintained branches).

If your pull request documents a NEW FEATURE, use the same Symfony branch where
the feature was introduced (and `master` for features of unreleased versions).

-->

Commits
-------

0f65736 Add buffer configuration option to http_client
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Merged
Merged
@rjwebdev rjwebdev deleted the http-client-auto-buffer branch May 9, 2020 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Feature Ready Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
6 participants
@rjwebdev @nicolas-grekas @joelwurtz @fabpot @OskarStark @carsonbot