Add a currency resolver API (with a CurrentCurrency object).

Created on 13 March 2019, over 6 years ago
Updated 13 October 2023, almost 2 years ago

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.

Feature request
Status

Needs work

Version

2.0

Component

Other

Created by

🇷🇸Serbia bojanz

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update almost 2 years ago
    787 pass
  • 🇭🇷Croatia valic Osijek

    Can we revive this :-D

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

  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update almost 2 years ago
    791 pass
  • Issue was unassigned.
  • 🇭🇷Croatia valic Osijek
  • 🇭🇷Croatia valic Osijek

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

  • 🇭🇷Croatia valic Osijek

    valic changed the visibility of the branch 3039854 to hidden.

  • Pipeline finished with Failed
    10 months ago
    Total: 487s
    #312957
  • Pipeline finished with Failed
    10 months ago
    Total: 642s
    #312965
  • Pipeline finished with Failed
    10 months ago
    Total: 637s
    #313022
  • Pipeline finished with Failed
    8 months ago
    Total: 174s
    #372734
  • Pipeline finished with Success
    8 months ago
    Total: 164s
    #372739
  • Pipeline finished with Success
    8 months ago
    Total: 160s
    #373754
  • Pipeline finished with Success
    8 months ago
    Total: 180s
    #373762
  • Pipeline finished with Success
    7 months ago
    #384859
  • Pipeline finished with Success
    13 days ago
    Total: 812s
    #552375
  • Pipeline finished with Failed
    13 days ago
    Total: 691s
    #552391
  • Pipeline finished with Success
    13 days ago
    Total: 947s
    #552410
  • Status changed to Needs review 13 days ago
  • 🇭🇷Croatia valic Osijek
  • 🇮🇱Israel jsacksick

    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:

    1. We define a new API, but besides defining a currency resolver, we aren't actually relying on the chain currency resolver? Maybe this is fine as this means no risk of introducing a performance regression I suppose.
    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...
    3. Perhaps we need a test price resolver that uses the new chain currency resolver as an example? Would have made sense to have it available from the Context I guess. 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.
    4. Perhaps we could extract the ServiceCollection from this MR: https://git.drupalcode.org/project/commerce/-/merge_requests/438/diffs#8... so we collect services the "new" way :).

    Back to the last point, perhaps ServiceCollection should be renamed ServiceCollector and getCollection() to just getServices()? Or getCollectedServices()...

  • 🇮🇱Israel jsacksick

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

    The ChainCurrencyResolverInterface could almost be skipped... We no longer need an addResolver() and we don't really need th getResolvers() method.

    We could keep the interface to be able to distinguish between the chain currency resolver and resolvers themselves, but not 100% sure we need to expose the resolvers to the outside world... Kept it in case there is a usecase for accessing the resolvers? But we aren't even calling the getResolvers() method ourselves...

    Also implemented return types and defined a service alias for autowiring.

  • 🇭🇷Croatia valic Osijek

    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

  • 🇮🇱Israel jsacksick

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

  • 🇭🇷Croatia valic Osijek

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

  • 🇮🇱Israel jsacksick

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

Production build 0.71.5 2024