Add pluggable currency conversion to replace the fixed Yahoo! Finance conversion from 7.x-1.x.

Comments

xano’s picture

Status: Active » Needs review
StatusFileSize
new8.25 KB

The patch adds a converter interface, and one converter class that provides fixed historical conversion rates. I didn't go for a centralized table as was used in 7.x-1.x, because that would only be a limited form of central storage. That particular table was only filled with currencies for which a rate was requested from Yahoo! Finance in the past, making its Views integration somewhat useless (as you couldn't display rates until they were explicitly requested first from somewhere else in the system. My implementation lets converters choose where and for how long they want to store their data. It also allows some converters to get entire lists of rates at once, or one rate at a time.

The converter for historical rates works, but will need content (rate) updates in the future.

What still needs to be done is find a way to order available converters by weight and loop through them when requesting a rate until one of them actually returns a rate. The current code is hardcoded to use the historical converter only in currency_conversion_rate().

Status: Needs review » Needs work

The last submitted patch, currency_1848966_00.patch, failed testing.

xano’s picture

Status: Needs work » Needs review
StatusFileSize
new10.45 KB

The tests failed, because the currency_converter Ctools plugin had a process callback that checked if the plugins really did implement CurrencyConverterInterface, which required loading the plugin classes. However, because Ctools has funky magic to notify the registry on behalf of plugin classes (so no .info file entries are needed), it gets all plugins during hook_registry_alter(), which in turn invoked the aforementioned process callback, which couldn't load the classes yet to verify their integrity. Using Ctools registry magic instead of .info file entries wasn't possible either, because the plugins need CurrencyConverterInterface, which can only be exposed to the registry through the .info file.

Bottom line: I removed this integrity check.

The attached patch also contains tests, and it orders the plugins by their weight property.

xano’s picture

StatusFileSize
new8.42 KB

The patch no longer deals with conversion, because that requires us to know about how prices are stored, and that is an application-specific thing. Instead, it only provides exchange rates.

Everything works, but I'm still deciding if we want to have all exchange rates in one database table, so they can be used with Views.

xano’s picture

For the record: we can always add Views integration later. We should be able to add conversion rate storage without too much trouble after this patch is committed.

xano’s picture

Allow rates to be fetched for multiple currency combinations at once: CurrencyConverterInterface::rateMultiple(array $rates = array()). If $rates is empty, then all available rates should be fetched.

xano’s picture

Status: Needs review » Needs work
xano’s picture

Status: Needs work » Needs review
StatusFileSize
new13.75 KB
  • CurrencyConverter is now a currency conversion plugin. However, it is a special plugin in that it does not provide any conversion rates itself, but it loops through all other plugins (the order of which is configurable) and uses their rates. This way, other code and UIs can either choose to use specific conversion plugins directly, or use this one.
  • Conversion plugins now have load(), loadMultiple(), and loadAll() methods to load one, multiple (but not all), or all rates they are able to load.

Currency.module will definitely NOT feature a single database table that stores the conversion rates. The proposed code supports getting all available conversion rates with a single function call, and it is likely that a submodule will be added that uses this functionality to fill a custom database table. The reason for this is that conversion plugins may use their own cache (CurrencyConverterFixedRates uses file-based storage, for instance) and that using a database table is bad practice because of duplication, unless specific use cases can only be solved using one.

Status: Needs review » Needs work

The last submitted patch, currency_1848966_03.patch, failed testing.

xano’s picture

Status: Needs work » Needs review
StatusFileSize
new13.81 KB

Re-roll.

amateescu’s picture

Status: Needs review » Needs work
+++ b/currency.currency.inc
@@ -13,6 +13,24 @@ function currency_currency_info() {
+      'class' => 'CurrencyConverter',

Why does this class have to be a converter plugin? It's just a factory after all, no?

+++ b/includes/CurrencyConverter.inc
@@ -0,0 +1,89 @@
+  static function load($iso_4217_code_source, $iso_4217_code_destination) {

I still hate this variable names with great passion. As far I see, we are only dealing with ISO 4217 codes here, so why can't we document that somewhere and give more humanly names to these poor arguments?

xano’s picture

Why does this class have to be a converter plugin? It's just a factory after all, no?

Consistency and documentation were reasons for me to make CurrencyConverter implement CurrencyConverterInterface, and if it's a plugin anyway, why not expose it as one? We may not have any use for it yet, but I imagine there will be applications that do. It *is* a conversion plugin, but rather than using its own unique source to fetch rates, it uses all other plugins at once.

I still hate this variable names with great passion. As far I see, we are only dealing with ISO 4217 codes here, so why can't we document that somewhere and give more humanly names to these poor arguments?

Consistency with bartfeenstra/currency and self-documentation were my reasons for naming the variables this way. Besides that, currency_code and iso_4217_code are equally long.

amateescu’s picture

and if it's a plugin anyway, why not expose it as one?

+++ b/includes/CurrencyConverter.inc
@@ -0,0 +1,89 @@
+    // Unlist this class.
+    unset($plugins['CurrencyConverter']);

Exposed where? :) No, I really think this not a good design..

Besides that, currency_code and iso_4217_code are equally long.

They might be equally long, but iso_4217_code sounds awful in comparison to me.

+++ b/includes/CurrencyConverter.inc
@@ -0,0 +1,89 @@
+  static function load($iso_4217_code_source, $iso_4217_code_destination) {

So.. $currency_from, $currency_to really doesn't look better to you?

xano’s picture

Exposed where? :) No, I really think this not a good design..

It's exposed in currency_currency_converter_info(). The unset() is used to make sure the converter does not use itself when fetching rates; this has to do with loading the list of other converters that are available, and not with how CurrencyConverter works conceptually. No matter how you look at it, CurrencyConverter functions like a conversion plugin, so I see no reason not to expose it.

So.. $currency_from, $currency_to really doesn't look better to you?

We can't name those variables that way, because they contain currency codes, and not currencies (which in the context of Currency are objects). But fine, let's name them $currency_code_... then, but we'll lose a bit of self-documentation.

xano’s picture

Status: Needs work » Needs review
StatusFileSize
new13.58 KB

I updated the iso_4217_... variable names to currency_code_....

amateescu’s picture

Status: Needs review » Fixed

Committed to 7.x-2.x. Thanks for bearing with me :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xano’s picture

Assigned: xano » Unassigned