Feature/throw exception when response status code not 200

[4c0n]

Currently if the httplug adapter is not configured to throw exceptions on non 200-ok statuses, there is no way to check if a request to the slack api was successfull. In my case the calls were silently failing, providing no means of finding out what went wrong.

Now when http_errors is configured on the adapter, the behavior is the same as before, but when it is not configured to throw exceptions, this library will throw an exception.

[soullivaneuh]

Hi @4c0n and thank you for your contribution. :+1:

I'm not sure handling HTTP errors goes to the slack library I'm afraid, this is also why httplug provide a plugin to manage it on side project.

Could you please give us some use case with the errors you encounter?

Thanks

[4c0n]

hi @soullivaneuh , thanks for your response!

For example I was using the wrong channel name, using the guzzle 6 adapter, with http_errors: false. The API responded with a 404 response, but there was no way to find out from this package. I had to debug using httplug to find out what was going wrong. Preferably the method either always returns false on failure or it should throw an exception. Since using httplug is basically like a black box to the user (we're using this library not connecting to the api directly using httplug), I feel there should be a way to determine if sending the message was successful or not, without having to configure httplug to throw exceptions on none 200 statuses (but if that is what you really want, then it should be part of the documentation/instructions on how to use the library IMO).

[soullivaneuh]

Note: If merged, it must be released as a new major. Throwing exception is a BC break.

[4c0n]

@soullivaneuh Thanks for the review! I looked up the API errors and implemented exceptions for each of them and added an ErrorResponseHandler class. Please have another look and let me know if there is anything you would like to do differently. I will be testing it shortly.

[4c0n]

@soullivaneuh I tested the changes and made some adjustments. When httplug is not configured to throw exceptions, this library will now throw exceptions when an API error occurs. Potential changes/improvements:

• Throw IOException from ErrorResponseHandler for errors unrelated to the API
• Catch exceptions from httplug and process them as well to see if there was an API error if httplug IS configured to throw exceptions

By implementing the above we can guarantee that the behavior is the same regardless of the exception throwing configuration of httplug. The original exception could be included in the library one. So the library would not be handling http errors, just passing them on, which IMO makes sense, as devs should be dealing with errors coming from this lib, not dependencies of this lib.

Please let me know how you feel about this so that we can move forward :tada:

Merci beaucoup!

[4c0n]

@soullivaneuh Friendly ping

[RafaelFerreiraTVD]

I would like to see this on next release. I have the same issue as @4c0n. Good job 😃

[4c0n]

@RafaelFerreiraTVD Thanks! @soullivaneuh I updated the PR following your requests, please have another look. :)

[4c0n]

@soullivaneuh I modified the code to instantiate the error handler in the client constructor. Please have a look.

[4c0n]

@soullivaneuh The php7.1 environment in Travis is running a no longer supported version of PHPUnit (6.x): https://phpunit.de/supported-versions.html (did not check the other environments yet).

I never used Travis before and can't find a way to upgrade PHPUnit in the build system itself. Do you know if that is possible? I did read that it "prefers" phpunit versions that are installed through composer over the globally installed version. https://docs.travis-ci.com/user/languages/php/#default-build-script

Please advise.

[soullivaneuh]

If you need a more recent version of PHPUnit, the simpler way would be to require it globally with composer, as the bin path is already setup on this project. If you just need the last stable version, then composer global require phpunit/phpunit:@stable.

The other way would be to simply download the .phar file: https://phpunit.de/getting-started/phpunit-8.html

[4c0n]

@soullivaneuh The builds are fixed. Please have a last look.

[4c0n]

@soullivaneuh Could you please have a look at this? Please keep in mind that this PR has been open for almost 3 weeks now. I would very much like to merge and close this PR as soon as possible.

[soullivaneuh]

Hi @4c0n,

I understand, but please keep in mind it's the open-source world and I also have work to do for my company.

I will look at this at soon as possible.

Regards.

[soullivaneuh]

Also, waiting any release, please note you can use your fork on your project dependencies! :+1:

[RafaelFerreiraTVD]

Hi @soullivaneuh it seems good to merge.

[soullivaneuh]

Thank you for you work and you patience @4c0n! :+1: