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.

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

Deprecate lastCallFailed. #294

Merged
merged 1 commit into from
Sep 26, 2021
Merged

Conversation

icedream
Copy link
Contributor

@icedream icedream commented Sep 23, 2021

This PR is a result of discussion in #293.

Redmine\Api\AbstractApi::lastCallFailed fails to acknowledge 2xx codes that are not 200 or 201 as success, however changing that method may break old code relying on this behavior.

Redmine introduced a change to successful responses in 4.1.0 that leads to API calls such as issue updates leading to a yet undocumented 204 response, more details on that in the other issue.

We already have an alternative way to check for success, and that is to simply check the status code directly by calling Redmine\Client\Client::getLastResponseStatusCode. This PR will point users towards that method instead.

I am going to check whether this can be clarified in the documentation as well somehow before I mark this as ready to review.

@kbsali kbsali marked this pull request as ready for review September 24, 2021 07:06
@kbsali
Copy link
Owner

kbsali commented Sep 24, 2021

@icedream why didn't you update #293 ?

@icedream
Copy link
Contributor Author

icedream commented Sep 24, 2021

@icedream why didn't you update #293 ?

I was expecting GitHub to be able to change the source branch of an existing PR somehow, since the branch's name did not match up with the intended purpose of deprecation. I couldn't do it, so I created a new PR instead.

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.

Could you also please update the CHANGELOG.md? But that's a little something I can do later.

src/Redmine/Api/AbstractApi.php Outdated Show resolved Hide resolved
@Art4 Art4 added this to In progress in Release v2.1.0 via automation Sep 24, 2021
@icedream icedream requested a review from Art4 September 24, 2021 23:50
CHANGELOG.md Outdated Show resolved Hide resolved
Release v2.1.0 automation moved this from In progress to Review in progress Sep 25, 2021
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.

👍 Thank you!

Release v2.1.0 automation moved this from Review in progress to Reviewer approved Sep 26, 2021
@kbsali kbsali merged commit c5a2e37 into kbsali:v2.x Sep 26, 2021
Release v2.1.0 automation moved this from Reviewer approved to Done Sep 26, 2021
@kbsali
Copy link
Owner

kbsali commented Sep 26, 2021

👍 thanks a lot @icedream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants