Split timeout in connectTimeout/executeTimeout #98
Conversation
There was a problem hiding this comment.
@mlocati I did a first review of your PR. As commented in details, I think we can use the old parameter timeout instead of executetimeout.
The PR needs to be completed with unit test to cover the new features, can you add it?
Moreover, the PR should be send to develop because this is a new feature.
Thanks!
src/Client/Adapter/Curl.php
Outdated
| } | ||
| } | ||
|
|
||
| if (isset($this->config['executetimeout'])) { |
There was a problem hiding this comment.
Instead of add a new parameter executetimeout why we don't use the old one timeout? The meaning is the same and we can be aligned with the curl constant naming.
src/Client/Adapter/Socket.php
Outdated
| } elseif (isset($this->config['timeout'])) { | ||
| $connectTimeout = $this->config['timeout']; | ||
| } else { | ||
| $connectTimeout = @ini_get('default_socket_timeout'); |
There was a problem hiding this comment.
We generally don't use the @ operator. Anyway, in this case the value default_socket_timeout is available since PHP 4.3.0.
src/Client/Adapter/Socket.php
Outdated
| $timedout = $info['timed_out']; | ||
| if ($timedout) { | ||
| $this->close(); | ||
| if (isset($this->config['executetimeout'])) { |
There was a problem hiding this comment.
As commented for the curl adapter, even in this case we should continue to use the timeout parameter instead of adding a new one (executetimeout).
src/Client/Adapter/Socket.php
Outdated
| } elseif (isset($this->config['timeout'])) { | ||
| $executeTimeout = $this->config['timeout']; | ||
| } else { | ||
| $executeTimeout = @ini_get('default_socket_timeout'); |
There was a problem hiding this comment.
Same comment of curl adapter. Don't use the @ operator.
src/Client.php
Outdated
| 'useragent' => 'Zend\Http\Client', | ||
| 'timeout' => 10, | ||
| 'connecttimeout' => null, | ||
| 'executetimeout' => null, |
There was a problem hiding this comment.
We should use the old value timeout instead of adding executetimeout.
But set them to null, to keep backward compatibility
mlocati
force-pushed
the
split-timeout-in-connect-and-execute
branch
from
c4be26a to
6da4e3d
Compare
Done in 6da4e3d
I'm going to do this.
PR updated: now it's against develop |
Done in 0bff41b |
|
@mlocati thanks for your contribution! |
2 participants
See #96