Conversation
nunomaduro
left a comment
nunomaduro
left a comment
There was a problem hiding this comment.
Thanks for this pull request, good job.
Let's make sure we address my feedback before merging.
src/Config/SearchConfig.php
Outdated
| 'defaultHeaders' => array(), | ||
| 'defaultForwardToReplicas' => null, | ||
| 'batchSize' => 1000, | ||
| 'gzipEnabled' => true, |
There was a problem hiding this comment.
This is a good point, @chloelbn you may need to refactor the pull request in order to prepare the PHP client for other compression types.
There was a problem hiding this comment.
Don't hesitate on syncing with me for tackling this refactorization.
There was a problem hiding this comment.
done in the last commit, what are your thoughts?
src/Http/Psr7/Request.php
Outdated
| } | ||
|
|
||
| if ($this->hasHeader('Content-Encoding')) { | ||
| $body = gzencode($body, 9); |
There was a problem hiding this comment.
Let's double check the level here, we are not sure if 9 is the correct level.
There was a problem hiding this comment.
Let's unit test this to make sure we have this header in both requests, maybe is already the case.
There was a problem hiding this comment.
tests added, as for compression 9 is the higher level but I guess it will take more time. Not sure how to benchmark which level to use
src/RetryStrategy/ApiWrapper.php
Outdated
| private function request($method, $path, RequestOptions $requestOptions, $hosts, $timeout, $data = array()) | ||
| { | ||
| if ($this->canEnableGzipCompress($method)) { | ||
| $requestOptions->addHeader('Content-Encoding', 'gzip'); |
There was a problem hiding this comment.
Is the underlying HTTP library computing the content-length header as well?
There was a problem hiding this comment.
@chloelbn Seems like tests are still failing, is the underlying http library setting this header?
"Content-Type: application/json; charset=utf-8"
There was a problem hiding this comment.
Or, the gzip feature might not be enable on the test servers we are targeting with the PHP library. Are they the same at the CTS one @nunomaduro? I couldn't check on travis as they are ciphered.
|
@chloelbn Let me know when I can review this 👍 |
|
@chloelbn I will check why travis is not passing the tests. |
|
3 participants
Describe your change
What problem is this fixing?