Latest exchange rates shouldn't be saved in config

Created on 14 June 2023, about 1 year ago
Updated 10 October 2023, 9 months ago

Saving those values in config makes them being overwritten on every deploy with some old values from the repo.

The latest exchange rates config is a dependency for exchanger config so it's not possible to gitignore the file as it'll lead to deploy errors when trying to import config.

It'd be best to refactor that so latest rates are kept in the site state, for now due to limited time I can post a workaround where that latest exchange rates config will not be a dependency so we can safely gitignore it.

🐛 Bug report
Status

Fixed

Version

2.0

Component

Code

Created by

🇵🇱Poland Graber

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @Graber
  • Issue was unassigned.
  • Status changed to Needs review about 1 year ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update about 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 about 1 year ago
  • 🇭🇷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.

  • 🇭🇷Croatia valic Osijek
  • @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

    • valic committed 22fae3dd on 2.0.x
      Issue #3366705: Latest exchange rates shouldn't be saved in config
      
  • Status changed to Fixed 9 months ago
  • 🇭🇷Croatia valic Osijek
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.69.0 2024