chobie / jira-api-restclient

php JIRA REST API
MIT License
218 stars 123 forks source link

Set timeout for fail gracefully #160

Open ish1301 opened 6 years ago

ish1301 commented 6 years ago

Sometime JIRA server is down/moved and API takes forever to respond. So consistent timeout address this issue and fail with message that "JIRA server have timed out after 30000 mili seconds"

aik099 commented 6 years ago

Please do this:

  1. add protected $timeout; property to CurlClient class
  2. add optional $timeout = null parameter to CurlClient class constructor, that will be assigned to that $timeout class property
  3. if $this->timeout is numeric, then set timeout on curl resource
  4. add tests to confirm, that timeout is respected
  5. add note to CHANGELOG.md file
  6. rebase (once #161 is merged) to avoid failing PHP 5.3 tests

This would avoid BC break with enforced timeout of 30 seconds.

aik099 commented 6 years ago

@ish1301 , I saw new commit, but it does only part of requested changes. I guess more commits are coming. Anyway, when you're ready to for code review just leave a message in here.

ish1301 commented 6 years ago

I can't do 2nd step, because that will force me to change ClientInterface and timeout can't be applied to other classes e.g. PHPClient.php

The only way to change timeout is $this->api->client->timeout = 30;

aik099 commented 6 years ago

I can't do 2nd step, because that will force me to change ClientInterface

I haven't said, that you need to change ClientInterface. I said, that you need to change CurlClient class constructor (which isn't part of the ClientInterface interface) so that you can specify curl-specific timeout. The timeout should be anything present in all client interface implementing parties.

The only way to change timeout is $this->api->client->timeout = 30;

That won't do of course.

ish1301 commented 6 years ago

Adding new parameter sendRequest() function returns below error, because it expects sendRequest() function to be exact structure as it's in ClientInterface

Declaration must be compatible with ClientInterface->sendRequest

ish1301 commented 6 years ago
screen shot 2017-11-13 at 3 45 34 pm
aik099 commented 6 years ago

Adding new parameter sendRequest() function returns below error, because it expects sendRequest() function to be exact structure as it's in ClientInterface

Declaration must be compatible with ClientInterface->sendRequest

Again I don't see where I requested to change that method signature.

Please don't do things that aren't requested.