search

moneyphp / money

PHP implementation of Fowler's Money pattern.
http://moneyphp.org
MIT License
4.63k stars 441 forks source link

Add php-decimal calculator #516

Closed rtheunissen closed 3 years ago

rtheunissen commented 5 years ago

Hi everyone, this patch adds support for the new php decimal extension.

It's a real shame how the calculators are set up, because it allows float to used (thereby opening the door to poor precision) and requires that the return value of each operation be string. The decimal extension uses an optimized internal value that is not a string, so the conversion to and from string for every operation adds unnecessary overhead.

What I'd love to see is a PHP 7 update of this library where input is normalized (string or int, ideally) before being passed to the calculator, and to not require that the return result of the calculators be string.

This implementation passes the tests though, so it'll have to do for the time being.

Please see http://php-decimal.io for more information.

frederikbosch commented 5 years ago

@rtheunissen Nice work over there with the extension, I like it. I was not aware of it.

@sagikazarmark I am in favour of merging this PR in the 3.x range. And I also think @rtheunissen is right regarding risks of float usage. We should take care of that in the 4.x releases.

sagikazarmark commented 4 years ago

Sorry for the late answer, time has passed quickly. I'm definitely in favor of moving to PHP 7.3+ and improving internal semantics.

One comment related to this particular PR: we have moved to GitHub Actions recently, so the CI part of the PR needs to be actualized. Other than that 👍

Can you update the PR @rtheunissen ?

sagikazarmark commented 4 years ago

and to not require that the return result of the calculators be string.

What should the result be then?

rtheunissen commented 4 years ago

and to not require that the return result of the calculators be string.

What should the result be then?

An abstract number type. The decimal extension does not use strings internally, so to convert to and from string for every operation is wasteful. Instead, we can create an interface like Number that supports whatever operations we need, and leave it up to the implementation to store the internal data however it wants. BC and any other string-based calculator could use a generic StringNumber, for example.

sagikazarmark commented 4 years ago

Would this be an interface between Money and calculators? Or an internal representation that Money uses as well? (Currently `Money uses string internally)

frederikbosch commented 3 years ago

Since we want to keep maintenance for this library not too high, I would recommend publishing this in a separate repo. I am very sorry for the delay.