- Issue created by @Graber
- Issue was unassigned.
- Status changed to Needs review
over 1 year ago 10:36am 14 June 2023 - last update
over 1 year ago 16 pass - 🇵🇱Poland Graber
That's for a quick workaround.
As I said though, those rates shouldn't be stored in config for a comprehensive solution.
- Status changed to Active
over 1 year ago 11:46am 14 June 2023 - 🇭🇷Croatia valic Osijek
Exchange rates are config entities with their benefits and drawbacks.
As is with config entities, in some cases, some features are welcome; in others not so much.You can easily avoid these exports & overwrites on deployment by using the config_ignore → module
and adding this pattern to be ignored completely.
commerce_exchanger.latest_exchange_rates.*
For this purpose, the patterns for configuration files of the entity are different than those of rates themselves, so that can be easily ignored only for rate exports.
Plugins:
commerce_exchanger.commerce_exchange_rates.{machine_name_of_exchange_rate}.yml
Exchange rates:commerce_exchanger.latest_exchange_rates.{machine_name_of_exchange_rate}.yml
Not that I am not open to suggestions. We could store rates outside config files (etc. state table) - but we loose than ability to fix some rates by using config overrides
- 🇵🇱Poland Graber
Thanks, I know. I didn't want to add config_ignore or config_split for this single purpose only as that'd seem an overkill but I think eventually one of them will land on the project anyway probably..
In general rates change every day.. you mean daily release tags just to update currency rates that'll be usually fetched from some APIs (I'm using fixer.io)? Doesn't sound right.
but we loose than ability to fix some rates by using config overrides, or in some cases to deploy rates with drush cim (there are those cases as well)
- It'd need even more work then to make this right - maybe a content entity type that references exchange rate provider and has a serialized rates field with some info that a certain rate is overridden and until what date.. certainly not config entites for something so dynamic as exchange rates.
- 🇭🇷Croatia valic Osijek
So, let's do this for 2.x, and I would want to resolve the question of historical rates. Did leave a place for that, but never implemented.
Idea
- Use custom table - TBD for historical rates
- Use state to store rates - but this would clear cache when the state is updated
- Use content entity - we can then implement easy historical rates, but we always need to return always latest in default
calculation - Key value - this would be using builtin Drupal key-value store - but there is always heavily used in some cases, so not sure that is a good idea to put more there
I am for the first or third approach. Even the content entity seems a bit overkill and raises complexity when a new currency is added, but would be easier to manipulate with it perhaps. (even we could add this field type to store currencies as serialized values https://www.drupal.org/project/commerce_currencies_price →
Custom table though seems to best balance between cache-related questions, flexibility, and ability to store as well as historical rates.
- @valic opened merge request.
- 🇭🇷Croatia valic Osijek
Strange difference, locally running phpunit this returns OK
$this->assertEquals(round(1 / 0.602409, ExchangerProviderRemoteInterface::EXCHANGER_ROUND_PRECISION), $rates['EUR']['AUD']['value']);
but on GitlabCI we got this
1) Drupal\Tests\commerce_exchanger\Kernel\ExchangerProviderPluginTest::testImportCrossSyncTransformRates Failed asserting that 1.66 matches expected 1.660002.
- 🇭🇷Croatia valic Osijek
So temporarily did use rounding of 2 for tests, instead of 6 as we use, to not block merging.
There is a new DB table -
commerce_exchanger_latest_rates
which stores the latest rates and is used by default. Additionally, there is
commerce_exchanger_historical_rates
which can be used to store rates for longer periods per date, and that one is opt-in - the user needs to select this.Added new service to handle get/set for rates, and there are no changes around the plugin system, minimal around price calculator interfaces, so any extended code may need minimal customization, if any.
Added DB update hook to migrate existing rates.
Happy to merge this. And in follow up need to brush some additional minor details, and maybe we add event for altering rates during import, so that there is option for it
- Status changed to Fixed
about 1 year ago 9:00am 10 October 2023 Automatically closed - issue fixed for 2 weeks with no activity.