- Merge request !183Issue #3039854: Add a currency resolver API (with a CurrentCurrency object). → (Open) created by valic
- 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
- last update
almost 2 years ago 791 pass - Issue was unassigned.
- Status changed to Needs review
13 days ago 5:29pm 20 July 2025 - 🇮🇱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:
- 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.
- 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...
- 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.
- 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 justgetServices()
? OrgetCollectedServices()
... - 🇮🇱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 anaddResolver()
and we don't really need thgetResolvers()
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 implementedI 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).