Skip to content

Conversation

@alopex06
Copy link

@alopex06 alopex06 commented Jun 8, 2016

setWatchers Method pass a String as Data for the api call and not an array. This produces an warning that crash some applications.

PHP Error:

PHP Catchable fatal error: Argument 3 passed to chobie\Jira\Api::api() must be an array, string given

setWatchers Method use String as Data
PHP Catchable fatal error:  Argument 3 passed to chobie\Jira\Api::api() must be an array, string given
@aik099
Copy link
Member

aik099 commented Jun 8, 2016

Interesting. More issues related to this, that I've found:

  1. set setWatchers method is the only one, that does so
  2. the ClientInterface::sendRequest method (in all implementations) that is called from Api::api when we're doing GET request is expecting $data to be array (passes it into http_build_query), but don't care in all other request methods (e.g. POST)

@jpastoor
Copy link
Collaborator

jpastoor commented Jun 8, 2016

https://docs.atlassian.com/jira/REST/latest/#api/2/issue-addWatcher
seems indeed valid to add as string, so I don't think we can enforce data to be an array. (regression in 49615cf)

@aik099
Copy link
Member

aik099 commented Jun 8, 2016

The change (when typehint was added) was a result of wrong DocBlock comment on the sendRequest and api methods. To avoid such mistake in future I'm proposing:

  1. update data type (+ reformat DocBlock) for Api::api to array|string
  2. update data type (+ reformat DocBlock) for all sendRequest methods (2 implementation + interface) to array|string
  3. in sendRequest implementations throw an InvalidArgumentException when GET request is made, but $data isn't an array

@jpastoor
Copy link
Collaborator

jpastoor commented Jun 8, 2016

Agree with aik099 points and would very much like to see that in this PR.

Some additional wishes (but if you aint sure how to do it I'll add it soon in a separate PR)
4. Unittest to validate 3.
5. Unittest to validate setWatcher propagates correctly to the sendRequest interface

@jpastoor jpastoor changed the title Fix warning Removed incorrect typehint of $data in Api::api() which caused setWatchers to throw a warning Jun 8, 2016
throw InvalidArgumentException
src/Jira/Api.php Outdated
* @param string $method Request method.
* @param string $url URL.
* @param array $data Data.
* @param array|string $data Data.
Copy link
Member

Choose a reason for hiding this comment

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

Please reformat DocBlock to alight parameter names and their descriptions.

$url .= '?' . http_build_query($data);

if ( !is_array($data) ) {
throw new \InvalidArgumentException('Data must be an array.');
Copy link
Member

Choose a reason for hiding this comment

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

Please mention in DocBlock via @throws tag, that this exception is being thrown.

@aik099
Copy link
Member

aik099 commented Jun 8, 2016

Also PHPClient and ClientInterface where not changed.

@alopex06
Copy link
Author

alopex06 commented Jun 8, 2016

Ok thank you for the quick responses.
@jpastoor that would be nice if you could update the unittests later.

@aik099
Copy link
Member

aik099 commented Jun 8, 2016

@jpastoor that would be nice if you could update the unittests later.

Not used to writing unit tests?

@jpastoor jpastoor merged commit 66fa66b into console-helpers:master Jun 8, 2016
@jpastoor
Copy link
Collaborator

jpastoor commented Jun 8, 2016

Merged, thanks

jpastoor added a commit to jpastoor/jira-api-restclient that referenced this pull request Jun 8, 2016
if ( $method == 'GET' ) {
$url .= '?' . http_build_query($data);

if ( !is_array($data) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be moved up. Otherwise you already get notice about wrong parameter usage in http_build_query call.

Copy link
Member

Choose a reason for hiding this comment

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

And same part in PHPClient is still missing.

@aik099
Copy link
Member

aik099 commented Jun 9, 2016

@jpastoor , this PR is incomplete. It not only misses tests, but also misses points we've mentioned as needed.

Therefore until you check that all requested items were done don't merge it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants