feat: allow for http adapter param

[saas786]

• Base client class now accepts Http Adapter object as param.
• Add http adapter interface.

[SinusPi]

This seems like a useful PR, I'd happily write a curl variant for my needs. What's missing for this to be merged? I could think of adding implements HttpAdapterInterface to the HttpAdapter class definition, I guess, but is that what's wrong here?

[saas786]

This seems like a useful PR, I'd happily write a curl variant for my needs. What's missing for this to be merged? I could think of adding implements HttpAdapterInterface to the HttpAdapter class definition, I guess, but is that what's wrong here?

I have been using my fork with this PR.

But in order to be merged into core, core needs to implement error handling using PSR I guess, that way, if I just merge this PR, it should work without issues.

But with current implementation regarding error handling, this can break error handling, you will have to write your own error handlers.

[SinusPi]

How does the proposed change affect error handling, though? Obviously any other HTTP adapter would have to implement their own, but it wouldn't "break" error handling. It just wouldn't improve it, and the adapters would be on a "need to handle your own dirt" regime. Still better than no way to plug an adapter, no?

…

On Fri, 4 Mar 2022 at 14:00, saasfreelancer ***@***.***> wrote: This seems like a useful PR, I'd happily write a curl variant for my needs. What's missing for this to be merged? I could think of adding implements HttpAdapterInterface to the HttpAdapter class definition, I guess, but is that what's wrong here? I have been using my fork with this PR. But in order to be merged into core, core needs to implement error handling using PSR I guess, that way, if I just merge this PR, it should work without issues. But with current implementation regarding error handling, this can break error handling, you will have to write your own error handlers. — Reply to this email directly, view it on GitHub <#636 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3QELOBQUE5WG4CLYGYILTU6ICOPANCNFSM5E5L7P5Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

-- Adam "Sinus" Skawiński

[saas786]

I am not blaming somebody here...

I might be wrong...

But this doesn't work when I use Guzzle Client:

        try{}
        catch (\Recurly\Errors\Validation $e) {}
        catch (\Recurly\Errors\NotFound $e) {}
        catch (\Recurly\RecurlyError $e) {}

but only this work:

        try{}
        catch (Exception $e) {}

Because errors are not being thrown / handled by recurly, but rather by Guzzle now, so they are more like this:

try {} catch (\GuzzleHttp\Exception\RequestException $e) {}

[SinusPi]

(By the way: your PR won't merge cleanly anymore; there have been changes to the Base_Client constructor definition.)

[SinusPi]

After incorporating your HttpAdapterInterface into my variant (a cURL adapter), I see what you meant with the exceptions. Well, yes, you'll probably have to catch the GuzzleHttp\Exception exceptions and throw them back at the "final" code. For simplicity's sake, I just went with a simple ConnectionError class that wraps any cURL error details if the connection fails entirely, but leaves it up to the Recurly lib to throw its exceptions if the connection succeeds but the response isn't valid.