ash-jc-allen / laravel-exchange-rates

A Laravel wrapper package for interacting with the exchangeratesapi.io API.
MIT License
432 stars 46 forks source link

Fixed exception in SharedDriverLogicHandler::exchangeRate() #128

Closed neochief closed 11 months ago

neochief commented 11 months ago

Hi!

I'm getting "Return value must be of type array|float, string returned" from SharedDriverLogicHandler::exchangeRate()

This is the fix for this problem.

ash-jc-allen commented 11 months ago

Hey @neochief! Thanks for picking up on this and submitting a fix for it πŸ˜„

Please could you let me know which driver you were using and any steps to reproduce the error? I just want to double-check I've not missed this off anywhere else too.

neochief commented 11 months ago

Hi!

Sure, I've used the 'exchange-rates-data-api' driver. I use laravel 9.52. I simply call this two times, and get the error:

ExchangeRate::convert(10, 'EUR', 'USD', \Carbon\Carbon::now())

The issue is related to this problem: https://github.com/laravel/framework/pull/38933

I use redis cache store. I checked that your code indeed tries to save the float to cache, but then retrieves string.

ash-jc-allen commented 11 months ago

Hey @neochief! Sorry for the delay in getting this sorted. I've been super busy over the past week, but I'm catching up on things now πŸ™‚

Thanks for the steps to reproduce this bug. I managed to recreate the issue straight away with them!

The package also supports the functionality for you to get the exchange rates and conversions for multiple currency pairs at once like so:

ExchangeRate::convert(10, 'EUR', ['USD', 'GBP'], \Carbon\Carbon::now());

Running that would return something like:

[
    'USD' => 12.24,
    'EUR' => 11.54661,
]

I think your proposed (float) would break that part of the feature. It seemed to cast the array to 1.00 as a float. So I pushed a quick change to your fix that should hopefully only cast to a float if we're retrieving a single exchange rate. If we're fetching multiple rates, it should leave the array as-is.

If possible, would you be able to have a quick check of this and make sure my proposed change still solves your issue? I'd really appreciate it, but it's totally cool if not πŸ˜„

If everything looks good, I'll get it merged and released ASAP!


P.S. - I'm a huge fan of your work! I use Refactoring Guru all the time and I've referenced it in a few of my blog posts before too! πŸ˜„

neochief commented 11 months ago

Yeah, that makes sense. Sorry, I didn't have the time to debug the whole thing, I thought you cache everything on the currency level, so that you can construct such an array from the currencies you already have in cache. In any case, I tested your solution and can confirm that it works for both single currency and the array.

And by the way, I'm flattered that you know about my project. I'm glad that my work was helpful to you ☺️ Kudos and good luck!

ash-jc-allen commented 11 months ago

Hey @neochief! Apologies for taking so long to get this merged in. This week has been crazy busy so I've not had much time to get stuck in. I'm getting a release built now, so I should have this available for installing tonight πŸ˜„

ash-jc-allen commented 11 months ago

This should be available in v7.0.1 of the package now (https://github.com/ash-jc-allen/laravel-exchange-rates/releases/tag/v7.0.1).

Thanks again for the help on this, I appreciate it! πŸš€