decimal count for currencies

Why don't we just have a centralized currency formatting function, that is also themable?

This way, you can override the value in the theme any way you like, and do away with the decimals if you want, or add the symbol if you want, ...etc.?

That is what I am saying, a unified way to output the currency is a theme_currency($symbol, $amount) function that can be overridden.

Inside that we can use money_format() by default with no side effects since it is in PHP 4 and 5.

Any patches?

Example from user.module:

function theme_user_picture($account) {
  if (variable_get('user_pictures', 0)) {
    if ($account->picture && file_exists($account->picture)) {
      $picture = file_create_url($account->picture);
    }
    else if (variable_get('user_picture_default', '')) {
      $picture = variable_get('user_picture_default', '');
    }

    if (isset($picture)) {
      $alt = t("@user's picture", array('@user' => $account->name ? $account->name : variable_get('anonymous', t('Anonymous'))));
      $picture = theme('image', $picture, $alt, $alt, '', FALSE);
      if (!empty($account->uid) && user_access('access user profiles')) {
        $picture = l($picture, "user/$account->uid", array('title' => t('View user profile.')), NULL, NULL, FALSE, TRUE);
      }

      return "<div class=\"picture\">$picture</div>";
    }
  }
}

You call it like this:

$output .= theme('user_picture', $user);

So, you have to define the following function in the module.

function theme_currency_amount($symbol, $amount) {
....
}

Then call it via:

$output .= theme('currency_amount', '$', 100.26);

Then to override it, you create a function called phptemplate_currency_amount() in the template.php of your theme, and it will override what is in the module.

Read more about theming as it is explained in far more detail.

You are on the right track.

Think about it for some time until it firms, and then make a patch.

Also, it would be nice to use format_number() inside the the theme function.

As for the number of arguments, we should consider making it a keyed array, instead of having 4 arguments. And we can check if the argument is not an array, then it is only the amount and we act accordingly.

array(
'amount' => $amount,
'symbol' => '$',
...
);

As for the original question, the whole idea is not to clutter the module with special cases.

Now, on your site you do this in template.php:

phptemplate_currency_amount($amount, $currency, $symbol, ...) {
  switch($currency) {
    case 'JPY':
      return number_format($amount);
    default:
      return number_format($amount, 2);
  }
}

Countries using comma or period for thousands separators can use whatever they like.

I am taking the "less code" approach.

Most currencies just work right now, and only a minority need this feature.

So, having those few sites that use those minority currency override things in template.php makes the module leaner for everyone else (the majority).

I am against creeping featuritis and bloat and try to avoid adding code for less used features.

Less code is better code ...

Adding the extra field makes the array bigger and consume more memory. It also makes it added work to maintain in the future (more elements, prone to editing).

Perhaps if we move the currency info array to a database table that is created on the module install, we can add that extra field, without being too concerned with memory usage. We can provide a default theme function that will cater for all currencies, yet will still be overridable for those want to do so.

OK, roll a patch with the added field in the array and a theme function to display the stuff.

Then measure using devel's performance module and see the difference before and after.

If it is negligible, I will accept it.

We need the patch to go in for 6.x later.

The patch does not apply.

Can you please make sure that patches are created against CVS, with the tag DRUPAL-5, or the 5.x-1.x-dev tarball?

Still needs work.

We have two functions that return currency information:

The old one is currency_api_get_list() which returns the code and name, and new one is currency_api_get_currencies() with code, description and symbols.

There is also currency_api_get_symbols() which can be folded into that too.

Having data in many places is a bad thing. Better unify them all in one place, so maintenance means just one place to change.

Also, we need to do this to 6.x-1.x-dev when 5.x is done.

I am inclined just to break the API, and make this 5.x-2.x-dev and document that the old functions do not exist. This is what Drupal core does to stay lean: break the API, and document the changes so people can migrate.

We can document the API in the README.txt file as a new section that lists the functions to be used.

Alternately, Can't we just change them into wrapper functions? Internally they call the new function and return only the needed info from the big array.

I prefer we cleanup the API and create 5.x-2.x-dev.

What do others think? Hopefully we get some comments.

I noticed the patch is against an older version

-//$Id: currency_api.module,v 1.4.2.18 2008/10/30 16:45:57 kbahey Exp $
+//$Id: currency_api.module,v 1.4.2.17 2008/10/20 14:33:16 kbahey Exp $

Can you please reroll against the last version?

Also, can you include in it updates to API.txt to reflect the latest API?

Sweet addition. I'll be using the decimal count for the Money CCK field :)

#217842: Format currencies according to their own custom, add hints for this to the Currency API

Would you mind checking the attached patch?

It implements currency_api_get_currencies() as in previous patches, but then it keeps the current APIs reusing the array that is provided by the new API, so the data is just stored in one single place and it offers backwards compatibility with existing theird party modules that depend on the currency api.

For the theme function... I would suggest taking the Format Number API into account here as well, since it provides a method to define thousand separator and decimal point symbols. Anyway, this thing could be provided on a separate patch if needs be, maybe?

The attached patch would be enough for the Money CCK field to reuse the decimal count settings per currency.

BTW, the patch here is against the -dev package for D6.

hmm... maybe it is not really the best place here to provide a theme function for currency amount?

For the Money CCK field I would happily use the new API with information about decimal places per currency, as an option, but not the theme function as it doesn't really fit the needs of the field formatter, and I prefer using the Format Number API for the amount.

Would it be possible to review the patch in #24 and discuss the theme function after that one gets in?

@markus_petrux

I am ok with the patch in #24.

However, do we really need a separate module to implement a theme function? I think it is kind of overkill with more dependencies, more things to maintain/upgrade, ...etc.

I can commit #24. But would like to know your (and others') views on a 3rd module dependency. Rather add a theme function in currency_api, even if the other module can enrich it by overriding the function. So we have a basic function that is suitable for most people in currency_api, and those who want to be more fancy can install the other module.

Thoughts?

Ok, thanks for taking a look at this. :)

Here's a new patch that includes #24 + the theme function in #21 + doxygen for functions where it was missing.

Committed.

Thank you!

Thanks! :-D

Any chance for a new release?

@kbahey: Could you please create a new release so that the changes introduced here are included on a official release?

Latest Money CCK field needs this, and an official release here would save a few headaches to users. Please?

Thank you :)

markus_petrux, I granted you CVS commit access to this module, since you are a long time contributer to Drupal.

You can go ahead and create a stable release yourself. Be sure to run the cvs release notes script.

Close this issue when you are done.

Khalid, thanks for the confidence. Though, I had some trouble creating the release: #455380: Request to fix or delete a failed project release. I hope it can be fixed asap.

Release node has been fixed! :-D