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

Improve exceptions #300

Merged
merged 8 commits into from
Dec 25, 2021
Merged

Improve exceptions #300

merged 8 commits into from
Dec 25, 2021

Conversation

Art4
Copy link
Collaborator

@Art4 Art4 commented Dec 14, 2021

At the moment I have the problem that exceptions thrown by the library cannot be distinguished from other exceptions thrown for other reasons. This makes testing with the library a bit harder.

At the moment we throw only two different exceptions:

  1. \Exception, when something fails during the transfer to Redmine or if an API parameter is invalid or missing
  2. \InvalidArgumentException when a non-existent API is requested from the client.

This PR introduces these new exceptions that extend the previous exceptions:

  1. \Redmine\Exception\ClientException replaces \Exception in CurlClient and Psr18Client.
  2. \Redmine\Exception\InvalidApiNameException replaces \InvalidArgumentException('... is not a valid api. Possible apis are ...').
  3. \Redmine\Exception\MissingParameterException replaces \Exception('Missing mandatory parameters').
  4. \Redmine\Exception\InvalidParameterException replaces \Exception('Possible values for ...').

Additionally these new exceptions implement the interface \Redmine\Exception.

This makes it easier to catch only the exceptions thrown by the Redmine library. Nevertheless, we preserve backward compatibility.

try {
    $client->getApi('issues')->create($data);
} catch (\Redmine\Exception $e) {
    // exceptions from kbsali/redmine-api
} catch (\Throwable $e) {
    // other exceptions
}

What do you think, @kbsali? If you like the idea, I will create new tests for the new exceptions.

Copy link
Owner

@kbsali kbsali left a comment

Choose a reason for hiding this comment

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

I love the idea! 👍

src/Redmine/Exception.php Outdated Show resolved Hide resolved
@Art4
Copy link
Collaborator Author

Art4 commented Dec 14, 2021

I love the idea! 👍

Thank you. I will finalize the PR.

Btw what do you think about replacing travic-ci with Github Actions?

@Art4
Copy link
Collaborator Author

Art4 commented Dec 14, 2021

I've found two more cases where exceptions can be thrown and added two new Exception: MissingParameterException and InvalidParameterException.

Copy link
Owner

@kbsali kbsali left a comment

Choose a reason for hiding this comment

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

👍

@Art4 Art4 marked this pull request as ready for review December 14, 2021 14:00
@Art4
Copy link
Collaborator Author

Art4 commented Dec 15, 2021

This PR is ready for review. Please take a special look at the wording in the docs. 😊

@Art4 Art4 requested a review from kbsali December 21, 2021 00:07
Copy link
Owner

@kbsali kbsali left a comment

Choose a reason for hiding this comment

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

very nice! Thanks for this big PR and sorry for the late review.
Only minor comments, you could decide not to address them! :)

src/Redmine/Api/Group.php Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
src/Redmine/Api/Group.php Show resolved Hide resolved
Co-authored-by: Kevin Saliou <kevin@saliou.name>
@kbsali kbsali merged commit c9b33cd into kbsali:v2.x Dec 25, 2021
@Art4 Art4 deleted the improve-exceptions branch January 5, 2022 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants