How to deal with integer limits?

[mnapoli]

Hi, we have to deal with Money and store it in database. In that process we came up with a few issues that I'd like to discuss so that maybe we can find "best practices" on how to deal with all that.

First there's the problem of integer limits in memory. On a 32bits system the amount will be stored as an integer up to something like 21 million $/€/… (because it's stored as cents). That's not much and can be a real problem in some applications, I wonder if that should be mentioned in the README? Is the Money class only appropriate for 64b systems (where the limit is much less of a problem for most cases)? Should the amount be stored differently on 32b? (for our team it won't be a problem because we use only 64 bits systems)

Then there's the question of how to store it in database. For example in MySQL you can use the integer type which has the same limit as a 32b PHP integer. So obviously it's much better to use BIGINT which is the equivalent of a 64b integer.

However if you use Doctrine (quite popular in the PHP world), the entity manager will map a SQL BIGINT to a PHP string in order to be compatible with 32b systems… So Money will end up with a string instead of an int as the amount, which is an issue I guess because the class expects to have an int there (I expect things to break/not behave correctly). So we have several options, like storing the whole Money object serialized (we see that the class even implements Serializable in the next version) but it comes with a lot of other issues, creating custom Doctrine types to map SQL BIGINT to actual PHP integers (i.e. it assumes a 64b system is used), store the object as a string (e.g. 12 EUR), etc. Is there some kind of best practices for that? Obviously storing as integer/bigint is great because it allows filtering, ordering, etc in SQL queries, but is that a big "no no" (just like storing money in float is a big "no no")?

[camspiers]

You are right in everything you say. Unrestricted integer support has been discussed before and would be possible with something like bcmath (#7), but it just hasn't been implemented.

I think that we should at least add a clear message in the README that money sizes are limited by integer size in your environment (32bit vs 64bit), and if you are using a database the same holds true for that.

There are also some other things to be aware of, if you look over at the @sebastianbergmann implementation of Money, I added a bunch of failure states when arithmetic on money would cause loss of precision (and sometimes completely crazy values).

https://github.com/sebastianbergmann/money/commits?author=camspiers

These changes should probably be ported over to this library too. These problems become particularly pernicious when dealing in currencies like bitcoin, but they are also a huge problem if people don't validate ecommerce inputs well.

To give an obvious scenario, imagine a product worth $5000, on a 32 bit system, adding 500000 of these items to your cart will cause your cart to have a negative total, WTF, let's just hope that people have their systems setup well to catch this stuff.

https://github.com/mathiasverraes/money/blob/master/lib/Money/Money.php#L216

[mnapoli]

Thank you for the great answer! Those check you implemented on Sebastian Bergmann's implementation are indeed pretty great. We ended up rolling our own implementation 100% based on that implementation, minus the currency (we don't have to deal with currencies).

For the database I map PHP's integers to BIGINT. At any point (from DB, to DB, etc.) if there's an int overflow there will be an exception, so at least there won't be any silent data corruption/wtf.

It would indeed be good to implement such checks in this library too.

[camspiers]

You're welcome. I don't have bandwidth at the moment, but I am fairly confident that were you to provide a PR @mathiasverraes would be keen to add the changes. Good luck.

[frederikbosch]

Recently I saw this being discussed on Twitter too. I believe the overall conclusion is that best thing to use would be GMP, if it is not available on the system switch to BC Math. Other libraries seem to prioritize this way too.

However, there might also be reasons for using BC Math over GMP because GMP only deals with integers, and not with decimals (there is a proposal for the GMP implementation). For another money library this was a reason to choose BC Math over GMP.

Taking into account the feature state of this library, I would say that best thing to prioritize as follows: GMP, BC Math and finally the (current) native PHP method. I am happy to implement and push a PR soon.

[mathiasverraes]

Please do :)
On 25 Nov 2015 10:38, "Frederik Bosch" [email protected] wrote:

Recently I saw this being discussed on Twitter too. I believe the overall
conclusion is that best thing to use would be GMP
http://nl3.php.net/manual/en/book.gmp.php, if it is not available on
the system switch to BC Math http://nl3.php.net/manual/en/book.bc.php.
Other ircmaxell/PHP-PasswordLib#10 libraries
seem to prioritize this way too.

However, there might also be reasons for using BC Math over GMP because
GMP only deals with integers, and not with decimals (there is a proposal
https://wiki.php.net/rfc/gmp-floating-point for the GMP
implementation). For another money library this was a reason to choose BC
Math over GMP https://github.com/ulabox/money.

Taking into account the feature state of this library, I would say that
best thing to prioritize as follows: GMP, BC Math and finally the (current)
native PHP method. I am happy to implement and push a PR soon.

—
Reply to this email directly or view it on GitHub
#111 (comment)
.

[sagikazarmark]

Should we have a separate class for this (like the BigMoney idea)?

[frederikbosch]

Not in my opinion. I would rather build this in. Why would you want to consume another api for larger values? Who knows when to use BigMoney? It would lead to unexpected results in the Money class, because you would have throw exceptions when you cannot calculate a result because the resulting or initial value is too large. The developer does not know when that is happening in developing time. How would a new BigMoney class interoperate with a Money class?

My idea was to pick a strategy in the constructor. Something like.

$calculators = [new GmpCalculator(), new BcMathCalculator(), new PhpCalculator()];
foreach ($calculators as $calculator) {
  if ($calculator->isSupported()) {

  }
}

[mathiasverraes]

BigMoney is something different: it means money with unlimited division.
You may want to work with something smaller than 1 cent, and round only
when you make an actual payment. That is fundamentally different behaviour
from Money, which always works with the smallest denomination.

That said, I agree the name BigMoney is confusing. I took from the java
implementation but PreciseMoney is probably a better name.

Both Money and PreciseMoney should be implemented with the better math
libs. Please PR against the nextrelease branch.

—
Mathias Verraes
Software Consultant
Value Object Comm.V http://value-object.com
Blog http://verraes.net/ - Email [email protected] - Twitter
http://twitter.com/mathiasverraes - LinkedIn
https://www.linkedin.com/in/mathiasverraes

On 25 November 2015 at 12:43, Frederik Bosch [email protected]
wrote:

Not in my opinion. I would rather build this in. Why would you want to
consume another api for larger values? Who knows when to use BigMoney? It
would lead to unexpected results in the Money class, because you would have
throw exceptions when you cannot calculate a result because the resulting
or initial value is too large. The developer does not know when that is
happening in developing time. How would a new BigMoney class interoperate
with a Money class?

My idea was to pick a strategy in the constructor. Something like.

$calculators = [new GmpCalculator(), new BcMathCalculator(), new PhpCalculator()];foreach ($calculators as $calculator) { if ($calculator->isSupported()) { }}

—
Reply to this email directly or view it on GitHub
#111 (comment)
.

[frederikbosch]

Just created a PR for this feature.

[frederikbosch]

This one can be closed.

[sagikazarmark]

Fixed in #115