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.

Sign up for GitHub

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

Jump to bottom

Api methods now consistently return as associative arrays instead of Result (BC warning) #137

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Api methods now consistently return as associative arrays instead of Result (BC warning) #137

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a33afca
Api methods (except those that return issue lists) now consistently r…
jpastoor Aug 18, 2016
7668f36
Added a series of unit tests
jpastoor Aug 18, 2016
85c17ec
Unit tests for addComment and getWorklogs
jpastoor Aug 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- Minimal supported PHP version changed from 5.2 to 5.3 by [@chobie].
- The `Api::getPriorties` renamed into `Api::getPriorities` by [@josevh].
- 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].
- Added local cache to `Api::getResolutions` by [@jpastoor].
- Renamed `Api::api` parameter $return_as_json to $return_as_array by [@jpastoor].
- Changed default `Api::api` parameter $return_as_array to TRUE by [@jpastoor].
- Api methods (except those that return issue lists) now consistently return as associative arrays instead of Result by [@jpastoor].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please replace instead of Result with instead of Result class instance.


### Removed
...
Expand Down
97 changes: 45 additions & 52 deletions src/Jira/Api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,14 +173,14 @@ protected function clearLocalCaches()
/**
* Get fields definitions.
*
* @return array
* @return array|false
Copy link
Collaborator

@aik099 aik099 Aug 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Is there a test case to confirm, that false is returned at some point?
  2. Maybe we should throw exception in ClientInterface on broken/error responses from JIRA instead?

This applies to other similar places where you've added |false to DocBlock.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Valid points, lets investigate that as part of #135.

Here I just made consistent for what we already have, and was not meant to complete unit test coverage for the methods involved, guess that is work for another time :)

Copy link
Collaborator Author

@jpastoor jpastoor Aug 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

On second thought its a bit dodgy to make a BC change without adding tests as well. I might add some tomorrow that validate against JIRA responses just like I did with getStatuses etc.

Copy link
Collaborator

@aik099 aik099 Aug 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Luckily changed methods aren't 100% of public class interface. Please create some basic tests for them (at least confirm, that they return non-empty array all the time).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to wait some time and merge PR with a tests, then merge PR quickly, but without any tests.

*/
public function getFields()
{
// Fetch fields when the method is called for the first time.
if ( $this->fields === null ) {
$fields = array();
$result = $this->api(self::REQUEST_GET, '/rest/api/2/field', array(), true);
$result = $this->api(self::REQUEST_GET, '/rest/api/2/field');

/* set hash key as custom field id */
foreach ( $result as $field ) {
Expand All @@ -199,7 +199,7 @@ public function getFields()
* @param string $issue_key Issue key should be "YOURPROJ-221".
* @param string $expand Expand.
*
* @return Result|false
* @return array|false
*/
public function getIssue($issue_key, $expand = '')
{
Expand All @@ -212,7 +212,7 @@ public function getIssue($issue_key, $expand = '')
* @param string $issue_key Issue key.
* @param array $params Params.
*
* @return Result|false
* @return array|false
*/
public function editIssue($issue_key, array $params)
{
Expand All @@ -222,7 +222,7 @@ public function editIssue($issue_key, array $params)
/**
* Gets attachments meta information.
*
* @return array
* @return array|false
* @since 2.0.0
*/
public function getAttachmentsMetaInformation()
Expand All @@ -239,13 +239,13 @@ public function getAttachmentsMetaInformation()
*/
public function getAttachment($attachment_id)
{
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/attachment/%s', $attachment_id), array(), true);
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/attachment/%s', $attachment_id));
}

/**
* Returns all projects.
*
* @return Result|false
* @return array|false
*/
public function getProjects()
{
Expand All @@ -255,43 +255,38 @@ public function getProjects()
/**
* Returns one project.
*
* @param string $project_key Project key.
* @param string $project_id_or_key Project ID or key.
*
* @return array|false
*/
public function getProject($project_key)
public function getProject($project_id_or_key)
{
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s', $project_key), array(), true);
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s', $project_id_or_key));
}

/**
* Returns all roles of a project.
*
* @param string $project_key Project key.
* @param string $project_id_or_key Project ID or key.
*
* @return array|false
*/
public function getRoles($project_key)
public function getRoles($project_id_or_key)
{
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/role', $project_key), array(), true);
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/role', $project_id_or_key));
}

/**
* Returns role details.
*
* @param string $project_key Project key.
* @param string $role_id Role ID.
* @param string $project_id_or_key Project ID or key.
* @param string $role_id Role ID.
*
* @return array|false
*/
public function getRoleDetails($project_key, $role_id)
public function getRoleDetails($project_id_or_key, $role_id)
{
return $this->api(
self::REQUEST_GET,
sprintf('/rest/api/2/project/%s/role/%s', $project_key, $role_id),
array(),
true
);
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/role/%s', $project_id_or_key, $role_id));
}

/**
Expand Down Expand Up @@ -350,7 +345,7 @@ public function getCreateMeta(
$data['expand'] = implode(',', $expand);
}

return $this->api(self::REQUEST_GET, '/rest/api/2/issue/createmeta', $data, true);
return $this->api(self::REQUEST_GET, '/rest/api/2/issue/createmeta', $data);
}

/**
Expand All @@ -359,7 +354,7 @@ public function getCreateMeta(
* @param string $issue_key Issue key should be "YOURPROJ-221".
* @param array|string $params Params.
*
* @return Result|false
* @return array|false
*/
public function addComment($issue_key, $params)
{
Expand All @@ -379,7 +374,7 @@ public function addComment($issue_key, $params)
* @param string $issue_key Issue key should be "YOURPROJ-22".
* @param array $params Params.
*
* @return Result|false
* @return array|false
* @since 2.0.0
*/
public function getWorklogs($issue_key, array $params)
Expand All @@ -393,7 +388,7 @@ public function getWorklogs($issue_key, array $params)
* @param string $issue_key Issue key should be "YOURPROJ-22".
* @param array $params Params.
*
* @return Result|false
* @return array|false
*/
public function getTransitions($issue_key, array $params)
{
Expand All @@ -406,7 +401,7 @@ public function getTransitions($issue_key, array $params)
* @param string $issue_key Issue key should be "YOURPROJ-22".
* @param array $params Params.
*
* @return Result|false
* @return array|false
*/
public function transition($issue_key, array $params)
{
Expand All @@ -421,7 +416,7 @@ public function transition($issue_key, array $params)
public function getIssueTypes()
{
$result = array();
$types = $this->api(self::REQUEST_GET, '/rest/api/2/issuetype', array(), true);
$types = $this->api(self::REQUEST_GET, '/rest/api/2/issuetype');

foreach ( $types as $issue_type ) {
$result[] = new IssueType($issue_type);
Expand All @@ -439,7 +434,7 @@ public function getIssueTypes()
*/
public function getVersions($project_key)
{
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/versions', $project_key), array(), true);
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/versions', $project_key));
}

/**
Expand Down Expand Up @@ -473,15 +468,15 @@ public function findVersionByName($project_key, $name)
/**
* Get available priorities.
*
* @return array
* @return array|false
* @since 2.0.0
*/
public function getPriorities()
{
// Fetch priorities when the method is called for the first time.
if ( $this->priorities === null ) {
$priorities = array();
$result = $this->api(self::REQUEST_GET, '/rest/api/2/priority', array(), true);
$result = $this->api(self::REQUEST_GET, '/rest/api/2/priority');

/* set hash key as custom field id */
foreach ( $result as $priority ) {
Expand All @@ -504,7 +499,7 @@ public function getStatuses()
// Fetch statuses when the method is called for the first time.
if ( $this->statuses === null ) {
$statuses = array();
$result = $this->api(self::REQUEST_GET, '/rest/api/2/status', array(), true);
$result = $this->api(self::REQUEST_GET, '/rest/api/2/status');

/* set hash key as custom field id */
foreach ( $result as $status ) {
Expand All @@ -525,7 +520,7 @@ public function getStatuses()
* @param string $issue_type Issue type.
* @param array $options Options.
*
* @return Result|false
* @return array|false
*/
public function createIssue($project_key, $summary, $issue_type, array $options = array())
{
Expand Down Expand Up @@ -564,7 +559,7 @@ public function search($jql, $start_at = 0, $max_results = 20, $fields = '*navig
'startAt' => $start_at,
'maxResults' => $max_results,
'fields' => $fields,
)
), false
Copy link
Collaborator

Choose a reason for hiding this comment

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

CS

each multi-line function call must be on separate line

);

return $result;
Expand All @@ -577,7 +572,7 @@ public function search($jql, $start_at = 0, $max_results = 20, $fields = '*navig
* @param string $version Version.
* @param array $options Options.
*
* @return Result|false
* @return array|false
*/
public function createVersion($project_key, $version, array $options = array())
{
Expand All @@ -603,7 +598,7 @@ public function createVersion($project_key, $version, array $options = array())
* @param integer $version_id Version ID.
* @param array $params Key->Value list to update the version with.
*
* @return false
* @return array|false
* @since 2.0.0
* @link https://docs.atlassian.com/jira/REST/latest/#api/2/version-updateVersion
*/
Expand Down Expand Up @@ -646,7 +641,7 @@ public function releaseVersion($version_id, $release_date = null, array $params
* @param string $filename Filename.
* @param array $options Options.
*
* @return Result|false
* @return array|false
*/
public function createAttachment($issue_key, $filename, array $options = array())
{
Expand All @@ -661,7 +656,7 @@ public function createAttachment($issue_key, $filename, array $options = array()
self::REQUEST_POST,
sprintf('/rest/api/2/issue/%s/attachments', $issue_key),
$options,
false,
true,
true
);
}
Expand Down Expand Up @@ -695,7 +690,7 @@ public function createRemotelink(
$options['application'] = $application;
}

return $this->api(self::REQUEST_POST, sprintf('/rest/api/2/issue/%s/remotelink', $issue_key), $options, true);
return $this->api(self::REQUEST_POST, sprintf('/rest/api/2/issue/%s/remotelink', $issue_key), $options);
}

/**
Expand All @@ -714,7 +709,7 @@ public function api(
$method = self::REQUEST_GET,
$url,
$data = array(),
$return_as_array = false,
$return_as_array = true,
$is_file = false,
$debug = false
) {
Expand Down Expand Up @@ -810,7 +805,7 @@ protected function automapFields(array $issue)
* @param string $issue_key Issue key.
* @param array $watchers Watchers.
*
* @return Result|false
* @return array|false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change array|false into array.

Method always returns an array.

*/
public function setWatchers($issue_key, array $watchers)
{
Expand All @@ -828,20 +823,18 @@ public function setWatchers($issue_key, array $watchers)
*
* @param string $issue_key Issue key.
*
* @return Result|array
* @return array|false
* @TODO: Should have parameters? (e.g comment)
*/
public function closeIssue($issue_key)
{
$result = array();

// Get available transitions.
$tmp_transitions = $this->getTransitions($issue_key, array());
$tmp_transitions_result = $tmp_transitions->getResult();
$transitions = $tmp_transitions_result['transitions'];
$transitions = $this->getTransitions($issue_key, array());

// Look for "Close Issue" transition in issue transitions.
foreach ( $transitions as $v ) {
foreach ( $transitions['transitions'] as $v ) {
// Close issue if required id was found.
if ( $v['name'] == 'Close Issue' ) {
$result = $this->transition(
Expand All @@ -862,39 +855,39 @@ public function closeIssue($issue_key)
*
* @param string $project_key Project key.
*
* @return array
* @return array|false
* @since 2.0.0
*/
public function getProjectComponents($project_key)
{
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/components', $project_key), array(), true);
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/components', $project_key));
}

/**
* Get all issue types with valid status values for a project.
*
* @param string $project_key Project key.
*
* @return array
* @return array|false
* @since 2.0.0
*/
public function getProjectIssueTypes($project_key)
{
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/statuses', $project_key), array(), true);
return $this->api(self::REQUEST_GET, sprintf('/rest/api/2/project/%s/statuses', $project_key));
}

/**
* Returns a list of all resolutions.
*
* @return array
* @return array|false
* @since 2.0.0
*/
public function getResolutions()
{
// Fetch resolutions when the method is called for the first time.
if ( $this->resolutions === null ) {
$resolutions = array();
$result = $this->api(self::REQUEST_GET, '/rest/api/2/resolution', array(), true);
$result = $this->api(self::REQUEST_GET, '/rest/api/2/resolution');

foreach ( $result as $resolution ) {
$resolutions[$resolution['id']] = $resolution;
Expand Down
2 changes: 1 addition & 1 deletion src/Jira/Api/Client/CurlClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,11 @@ public function sendRequest(

$http_code = curl_getinfo($curl, CURLINFO_HTTP_CODE);

// If empty result and status != "204 No Content".
if ( $http_code == 401 ) {
throw new UnauthorizedException('Unauthorized');
}

// If empty result and status != "204 No Content".
if ( $response === '' && !in_array($http_code, array(201, 204)) ) {
throw new Exception('JIRA Rest server returns unexpected result.');
}
Expand Down
Loading