fbett / le-acme2-php

LetsEncrypt client library for ACME v2 written in PHP.
MIT License
30 stars 15 forks source link

Throw exceptions #2

Closed hayleyxyz closed 6 years ago

hayleyxyz commented 6 years ago

Currently errors are just dumped to standard output

Ideally these should be thrown using typed exceptions so they can be caught, handled and properly rendered

fbett commented 6 years ago

Thanks for your feedback.

Isn't it on the implementor to decide how to handle the thrown exceptions? I though, it would be very easy to implement a try/catch block while calling the methods like in the http example.

Remember, all the current exceptions are thrown on hard problems:

I would suggest everyone to try it again from beginning, if an exception is thrown because of connection problems.

At what kind of situation did you thought?

Of course, all PRs are welcome.

hayleyxyz commented 6 years ago

I guess this is just a difference in coding styles, but personally I don't find plain RuntimeExceptions enough to build a rock-solid application. Uniquley typed exceptions lets the implementor handle each failure case differently, if needed

For example: https://github.com/fbett/le-acme2-php/blob/master/src/LE_ACME2/Response/AbstractResponse.php#L52 I would expect this to throw a RateLimitReachedException, then as an implementor I know to sleep my application for a period of time

Furthermore, functions that can commonly fail should have their return value checked, and throw an exception on failure

Example: https://github.com/fbett/le-acme2-php/blob/master/src/LE_ACME2/Connector/Storage.php#L97 file_put_contents() can fail for multiple reasons; permissions, fd exhaustion, no space on device If this threw an IOException, as an implementor I would probably exit out here and report the exception so a human can come in an sort out the problem, since the application can't fix it itself

A less important example is here: https://github.com/fbett/le-acme2-php/blob/master/src/LE_ACME2/Account.php#L49 The function returns null on failure, so I can detect that, however an exception that tells me the explicit reason why would be nice

fbett commented 6 years ago

Ok, so there is a new commit that should satisfy.

About the referenced examples:

  1. RateLimit There was a hope to find a way to handle rate limits within this library. This was not fully implemented as described in README.md. But is see, that the way to throw a RateLimitReachedException is currently a good idea.

  2. Null response of Account::create and others A situation, i was also not happy with. An InvalidResponseException will be thrown now.

  3. PHP functions like file_put_contents I'm not a fan of to handle system errors, if it is not necessary. In the case of file_put_contents, PHP will raise a message, if a problem occurs. These php messages could be handled by an exception handler. Additionally in case of your referenced example, nothing is lost: If file_put_contents fails only the cache file is not written. So i've not added here some error handling.