If I want to save an invoice with prices higher than 10000, the datatype FLOAT is not precise enough to save values with 2 decimal digits.
For instance, if the price is 12345.95, it gets rounded to 12346.
And if the price is 123456.70, it gets rounded to 123457.
If the datatype for all price fields is set to DOUBLE, this won't be a problem.
I will work on a fix personally and publish the patch as soon as it's done.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | pm-invoice-fixedprecision-1.patch | 3.98 KB | juliangb |
| #1 | storm-prices-float-to-double-1804270-0.patch | 3.58 KB | Raphael Dürst |
Comments
Comment #1
Raphael Dürst commentedAs promised, the patch to fix this issue.
Comment #2
juliangb commentedAny thoughts on whether a fixed precision data type would actually be better?
That'd remove any potential for this problem again at any scale.
Retesting against 2.x-dev since there have been a few changes since 2.0.
Comment #3
juliangb commented#1: storm-prices-float-to-double-1804270-0.patch queued for re-testing.
Comment #4
kfritscheIn my mind, the correct way would be something like "DECIMAL(20,4)". dunno the drupal syntax for this.
If we fix this, we should fix it right. Otherwise somebody else will complain. float and double are imprecise and we are talking about prices here, this should be precise. There it should be NUMERIC or DECIMAL. On the other hand php only uses double (i think). So after reading it from the DB it get imprecise. But at least we could save the correct value ;)
If I remember correctly double get imprecise around 15 digits? So around at a price of a trillion dollar? Okay, maybe we can let it at double ;)
Comment #5
juliangb commentedAgreed, if we're going to change, lets change to fixed precision.
This page may be helpful (although targeted towards D7 now - same principles should apply for D6) - https://drupal.org/node/159605
Comment #6
Raphael Dürst commentedThis was not yet fixed in PM for D7 and I think, it's pretty important that we do this.
We can backport it to Storm, when it's implemented in PM.
I agree, that we should do this with a fixed precision instead of FLOAT.
Comment #7
juliangb commentedI've added this to the roadmap.
#1862286: Meta: Development Roadmap - 7.x-2.x version
We should definitely get this fixed, but doesn't need to hold up an alpha release.
Comment #8
Raphael Dürst commentedI think, for consistency, we should do this for all pm content types and not only for pminvoice.
What are your thoughts?
Comment #9
juliangb commentedIn principle yes, but let's not lose touch with our plan to move all fields to field api. (I'd like our time to be as productive as possible).
Comment #10
Raphael Dürst commentedYou're right. I guess, it makes more sense to directly use fields with proper date types and don't change the current data types for now.
Comment #11
juliangb commentedRight - let's postpone this for now to focus on the transition to Field API.
Before a beta is released, let's review that the issue has been solved.
Comment #12
juliangb commentedI wanted to get a few issues ticked off, and whilst these fields will be replaced by Field API, it would be worth getting a fix in for the time being.
See this page for currency data types: http://stackoverflow.com/questions/628637/best-data-type-for-currency
Patch attached (fixed precision, decimal 19, 4).
Comment #13
juliangb commentedCommitted.