Skip to content

Return false from AbstractApi::get() on empty response body #275

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.

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

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

phansys
Copy link
Contributor

@phansys phansys commented May 19, 2021

Fixes #274.

@kbsali
Copy link
Owner

kbsali commented May 22, 2021

the CI is broken, could you try to fix it?

Copy link
Collaborator

@Art4 Art4 left a comment

Choose a reason for hiding this comment

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

This PR will fix the BC break. Thank you 👍

To fix the tests we also have to fix the expected return type in IssueRelation.php, see
https://github.com/kbsali/php-redmine-api/blob/v1.8.0/src/Redmine/Api/IssueRelation.php#L45

-        if (null === $ret) {
+        if (false === $ret) {

Also please see the requested changes.

@@ -42,7 +42,7 @@ public function lastCallFailed()
* @param string $path
* @param bool $decodeIfJson
*
* @return string|array|SimpleXMLElement|null
* @return string|array|SimpleXMLElement|false|null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the |null because this will never be the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json_decode() is able to return null if the decoded string is "null", but I think the API response will never print that.
Ref: https://3v4l.org/8sXS6.

@Art4 Art4 added bug pending: await feedback waiting for feedback labels May 22, 2021
@phansys
Copy link
Contributor Author

phansys commented May 22, 2021

To fix the tests we also have to fix the expected return type in IssueRelation.php, see

I saw that condition, that's in part why I couldn't spend more time in this PR yet.
Given that condition was introduced in version 0.3.0, I don't know if it was always the expected value near the failed responses.
I need to analyze how it was behaving prior to #257 in order to understand how it was working.
Even, since the changes in #257 were published in version 1.7.0, I wonder if we should provide a BC layer with a deprecation path for users who can be relying on this new behavior. Have you some concerns about this?

@Art4
Copy link
Collaborator

Art4 commented May 22, 2021

To be honest I have not deeply dived into the AbstractApi.php. With #257 I moved the parsing functionality from Client::get() to AbstractApi::get() and made the mistake to return null instead of false. I would keep it this old way returning false for BC instead of provide a deprecation path. Rewriting the AbstractApi could be a task after releasing v2.

@phansys phansys requested a review from Art4 May 22, 2021 16:12
@phansys phansys marked this pull request as ready for review May 22, 2021 16:13
Copy link
Contributor Author

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Should I update the CHANGELOG explaining this change or it is updated when releasing?

Copy link
Collaborator

@Art4 Art4 left a comment

Choose a reason for hiding this comment

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

@phansys Thank you very much. 👍
@kbsali This PR is ready for merge. After merge I will release v1.8.1 and port this PR into v2 branch.

@Art4
Copy link
Collaborator

Art4 commented May 22, 2021

Should I update the CHANGELOG explaining this change or it is updated when releasing?

I will do it before releasing v1.8.1. Thank you.

@Art4 Art4 removed the pending: await feedback waiting for feedback label May 22, 2021
@kbsali
Copy link
Owner

kbsali commented May 22, 2021

Thanks @phansys ! 👍

@Art4
Copy link
Collaborator

Art4 commented Jun 1, 2021

@kbsali Can you please merge this? Or can I do it?

@kbsali
Copy link
Owner

kbsali commented Jun 1, 2021

sure, go ahead! :)

@Art4 Art4 merged commit 6811236 into kbsali:v1.x Jun 1, 2021
This was referenced Jun 1, 2021
Art4 added a commit that referenced this pull request Jun 1, 2021
@phansys phansys deleted the issue_274 branch June 5, 2021 22:49
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.

3 participants