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.

Comments

Raphael Dürst’s picture

Status: Active » Needs review
StatusFileSize
new3.58 KB

As promised, the patch to fix this issue.

juliangb’s picture

Version: 6.x-2.0 » 6.x-2.x-dev

Any 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.

juliangb’s picture

kfritsche’s picture

In 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 ;)

juliangb’s picture

Status: Needs review » Needs work

Agreed, 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

Raphael Dürst’s picture

Project: Storm » Drupal PM (Project Management)
Version: 6.x-2.x-dev » 7.x-1.x-dev
Component: Storm Invoice » Code

This 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.

juliangb’s picture

I'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.

Raphael Dürst’s picture

I think, for consistency, we should do this for all pm content types and not only for pminvoice.
What are your thoughts?

juliangb’s picture

In 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).

Raphael Dürst’s picture

You'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.

juliangb’s picture

Status: Needs work » Postponed

Right - 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.

juliangb’s picture

Status: Postponed » Needs review
StatusFileSize
new3.98 KB

I 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).

juliangb’s picture

Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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

  • Commit 66cb0bb on 7.x-1.x, 7.x-2.x by juliangb:
    Issue #1804270 by Raphael Durst, juliangb: Fixed MySQL datatype (FLOAT)...