Description

This is an add-on module for Drupal Commerce that provides a way to display formatted prices according to locale information.
For example, with this module a Euro product price could be displayed as €30,000.00 for visitors from Ireland, while visitors from France would see it as 30 000,00 €.

This module also provides a countries/currencies list, as well as a Rules action that convert country code to its currency. Useful when you want to set the user currency according to his/her country, works well with the Commerce Multicurrency module.

This module differs from Currency for Drupal Commerce as it uses the Drupal Commerce currency system while Currency for Drupal Commerce replaces it.

Project URL

https://drupal.org/sandbox/LondonITGuys/2035115

Git

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/LondonITGuys/2035115.git commerce_currency_locale

PAReview

http://ventral.org/pareview/httpgitdrupalorgsandboxlondonitguys2035115git
I know there are some errors and warnings, but I was hoping I could get some feedbacks about them:

  • Do the PHP variable names have to be lower case?
  • Do long arrays without key need to be written vertically?

Thank you!

Review bonus

Working on it...

Comments

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Concept review

I'm the maintainer of Currency, which integrates with Drupal Commerce through Currency for Drupal Commerce.

My first thought, and I hope you can understand this, is that this module does a lot of things that is already done by other modules. Things that are complex, and of which we ideally do not need more implementations that are not fully correct.

One of those things is the country to currency mapping. It is impossible to do this 1:1, which is why Currency, for instance, allows countries to have multiple currencies associated with it, in case countries require this.

The main thing it does wrong, is that the localized formatting is incorrect. It requires per-currency locale settings, while formatting should not be done based on the currency, but on the locale the price is displayed in. See Unicode's CLDR. This is done partially right, but it still won't work like it should.

This module does more than what Commerce does by default, but as Drupal has too many broken number/price formatters already, and I have spent a lot of time on researching the right way to do this when I wrote Currency 7.x-2.x and 8.x-3.x, I would like to advise this project leverages such modules instead of reinventing a wheel that is not even round.
@jibize, if you would like to collaborate on a more solid solution, I want to invite you to the Currency queue, where we can discuss future development for both 7.x-2.x and 8.x-3.x.

Code review

At first glance the code looks solid and well-documented. One point of attention is that if a function's return value is an array, the array structure needs to be documented too.
Also, the callbacks that hook_commerce_currency_locale_geoloc_module_info() should return are described as functions. This is incorrect (PHP is not JavaScript, for instance). Do you mean function names? If so, please allow any callable to be returned, so as to allow plugins to be OOP.

jibize’s picture

jibize commented

Concept review

Thank you very much Xano for your input.

I understand your point and I agree that the Currency module is a much more solid solution. I created this module mainly because of rszrama comment from https://drupal.org/node/1047270#comment-6972120, the idea is to try to do some locale/currency logics while keeping the Drupal Commerce currency system, but this may not be the way to go... Maybe someone from the Commerce Guys Team could also give his/her opinion on this?

In any case I wouldn't mind if this module is not going full project as I never really created a Drupal module before and I learnt a lot anyway! ;-)

I will take a look at the Currency module queue thank you Xano!

Code review

I figured that I would keep learning anyway so I made some changes to the docs, thanks!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » klausi

I'll look at this now in the Project applications sprint

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: klausi » Unassigned
Status: Needs review » Closed (won't fix)

Ok, so it looks like you can accomplish your goals with the Currency module and we always prefer collaboration over competition. Thanks for working with the community!

If you have changed your mind and you think you must release a separate project, please re-open this application (and don't forget to state the exact differences on the project page then).

Otherwise feel free to get back to us with a new project that you want to promote!

jibize’s picture

jibize commented

Thank you Klausi for looking at the project application.

Yes you are right, as Xano has rightly written his module provides the same functionality and more, I guess I got a little carried away when I first started working on this and didn't properly look at the Currency module.

Thanks again to both of you.