Closed (fixed)
Project:
Currency
Version:
7.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
23 Nov 2012 at 22:18 UTC
Updated:
24 Jul 2013 at 08:46 UTC
Jump to comment: Most recent file
Comments
Comment #1
xanoThe 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().
Comment #3
xanoThe 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.
Comment #4
xanoThe 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.
Comment #5
xanoFor 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.
Comment #6
xanoAllow 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.Comment #7
xanoComment #8
xanoload(),loadMultiple(), andloadAll()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.
Comment #10
xanoRe-roll.
Comment #11
amateescu commentedWhy does this class have to be a converter plugin? It's just a factory after all, no?
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?
Comment #12
xanoConsistency 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.
Consistency with bartfeenstra/currency and self-documentation were my reasons for naming the variables this way. Besides that,
currency_codeandiso_4217_codeare equally long.Comment #13
amateescu commentedExposed where? :) No, I really think this not a good design..
They might be equally long, but
iso_4217_codesounds awful in comparison to me.So..
$currency_from, $currency_toreally doesn't look better to you?Comment #14
xanoIt'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.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.Comment #15
xanoI updated the
iso_4217_...variable names tocurrency_code_....Comment #16
amateescu commentedCommitted to 7.x-2.x. Thanks for bearing with me :)
Comment #18
xano