- Issue created by @mrweiner
- ππΊHungary djg_tram
This module clearly states in the README that it doesn't do any resolving on its own but uses resolvers found in the system. Which is, in my opinion, the correct approach because it separates the actual multi-currency support from a selection of additional behaviors whose requirements might be different from site to site. I was well aware of that module before I started working on mine and, frankly, this was one of the reasons, I needed a cleaner solution that makes the multi-currency handling as automatic as possible β and I thought that others might benefit from it, too.
I added explicit support for Commerce Exchanger because it's a very common requirement (I use it, too). Other resolvers can be added in a similar way if it's not possible to rely on them automatically, without any extra support, just as part of the resolver chain.
- πΊπΈUnited States mrweiner
This was a poorly thought out issue for me to have opened. I think I see what you're saying. From an implementation perspective, I don't think there is an "integration" to be built here -- commerce_currency_resolver would just need to add support for your multi-currency field in their resolver for the two to be compatible.
I think I'm just stuck in the middle where I like the approach and field over here, but I need the more robust geo-handling that they have over there.
Their geo-based currency determination actually isn't in a resolver, but just some service and config handling in their CurrentCurrency service. So that might turn this into a feature request ticket for geo-handling, which would basically be copy-pasting their code into this module. That just results in the same code essentially being maintained in two places, though, which isn't ideal but maybe unavoidable.
- πΊπΈUnited States mrweiner
Or, I suppose, another option would be some way to leverage their CurrentCurrency service logic inside of commerce_currencies' CurrentCurrency logic, and then somehow bypass their resolver. Maybe this is just something I need to patch in on our end and not something you need to support.
- ππΊHungary djg_tram
This was a poorly thought out issue for me to have opened.
Not necessarily. :-) Now that I looked into the code, it's not really much. It looks for either of the two dependent geo-ip packages and if present, asks them for the country. Really nothing duplicated, just a single call in each case. And then
commerceguys/intl
seems to maintain a country > currency map that could be queried to get the currency. All that could be placed into a very simple submodule providing this optional resolver. I'm not sure I'll have time to play with this for a week or two, and I also have something else I promised in another issue to look into but if you feel like experimenting with it, sure. Or wait a little and I'll try to do it a bit later. - πΊπΈUnited States mrweiner
Updating the issue title :)
Yeah you're right, simplest option would be to use that
commerceguys/intl
currency map. Commerce Currency Resolver has a whole admin config form to be able to select which currencies apply to which countries. But that's probably overkill in practice, and overrides could be handled with a hook/event. Good call.Yeah I'll do some experimentation. Do you think that an additional resolver is even needed for this? Seems like it could be handled in CurrenCurrency as an additional case. That's how CCR does it. There's a check for the modules in their config form, and then if the geo option is chosen then it's used in CurrentCurrency.
- ππΊHungary djg_tram
Yes, that's the possibility but then you probably would want a new setting for the field? Even if the geo module is present, the admin might not want to use it or not for all price fields.
- πΊπΈUnited States mrweiner
Alright, draft MR is ready that adds the basic geolocation functionality. Leaving it a draft until I can add interfaces for method documentation.
- Adds configuration form/route + settings config and associated permission
- Adds CurrencyHelper service to determine available geo modules, get currency for the user's country, etc
- Updates CurrentCurrency to leverage geolocation if the setting is enabled
- Updates the cookie case to leverage new isUsableCurrency method instead of using loadByProperties. This will be slightly more performant since loadByProperties is not cached, whereas loadMultiple is.
I've added the geolocation setting as a module-level setting as opposed to a field-level setting. I'm not sure of a case where you'd want to use geolocation for one field's currency but not another. It would also add some complications due to interactions between field-level handling and the current global cookie handling.