Skip to content

Conversation

@betterphp
Copy link
Contributor

By default when a HTTP code other than 200 is returned file_get_contents does not return the content. Setting this flag means that the data is always returned and error messages from JIRA can be parsed correctly

By default when a HTTP code other than 200 is returned file_get_contents does not return the content. Setting this flag means that the data is always returned and error messages from JIRA can be parsed correctly
@aik099
Copy link
Member

aik099 commented Aug 26, 2016

Please also add tests in https://github.com/chobie/jira-api-restclient/blob/master/tests/Jira/Api/Client/AbstractClientTestCase.php, that would demonstrate that it works.

@betterphp
Copy link
Contributor Author

That throws a PHP error if the ignore_errors flag is not set


public function testErrorResponseValueReturned()
{
$this->traceRequest(Api::REQUEST_GET, array('http_code' => '403', 'response_mode' => 'trace'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't look to explicit. I expect some predetermined output being sent back from REST endpoint and then assertion made on actual output.

@betterphp
Copy link
Contributor Author

Using the existing GET request test?

$this->assertEquals($data, $trace_result['_GET']);
}

public function getRequestWithKnownHttpCode()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename into getRequestWithKnownHttpCodeDataProvider.

@aik099
Copy link
Member

aik099 commented Aug 26, 2016

Using the existing GET request test?

Yes, that looks good.

Also mention this change in CHANGELOG.md, because this one of required steps during PR submission.

{
$data = array('param1' => 'value1', 'param2' => 'value2');
$trace_result = $this->traceRequest(Api::REQUEST_GET, $data);
$trace_result = $this->traceRequest(Api::REQUEST_GET, array_merge(['http_code' => $http_code], $data));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library is PHP 5.3+ compatible and PHP 5.4+ specific syntax (short arrays in this case) should not be used.

@aik099
Copy link
Member

aik099 commented Aug 26, 2016

Today’s review completed.

CHANGELOG.md Outdated
- Remove trailing slash from endpoint url by [@Procta].
- Added local cache to getResolutions by [@jpastoor].
- Renamed Api::api() parameter $return_as_json to $return_as_array by [@jpastoor].
- ignore_errors flag added to HTTP context by [@betterphp].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. this should be added to Fixed section
  2. this a fix to case, when error text not being available as API call result and should be written as Error details from failed API calls were not available back from Api::api method call.
  3. for your nickname to be highlighted on changelog page you also need to add it in mapping at the bottom of this file

@betterphp
Copy link
Contributor Author

Is that not when this is 14950cf ?

@aik099
Copy link
Member

aik099 commented Aug 26, 2016

My bad. You're sending fixes at light speed and I've totally missed that one.

Yes, you're correct.

@aik099
Copy link
Member

aik099 commented Aug 26, 2016

Also it seems, that e-mail you've used to author commits in this PR isn't linked to your account. Because of this:

  1. the author name/icon on commits is incorrect
  2. once merged PR won't be counted towards your contribution count on your public profile page

@betterphp
Copy link
Contributor Author

betterphp commented Aug 26, 2016

I've added the email address to my account which seemed easier than editing history. It was wrong because I'm doing this at work and we use a different git system

@aik099 aik099 merged commit 399401c into console-helpers:master Aug 26, 2016
@aik099
Copy link
Member

aik099 commented Aug 26, 2016

Merging, thanks @betterphp .

@aik099
Copy link
Member

aik099 commented Aug 26, 2016

Next time please send PR from different then master branch. This way if you have 2 PRs in progress they don't share code on GitHub.

@betterphp
Copy link
Contributor Author

Cool cool, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants