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 enabling buffering based on the content-type of the response #32565

Open
wants to merge 1 commit into
base: 4.4
from

Conversation

@rjwebdev
Copy link
Contributor

commented Jul 16, 2019

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 by their content types by defining the content types as regexes in the buffer option

@rjwebdev rjwebdev requested review from dunglas, lyrixx, sroze and xabbuh as code owners Jul 16, 2019

@rjwebdev rjwebdev changed the base branch from 4.4 to master Jul 16, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 17, 2019

@nicolas-grekas
Copy link
Member

left a comment

Thanks for working on this :)

@rjwebdev rjwebdev force-pushed the rjwebdev:http-client-auto-buffer branch from c915a31 to f4ddd10 Jul 18, 2019

@rjwebdev

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

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

left a comment

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 rjwebdev:http-client-auto-buffer branch from ec3e7bb to 0452bf6 Jul 20, 2019

@rjwebdev rjwebdev force-pushed the rjwebdev:http-client-auto-buffer branch from e2500ff to f1e77b1 Jul 23, 2019

@nicolas-grekas
Copy link
Member

left a comment

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

@rjwebdev rjwebdev force-pushed the rjwebdev:http-client-auto-buffer branch from 66c4079 to 917a8d9 Jul 25, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

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 rjwebdev:http-client-auto-buffer branch from 917a8d9 to 7b33c42 Jul 25, 2019

@rjwebdev rjwebdev changed the base branch from master to 4.4 Jul 25, 2019

@rjwebdev

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I'd say yes :)

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

@rjwebdev

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

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

Also updated the documentation in a PR

@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 rjwebdev:http-client-auto-buffer branch from f56174f to 4eabdf4 Aug 1, 2019

@nicolas-grekas
Copy link
Member

left a comment

last comments on my side :)
@joelwurtz could you please have a look? WDYT?

@rjwebdev rjwebdev force-pushed the rjwebdev:http-client-auto-buffer branch from 4eabdf4 to 51f40c4 Aug 1, 2019

@joelwurtz

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Regex is nice to cover a lot of use case, I'm just wondering about the DX if this is the best option ?

If i want to create the regex that cover all of the text types it's not an easy one to create but this may be resolve by providing good examples in the documentation ?

(And providing some regex as constant to help for common use cases would even be better, but i think it's too soon to provide this, let's see how it is used before providing this)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

can you please rebase to get rid of the conflict? we removed the ForwardCompatTestTrait recently.

@rjwebdev rjwebdev force-pushed the rjwebdev:http-client-auto-buffer branch from 51f40c4 to 96e1076 Aug 4, 2019

@rjwebdev

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Should be ok now

@nicolas-grekas nicolas-grekas force-pushed the rjwebdev:http-client-auto-buffer branch from 96e1076 to 492d0d9 Aug 8, 2019

@fabpot

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Ok. So what should happen with this PR?

@fabpot

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

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

@@ -10,6 +10,7 @@ CHANGELOG
* added `$response->toStream()` to cast responses to regular PHP streams
* made `Psr18Client` implement relevant PSR-17 factories and have streaming responses
* added `max_duration` option
* added support for regexp for the buffer option

This comment has been minimized.

Copy link
@OskarStark

OskarStark Aug 20, 2019

Contributor
Suggested change
* added support for regexp for the buffer option
* added support for regexp for the `buffer` option
@@ -534,6 +535,7 @@
<xsd:attribute name="local-pk" type="xsd:string" />
<xsd:attribute name="passphrase" type="xsd:string" />
<xsd:attribute name="ciphers" type="xsd:string" />
<xsd:attribute name="buffer" type="xsd:string" />

This comment has been minimized.

Copy link
@OskarStark

OskarStark Aug 20, 2019

Contributor

is string correct here or is it because of the true/false or regex option?

@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

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

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