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

Restore BC in all() methods with empty Redmine response #337

Merged
merged 28 commits into from
Dec 28, 2023

Conversation

Art4
Copy link
Collaborator

@Art4 Art4 commented Dec 18, 2023

This PR handles a possible bug of Redmine. If Redmine is called with an unknown query_id parameter, Redmine responses with an 404 status code and an empty response body.

I'm not sure if in this case we should return an empty array or throw an more meaningful exception. Because this case throws a JsonException at the moment, it should not be a breaking change to throw another exception.

Any thoughts?

Refs #335.

@Art4 Art4 added bug pending: await feedback waiting for feedback labels Dec 18, 2023
@Art4 Art4 added this to the v2.4.0 milestone Dec 18, 2023
@Art4 Art4 requested a review from kbsali December 18, 2023 15:37
@Art4 Art4 changed the title Hanlde empty Redmine response Handle empty Redmine response Dec 18, 2023
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a1fd6dd) 95.98% compared to head (5dbecd0) 96.37%.

Additional details and impacted files
@@             Coverage Diff              @@
##               v2.x     #337      +/-   ##
============================================
+ Coverage     95.98%   96.37%   +0.39%     
- Complexity      395      434      +39     
============================================
  Files            27       27              
  Lines          1220     1298      +78     
============================================
+ Hits           1171     1251      +80     
+ Misses           49       47       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Art4
Copy link
Collaborator Author

Art4 commented Dec 19, 2023

I've investigated the issue about handling empty response bodies from Redmine and I've found a BC break in PHP-Redmine-API starting with v2.2.0 in AbstractApi::retrieveAll(). It turns out that this method was able to return array|false|string, but only array|false was documented in the PHPDoc tag.

After rewriting to AbstractApi::retrieveData() it only returns array or throws an exception. (That's the new and preferred way.) I will document this behavior in the new list() and listByX() methods, that are using the new retrieveData() method.

But for the (deprecated) AbstractApi::retrieveAll() and the (deprecated) all() methods I will restore the old behavior, so we should have BC again.

@Art4 Art4 self-assigned this Dec 19, 2023
@Art4 Art4 removed the pending: await feedback waiting for feedback label Dec 20, 2023
@Art4 Art4 changed the title Handle empty Redmine response Restore BC in all() methods with empty Redmine response Dec 22, 2023
@Art4 Art4 marked this pull request as ready for review December 22, 2023 10:13
@Art4 Art4 marked this pull request as draft December 22, 2023 10:14
@Art4 Art4 marked this pull request as ready for review December 22, 2023 10:28
@Art4 Art4 merged commit d810856 into kbsali:v2.x Dec 28, 2023
7 checks passed
@Art4 Art4 deleted the 335-catch-error-on-empty-response-body branch December 28, 2023 08:12
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.

1 participant