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.

Issue fork commerce-3039854

Command icon 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

bojanz created an issue. See original summary.

bojanz credited valic.

bojanz’s picture

Crediting valic for the contrib module.

zaporylie’s picture

Status: Active » Needs work
Issue tags: +Needs tests
StatusFileSize
new8.57 KB

Here's initial patch which defines services and interfaces mentioned in issue summary. No tests attached.

mglaman’s picture

Assigned: Unassigned » mglaman

Reviewing as part of a project where I just wrote my own currency selector 😬

mglaman’s picture

Okay, I'll be picking this up. We need pricelists that are per currency. This blocks #3096016: [PP-1] Allow providing prices for different currencies

valic’s picture

Can we revive this :-D

Pushed code from @zaporylie and added some basic test coverage for chain resolver and current currency

valic’s picture

Version: 8.x-2.x-dev » 3.0.x-dev
Assigned: mglaman » Unassigned

valic changed the visibility of the branch 3039854-add-a-currency to hidden.

valic changed the visibility of the branch 3039854 to hidden.

valic’s picture

Status: Needs work » Needs review
jsacksick’s picture

Status: Needs review » Needs work

I'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:

  1. Commerce doesn't actually use the CurrentCurrency service anywhere?
  2. I wonder if we should expand the Context object to be aware of the currency? Though it's always instantiated manually so perhaps not a good idea... But this way price resolvers would be aware of the currency without the need of injecting the CurrentCurrency service?
  3. If we aren't modifying the Context, perhaps we need a test price resolver that leverages the CurrentCurrency service as an example? At the same time... The order probably already has a currency when price resolvers are invoked for a given order item, so currency could be set using the order total currency code instead? But maybe the whole point of the CurrentCurrency is to be able to switch the order currency depending on the context and have different prices resolved on during the OrderRefresh?
  4. Let's switch to the AutowireLocator attribute for collecting tagged services.
jsacksick’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

I decided to skip defining a service collector but instead went with the AutowireIterator attribute.

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?

valic’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new260.95 KB

1. 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

Resolver

jsacksick’s picture

@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...

valic’s picture

I 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()

jsacksick’s picture

I'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).

jsacksick’s picture

One 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.

jsacksick’s picture

StatusFileSize
new43.61 KB

Added a CurrentCurrency condition 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 CurrentUserRole but 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:

Current currency

  • jsacksick committed f54f6c02 on 3.x authored by valic
    Issue #3039854 by valic, jsacksick, zaporylie, bojanz, mglaman: Add a...
jsacksick’s picture

Version: 3.0.x-dev » 3.x-dev
Status: Reviewed & tested by the community » Fixed

Merged!! Thank you everyone :).

Status: Fixed » Closed (fixed)

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