Currently, the Commerce Price module stores price amounts in an integer column. When a price is entered on a widget form, in hook_field_presave() it is converted to an integer using the API function commerce_currency_amount_to_integer(). This API function essentially multiplies the number by 10^(number of decimals in the currency) and casts the return value to an integer to round it for database storage in the integer column. Then in hook_field_load(), we convert this back to a decimal amount value from an integer using the opposite API function.

Randy pointed out that the better place to make this transformation was at display time. When we're performing operations on the data, we should leave it in the form we stored it in the database in. Making this switch would have the added benefit of getting us around some nasty errors that arose from our use of hook_field_presave(). The problem is that because the hook works on the field items by reference, any changes you make persist through the remainder of the page request. This means price amounts are suddenly showing up multiplied by 100 (this has been reported as a bug numerous times). To work around that, we were simply saving a clone of an order / line item when we knew we needed to display prices on that entity in the current page request. The problem is since we saved a clone, we lost any changes made to the object on save... this spawned other issues, such as the order total appearing out of sync when taxes were calculated until you refreshed a page. (Behind the scenes, taxes were actually applied and saved properly, and the order total was recalculated properly... we just were getting the display values from a stale order object.)

Unfortunately, during the course of my patch making, I discovered we still need the presave! The problem is price fields have a text data column that should contain a serialized array. We even noted in the field schema this is a serialized column... but unlike drupal_write_record(), the field API does not automatically serialize columns for you on save. This annoyed me to no end (read: for days) until I found workarounds in the field attach API that I hope are sufficient.

Once this was complete, I still had to resolve data entry issues on product, line item, and payment forms. When prices are entered, users will certainly expect to enter them as decimals, so we had to implement the appropriate validation handlers to convert the values to integers on entry (i.e. instead of converting on load) using the renamed API function commerce_currency_decimal_to_amount().

Note that "amount" now has been redefined to be the integer value - or more specifically the value in minor units expressed to the Nth decimal as specified by the currency. It's still possible for amounts to be decimal values, but they should be rounded to integers prior to database writes. While this generally means the amount is the price in the minor unit of the currency, it could actually be a fraction of the minor unit (e.g. if you altered your site to display U.S. Dollars to the thousandth instead of the hundredth by setting its decimals value to 3). Such alterations will be destructive, so they're best done at the very beginning of a site's development if it's a known requirement... otherwise you have to convert prices anywhere an amount is stored in the database, which includes field data and transaction data right now (and who knows what else in contrib).

I'm going to post the patch in a follow-up comment that links to related issues so I can use this nid in the patch name and keep the list of related issues up to date via edits.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama’s picture

Status: Active » Needs review
FileSize
24.24 KB

Attached patch encompasses the changes mentioned above and provides fixes in whole or in part for these related issues (some of which will now be marked duplicates of this one). Some tangential issues in payment were resolved regarding what API function we used to find the order total, too. Please give this a thorough review by applying the patch and doing anything you can think of in the UI or API to create and manipulate entities that have prices.

rszrama’s picture

Developers should be aware - as soon as this patch lands, you will likely need to update your payment integration or custom modules that interact with the price API / price field data. It's not that hard... you should just always convert decimal values to integer amounts on data entry. Price formatting will now automatically convert to the decimal value, but if you need to communicate the price to your payment gateway in decimal form, you'll need to use the API function commerce_currency_amount_to_decimal().

Once this patch lands, I intend to roll a beta3 so you can benchmark your modules against that once they're compatible.

Damien Tournoud’s picture

 function commerce_payment_transaction_save($transaction) {
...
+  $transaction->amount = round($transaction->amount);

Hu?


     $form['payment_terminal']['amount'] = array(
       '#type' => 'textfield',
       '#title' => t('Amount'),
-      '#default_value' => $default_amount,
+      '#default_value' => commerce_currency_amount_to_decimal($default_amount, $default_currency_code),
       '#size' => 10,
       '#prefix' => '<div class="payment-terminal-amount">',
     );
@@ -169,6 +169,16 @@ function commerce_payment_order_transaction_add_form_add_refresh($form, $form_st
  * Validation callback for commerce_payment_order_transaction_add_form().
  */
 function commerce_payment_order_transaction_add_form_validate($form, &$form_state) {
+  // If a payment method has already been selected...
+  if (!empty($form_state['payment_method']) && !empty($form_state['values']['amount'])) {
+    if (!empty($form_state['values']['amount']) && !is_numeric($form_state['values']['amount'])) {
+      form_set_error('amount', t('You must enter a numeric amount value.'));
+    }
+    else {
+      form_set_value($form['payment_terminal']['amount'], commerce_currency_decimal_to_amount($form_state['values']['amount'], $form_state['values']['currency_code']), $form_state);
+    }
+  }

Both things belong in either an #element_validate + value callback or a #process. As much as possible, this should be self-enclosed into the form layer.

+/**
+ * Implements hook_field_insert().
+ */
+function commerce_price_field_attach_insert($entity_type, $entity) {
+  _commerce_price_field_attach_unserialize_data($entity_type, $entity);
+}
+
+/**
+ * Implements hook_field_update().
+ */
+function commerce_price_field_attach_update($entity_type, $entity) {
+  _commerce_price_field_attach_unserialize_data($entity_type, $entity);
+}

Let's fix the name of the hook in the docblock and explain why we have to use the field attach hooks here (the field hooks are called *before* the operation, not after).

Also, in _commerce_price_field(_attach)?_unserialize_data() should iterate over all the languages in the incoming delta array, not only the suggested languages from the field API.

This whole thing is a big WTF, and would at least warrant a core issue.

+    $default_amount = round(commerce_currency_amount_to_decimal($items[$delta]['amount'], $currency['code']), $currency['decimals']);

Is there really a need to round?

rfay’s picture

Subscribing. @DamZ, note that (I think) he's rounding at the *minor* currency level. There might still not be a need to round, but it does make sense to round at that level (cents, not dollars).

Congratulations on wrestling this to the ground, Ryan.

rszrama’s picture

Good catch on the documentation mismatch, Damien. That's obviously a relic of my first attempt until, like you mentioned, I realized there was no post-save hook. In case it isn't obvious, the field attach hooks will get called after save, they just don't directly receive the items array (just the entity type / entity). Therein lies the need for the mondo helper function. I can revise it to use all passed in languages; I just had copied that code directly from somewhere else that I can't find at the moment. Sigh.

For others reading along, this workaround allows me to unserialize the data value after an insert or update has occurred so the data is accessible throughout the remainder of the page request. Without this, after an order is saved, when we try to display the order total using the "components list" display formatter, Drupal spills a lot of red ink letting you know the data is a string. : P

I'll update these docs and address the other issues Damien raised. Regarding rounding, I tried to leave rounding in place where it was before. The transaction amount rounding should go into its controller I suppose (just needs to round b/c the DB column is an integer - but this is better suited to the controller so an alternate controller can be used if someone decides to change the data type of that column). The default amount rounding was to prevent floating point errors from showing up in default amounts like "1.200000001" or something... it may not be necessary, so I'll look into how numbers appear without it or with different parameters.

I can switch that general validate handler to an element validate handler, though I wonder if it's safe to assume in an element validation function that other elements will be present on the form (namely the currency code, since it's required for validation). I suppose I could put the element validation handler on the payment_terminal array instead of amount. What do you think?

rfay’s picture

This is just a comment/trial balloon - would have chatted in IRC but didn't find you.

+  return trim(t('@code_before @negative@symbol_before@price @symbol_after @code_after', $replacements));

Is this really translatable? Maybe it is. If it is, shouldn't we be using locale-type features to do it?

A trainee last year pointed out to me that t() should really be used for translation, not for stringbuilding. PHP has great things like sprintf() and other tools for building strings like this. We just get in the habit of using t() for lots of things that don't have to do with translation and that is perhaps silly.

rfay’s picture

Status: Needs review » Needs work

Taking this for a test drive.

Don't we have to provide an update hook to update prices that are already on products? I might give this a spin after talking with you, @Ryan.

hunziker’s picture

A simple question: Why we use integers instead of a decimal and store the value as the most user expect format? With this we can avoid numerous problems:

  1. Updates of decimal values from 2 to 3 digits has no converting effects
  2. The views integration works natively
  3. Less conversion is need for integration
rszrama’s picture

Hmm, I made it translatable not just to have string building capabilities but on the off chance that the formatting options aren't flexible enough for a given language. For example, it might make sense for the negative symbol to be in front of a preceding currency symbol. So while it's not for translation, it's for other localization. However, it could be that it's best to use a custom format callback. : ?

And hmm... while the price amounts weren't harmed in the making of this patch, it's likely that old data columns will contain decimal values instead of integers in their components arrays. That's gonna be a bear to update... is it possible to perform a batch operation on update do you know?

hunziker’s picture

I thought a bit about the following transformation:

You have USD with 100 cents. You change this into 1000 cents and then back into 100 cents. (Move the decimal point from 2 to 3 and back to 2.) This means you need to update all product prices, all order total prices, order line prices and probably other prices stored in the database (for example payment or shipping fees).

If you had 1000 products and 10'000 orders. You need to recalculate all the stored prices. This are about 25'000 single operations. You cannot use the database to do it for you, because you need also to recalculate the price components, which are stored in a serialized manner. This leads to a big patch process for such a small change. Do we really want that?

rfay’s picture

@hunziker, if php had decimal capabilities instead of just integers and floats, all of this would be easy. But since it doesn't, we're exposed to the vagaries of floating-point numbers, and floating-point calculations aren't even predictable on different systems. So it's not really possible to use floats when working with money in a financial-type piece of software. That's what forced Ryan down the "use the minor currency type as an integer" approach.

@rszrama, from http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

If your update task is potentially time-consuming, you'll need to implement a multipass update to avoid PHP timeouts. Multipass updates use the $sandbox parameter provided by the batch API (normally, $context['sandbox']) to store information between successive calls, and the $sandbox['#finished'] value to provide feedback regarding completion level.

rszrama’s picture

@hunziker - There's nothing else to do, really. It's something that should obviously be decided before you fill your database up with prices.

hunziker’s picture

Ok. I see that there are problems in the handling. But if we store the date in the wrong way, we get more problems.
The database should provide some functions to convert decimals into integers on the fly. So you could store decimals in the database and then select them depending on some configurations. This would provide better integration with views and so on. How does magento handle this?

hunziker’s picture

I checked what the magento developers done:
They use a decimal(12,4) field for prices. Additionally they use the bcmath functions for localization / conversion. Would this not be the better approach? We can write a wrapper around the bcmath functions, if they are available they can be used, if not then the native in PHP written functions are used. If someone do not want to use bcmath, then he must live with the inaccurate price calculations.

rszrama’s picture

@hunziker Switching to a decimal doesn't change anything. MySQL treats decimal columns as integers, and we made the decision to use integer for database compatibility reasons a while ago. Additionally, this doesn't change anything. The problem is with price components in the serialized data array stored in the text data column, and that's why you'd need a batch operation to make mass updates.

rfay’s picture

Status: Needs work » Needs review

I was nearly completely successful using this patch after starting over.

I created a new database using Commerce Kickstart, built it, created a VAT tax (like I had before), applied the patch in #1, checked for updatedb, and went forward. I had no trouble, had no off-by-factor-of-100 issues. Nothing. So I'm going to say that the pain I had was perhaps due to having a couple of supporting modules in older states.

But for history's sake, this is the error I got, over and over, when trying to delete a tax item or a rule:

PHP Fatal error:  Unsupported operand types in /home/rfay/workspace/d7git/sites/all/modules/rules/includes/faces.inc on line 226

Moving this back to CNR as it doesn't seem that I was in my right mind.

Successfully made a screencast showing conditional tax tonight. Hurray! http://vimeo.com/22323135

hunziker’s picture

Status: Needs review » Needs work

@rszrama: Ok I see the problem is that, the decision is made and will be not changed. Is it then possible to move the serialized data into a correct database schema? Because then you can at least access the data directly and not only over your API.

BTW: How would you realize this use case:
The user don't want to convert the product prices automatically in a store with USD and EUR. He likes to set them by hand. How would you go for this?

rszrama’s picture

The attached patch addresses Damien's concerns. I did leave the function name _commerce_price_field_attach_unserialize_data() alone, as the arguments it works with are specific to the field attach process. Also, to ensure proper amount value setting, I did put the element validation handler on the parent payment_terminal element instead of the amount.

I'm doing further testing to ensure this doesn't break anything on update... I'm worried about the price component arrays of line item and order totals, as I believe they'll have price amounts stored as decimals instead of integers. This may require a batch update function.

Additionally, I've discovered bugs pertaining to tax inclusive price displays that I'm working on now...

Oooh, and I think I just realized what you meant here, Damien:


Both things belong in either an #element_validate + value callback or a #process. As much as possible, this should be self-enclosed into the form layer.

I missed that you were referring to the conversion of default amounts to decimals. Are you saying there's a way for us to convert the integer to a decimal through a process callback instead of directly invoking our API function? I don't believe I've ever done such a thing w/ FAPI but can look into it. (I did remove that round(), btw).

@hunziker - your questions are out of scope for this issue and I don't want to sidetrack it. I'm happy to chat about them in other issues or IRC, but the very short answers are price component data storage will have to be addressed in 2.x and just create currency specific price fields.

hunziker’s picture

@rszrama: Ok then let us start a new issue about the price integration for Commerce 2.x. In meanwhile to version 2.x we should look for ways to live with this weaknesses.

GuyPaddock’s picture

Subscribing...

rszrama’s picture

Alrighty, one more pass with the tax inclusive price entry bug fixed. I moved the element validate handler from the tax element to the price element, the same array as the other price validate handler that deals with data transformation. This ensures we're creating the tax price component after the base product price has already been converted from a decimal. I still need to revert and test what happens to these price components on update.

I looked into using a process callback. It appears we can use this to transform the default value of a price textfield to a decimal... will look into this as well. (This literally just clicked, as I wasn't planning on pursuing that further earlier.)

rszrama’s picture

Status: Needs work » Needs review
FileSize
35.18 KB

Alrighty, the attached patch includes update functions to properly update price components. All I'm doing is manually converting the components of each product's "commerce_product" field, each line item's "commerce_unit_price" field, and resaving them and then orders (to update order total field data). I could do more, but this should be sufficient given current functionality and usage.

Obviously since there are enormous sites out there, these updates should be run from the command line if you have a lot of products, line items, or orders. I have it batching 50 at a time, so on Eurocentres with its 175k products it should only require 3500 iterations. ; )

Messing around with #process was a no-go. We can always file a separate issue to fix this for 2.x if there's something I'm missing (which there surely is).

rszrama’s picture

The attached version forces field_cache_clear() at the start of each of our update functions to ensure no stale price data is cached from the prior method of handling price amounts. HT: rfay for turning it up.

rszrama’s picture

Status: Needs review » Fixed

After a few IRC reviews and a battery of update tests on my end, this one is ready to roll. Committed!

I updated the dev release notes to include the following text:

"As of 4-19-11, you should run update.php after updating to the latest dev release. Sites with large amounts of products, line items, or orders should read the update notes in commerce_order.install above the function commerce_order_update_7100()."

pcambra’s picture

I think that in commerce_cart_order_refresh the cloned order is not saved anymore so maybe this legacy comment could be a little confusing?

  if ($order_wrapper->value() != $original_order || $line_item_changed) {
    // Use a clone so the save doesn't interfere with price values in a live
    // order object that would be modified on presave.
    commerce_order_save($order_wrapper->value());
  }
aidanlis’s picture

That's a huge commit! Well done everyone.

rfay’s picture

Status: Fixed » Needs review
FileSize
609 bytes

Yay!

I did re-test this morning and it was fine. Thanks!

Here is (I think) pcambra's #25. It's possible we still have some clones laying around that were paranoid responses to the various problems before this wonderful patch.

rszrama’s picture

Status: Needs review » Fixed

Got it. Will commit that with the next commit.

moshe weitzman’s picture

Status: Fixed » Needs work

Just dropping by and perused the patch ...

+++ b/modules/order/commerce_order.install
@@ -180,3 +180,171 @@ function commerce_order_schema() {
+  db_query("TRUNCATE TABLE {commerce_calculated_price}");

we have x-platform code for this: http://api.drupal.org/api/drupal/includes--database--database.inc/functi...

+++ b/modules/payment/includes/commerce_payment.forms.inc
@@ -29,6 +29,7 @@ function commerce_payment_order_transaction_add_form($form, &$form_state, $order
+      '#element_validate' => array('commerce_payment_order_transaction_add_form_payment_terminal_validate'),

nice function name :)

rszrama’s picture

Status: Needs work » Needs review
FileSize
1.3 KB

lol We do have some doozies. : P

Thanks for the review; obviously didn't know we had a handy little function. Committed an fix to the update function and an admin include. There shouldn't be anything I have to do regarding the existing update function, is there?

rszrama’s picture

Status: Needs review » Fixed

I'm going to move this one back to fixed w/ the patch in and no further comments. Let's keep an eye on it, though, in case there are other missed API functions or regressions surface.

Status: Fixed » Closed (fixed)

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

madurax86’s picture

Version: 7.x-1.x-dev » 7.x-1.1
Component: Developer experience » User experience
Category: task » bug
Priority: Normal » Critical
Status: Closed (fixed) » Active

Its again there with 7x-1.1 version all amounts are displayed x100!! When the prices are displayed as raw amounts its always x100, i.e. $100.00 is 10000. This makes the price comparison useless to be used with DC. This may affect other areas too, I have only seen this one.

bojanz’s picture

Version: 7.x-1.1 » 7.x-1.x-dev
Component: User experience » Developer experience
Category: bug » task
Priority: Critical » Normal
Status: Active » Closed (fixed)

Raw amount is supposed to be x100 for dollars, because that's how the amount is stored in the DB.

rszrama’s picture

Yeah, there's an open issue for supplying UI components for Rules and Views that will properly handle currencies, but note that having to use the database amount (cents instead of dollars or euros) hardly makes the system useless. Math works just as fine on integers as it would on decimals so long as you know you're comparing cents to cents instead of dollars to dollars. And now you do. : )

madurax86’s picture

Component: Developer experience » User experience
Status: Closed (fixed) » Active

The bug came with me trying to search products in a price range using views, its better to leave it is if its intended but isn't there a simple way to just return true values when compared, I mean if this is the case of storing values as floats, why don't you guys store cents and dollars/rupees in two different columns? (Just my opinion). Otherwise isn't there anyway that I can use a view for this?

rszrama’s picture

Status: Active » Closed (fixed)

I'm pretty sure I told you on another issue that the solution here involves fixing this in the UI for Views and Rules, not changing the schema of the price field. The price field is and will continue to store price amounts as integers, no point reviving a closed issue.

Here's one related issue: #1219762: Add an exposed filter handler for price amounts. I've already given you another pertaining to Rules. We can implement custom filter handlers so Views works with currency in major units instead of minor units.

madurax86’s picture

Oh sorry, I did not find thath issue. I'll try the patch listed there.

WaveYouAt’s picture

Can this be the reason why I cannot create a rule to generate a new Commerce product anytime I create a different entity using tokens for that entity on the the price field in new rule pane?

I have installed Shopify API and Shopify module so I have my webhooks that keep my Shopify Products updated in Drupal. I'm facing this Limbo now. Should I style the Shopify products binding them in my panels, views and CSS classes? I will lose the power of Commerce...

Can I accomplish to create a new product and product display in commerce using shopify entity tokens? isn'this resource exausting...

Should I consider editing the PHP in the shopify module in order to populate commerce instead?

Please any help.