Commerce has the concept of a current country, locale, store.
It also has price resolvers, which allow selecting a price based on these, or other factors.
What it doesn't have is the concept of a current currency, that could be used by price resolvers.
This would be useful for example in Commerce Pricelist, cause it would allow defining prices per-currency and having them automatically selected. Relevant contrib module: http://drupal.org/project/commerce_currency_resolver
So, the plan is:
- CurrentCurrency and CurrentCurrencyInterface in \Drupal\commerce_price
- CurrencyResolverInterface, ChainCurrencyResolverInterface, ChainCurrencyResolver in \Drupal\commerce_price\Resolver
- StoreCurrencyResolver in \Drupal\commerce_store\Resolver which returns the store currency.
That means no DefaultCurrencyResolver in commerce_price, cause we have nothing to base the decision on, except returning the first alphabetical defined currency, which doesn't feel very useful.
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | current-currency-condition.png | 43.61 KB | jsacksick |
| #16 | resolver.gif | 260.95 KB | valic |
| #4 | 3039854-4-defines-structure.patch | 8.57 KB | zaporylie |
Issue fork commerce-3039854
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
bojanz commentedCrediting valic for the contrib module.
Comment #4
zaporylieHere's initial patch which defines services and interfaces mentioned in issue summary. No tests attached.
Comment #5
mglamanReviewing as part of a project where I just wrote my own currency selector 😬
Comment #6
mglamanOkay, I'll be picking this up. We need pricelists that are per currency. This blocks #3096016: [PP-1] Allow providing prices for different currencies
Comment #8
valicCan we revive this :-D
Pushed code from @zaporylie and added some basic test coverage for chain resolver and current currency
Comment #9
valicComment #13
valicComment #14
jsacksick commentedI'm generally not against the idea but it feels odd to me to define a new API Commerce itself isn't really leveraging? Reviewed the MR and left a few comments but also:
Comment #15
jsacksick commentedI decided to skip defining a service collector but instead went with the
AutowireIteratorattribute.Note that the ChainCurrencyResolverInterface now only extends the CurrencyResolverInterface and doesn't define a getResolvers() or an addResolver() method since I went with the attribute.
I'm simply wondering if we need to expose the resolvers to the outside world or not... Maybe could be added as a followup if requested?
Since return types were added for currency resolvers, then NULL needs to be explicitly returned, otherwise an error is returned.
I also defined service aliases for the new services that are available. The CurrentCurrency service and the ChainCurrencyResolverInterface..
I think the only problem maybe with the current approach is that it's inconsistent with existing service collectors... But that may simply our recommended way of collecting services moving forward?
Comment #16
valic1. Yes, it's not used anymore currently, so there is no changes in any of existing API-s.
2. Expanding context maybe have a sense, but it would introduce BC for that part. For majority of the shops probably which are single currency it does not benefit them, only multi currency which again is granulated how it's implemented
I am OK to RTBM this.
I did test with couple of my resolver, language, etc, it works great
Example of resolving per user language
Comment #17
jsacksick commented@valic: Just wondering about keeping / reintroducing
getResolvers()to expose the resolvers to the outside world.If this is later needed, that means introducing a breaking change but at the same time, it may not be needed :p.
It shouldn't really be needed, when I look at the other chain resolvers, we aren't even calling getResolvers() internally, which means if the chain resolver is extended / swapped, overriding getResolvers() wouldn't even have an impact...
Comment #18
valicI don't see it as needed, from perspective of per example commerce_currency_resolver, I did now wrote three resolvers with different weights, so they can be used even simultaneously, etc: cookie resolver first, if there is no cookie, geo resolver goes next, etc..
Don't have need to interact with getResolvers()
Comment #19
jsacksick commentedI'm fine committing this as is, but giving @zaporylie an opportunity to review this since he worked on this (even though that was years ago).
Comment #20
jsacksick commentedOne thing that we're missing that could be eventually addressed as followup is a CurrentCurrency condition plugin. This would be super helpful with promotions & shipping for example.
Comment #21
jsacksick commentedAdded a
CurrentCurrencycondition plugin.Not sure if we should add a "negate" checkbox. We skipped it for UX reasons in the past, which is why I skipped it here. Was wondering about copying the "Matching strategy" from the
CurrentUserRolebut it doesn't make sense here as the current currency cannot match all the currencies selected...So it's either "any" of the currency selected, or none of the selectec currencies.
See the screenshot below:
Comment #23
jsacksick commentedMerged!! Thank you everyone :).