picqer / exact-php-client

PHP Client library for Exact Online
MIT License
165 stars 201 forks source link

Provide a way to let exceptions bubble through to application #627

Closed vixducis closed 3 months ago

vixducis commented 11 months ago

For a specific issue I'm having it would be immensely helpful to have custom exception bubble through to the application instead of having this library catch it and rethrow it's own exception.

Let me explain in a bit more more detail: you can easily add middleware to the guzzle client the Connection class uses internally with Connection::insertMiddleWare. When you throw a custom exception inside that middleware, the Connection::parseExceptionForErrorMessages will catch that Exception and throw a ApiException, so my own custom exception never makes it back to my application. Ideally, parseExceptionForErrorMessages would only catch GuzzleException or any other exceptions that this library itself defines.

My proposal would be to add the following to the start of Connection::parseExceptionForErrorMessages:

if (! $e instanceof GuzzleException) {
    throw $e;
}

Another solution might be to implement a specific interface for exceptions that are allowed to bubble through so you can add this to Connection::parseExceptionForErrorMessages:

if ($e instanceof BubblesThroughExceptionInterface) {
    throw $e;
}
DannyvdSluijs commented 11 months ago

Hi @vixducis

I can see your use case and I think you're having a valid case. But I quickly checked the code and I can see the original exception in Connection:: parseExceptionForErrorMessages is being passed along as the previous parameter.

This means you can write something like:

try {
    $connection->.....();
} catch (ApiException $e) {
    $previous = $e->getPrevious();
    // Do something with previous exception which can be your exception
}

Or in case you have an ExceptionHandler you can unbox the ApiException there and see if you can process based on the previous exception.

Also note I'm not the core maintainer jst someone that is actively involved in this repo so it might be that others might point to other bits in the code that can help you out.

remkobrenters commented 11 months ago

@rubenwebparking can you chip in with your experience handling errors in the package? Do not recall we ever felt the need to extend the package for this purpose.

vixducis commented 11 months ago

@DannyvdSluijs I hadn't noticed the original exception wasn't being passed through, so that kind of gives me a way to handle this indeed. But it still requires me to handle all ApiExceptions, unwrap them, handle the exception if it is the one I'm looking for and rethrow for all other cases.

It is quite a unique scenario because of the middleware (which I would guess not a lot of people are using), so I'd understand this would be a no-go, but from a conceptual point of view, it kind of makes sense to only handle the exceptions your package is concerned with. Looking forward to other points of view.

rubenwebparking commented 11 months ago

@remkobrenters I agree that the Exception handling in the Connection class can be improved. Ideally, we only catch GuzzleExceptions, so that all other Exceptions (including TypeError for example) will not be wrapped in an ApiException class. However, this change can have a negative impact in existing implementations.

A list of interfaces or classes which are not processed by the error parser may be the way to go.

private function parseExceptionForErrorMessages(Exception $e): void
{
    if (\in_array(get_class($e), $this->throwExceptions, true)) {
        throw $e;
    }
}
remkobrenters commented 11 months ago

Yeah I understand. Tricky one backwards compatibility wise and doubt if the benefits of changing this will outweigh the compatibility impact.

vixducis commented 11 months ago

I understand, is this something that can be implemented on the next major release when breaking changes are to be expected? I can work around it for now, but it would be nice to have something to look forward to.

remkobrenters commented 3 months ago

I am closing this issue for now as it has been stalled for a while.