digitick / php-sepa-xml

* THIS PROJECT IS NO LONGER MAINTAINED* SEPA file generator in PHP
107 stars 63 forks source link

The "ControlSum" and the "InstructedAmount" value miscount #28

Open ghost opened 10 years ago

ghost commented 10 years ago

By using your example i added the following amount for a transfer:

'amount' => 19.99

In the XML output i get following output:

19.98

and:

19.98

I miss 0.01 Cent ;)

monofone commented 10 years ago

Yeah you a right, I pushed a commit to my fork with a test examining the failure. While fixing this I stumbled upon another issue. If you provide 19.999 as amount this will sum up to 20.00 at the moment. I´m not sure about how to handle this. I think it not the responsibility of a xml writer to round your amount. But on the other hand this lib is already calculating the sums of the whole Transfer which can´t be done without bringing the numbers to a common base.

Any Ideas ?

monofone commented 10 years ago

Today discussed at worked the case of casting floats. Its terrible;

There is something internally happen when casting from string to double that is not happening when casting from integer to double.

php > $d = '19.99' * 100; php > $d1 = (double)1999; php > var_dump($d); double(1999) php > var_dump($d1); double(1999) php > var_dump((integer)$d); int(1998) php > var_dump((integer)$d1); int(1999)

The following is explaining this a little.
php > printf('%10.20f', $d); 1998.99999999999977262632 php > printf('%10.20f', $d1); 1999.00000000000000000000

I think a possible solution is to only allow cent amounts and only in the xml generation a conversion to a float representation is done.

My experience with calculating prices is to always use cent amounts and only when displaying prices convert them to float representations.

So my idea is to only accept cent amounts as this is done internally already there should be no problem in the internal calculation.

markuswolff commented 10 years ago

I have not yet had a chance to look at the actual code, but just to throw in an idea, another way could be to use the bcmath extension when available. It allows precision math operations on values that are converted from strings: http://de1.php.net/manual/de/function.bcadd.php

ghost commented 10 years ago

Yes, the right way is to use bcmath Methods: http://www.php.net/manual/en/ref.bc.php

monofone commented 10 years ago

Isnt it more stable to only use cents then its not the responsibility of this library to calculate correctly as this should be done in the software using this library.

ghost commented 10 years ago

For me this litle changes are working:

File: vendor/digitick/sepa-xml/lib/Digitick/Sepa/TransferInformation/BaseTransferInformation.php

Line 83 and up:

/**
 * @param mixed $amount
 * @param string $iban
 * @param string $name
 */
public function __construct($amount, $iban, $name)
{
    bcscale(2);
    $this->transferAmount = bcadd($this->transferAmount, bcmul($amount, '100'));
    $this->iban = $iban;
    $this->name = StringHelper::sanitizeString($name);
}
monofone commented 10 years ago

@ianare Hey, I think this can be closed, right ?

markuswolff commented 10 years ago

Not sure if it can be closed - I just encountered the issue again using the latest trunk on PHP 5.3.3/Gentoo, one cent is missing in a transaction of originally 65.59€ (control sum is calculated correctly).

I was unable to reproduce this issue on PHP 5.4/Mac so far, so at the moment I'm guessing it's a bug in PHP 5.3.3, but this could just as well mean that float conversion is simply unreliable depending on either PHP version or platform, and all calculations should always be done in bcmath or using integers.

Regarding the current bcmath handling, currently there's a check if a parameter is passed as float and then bcmath will be used. Technically, this is incorrect, as bcmath accepts strings as input - so a type conversion will take place before the value is passed to bcmath. Worst case the type is cast twice (comes from data store as string, cast to float to activate bcmath processing in php-sepa-xml, cast back to string to pass on to bcmath).

I'll try and pinpoint the problem further.

Patafix commented 10 years ago

I think there is a coherence probleme with the way you fix the problem.

If i'm not wrong this code means the amount is in cents, if its a int, and if it's float it's not in cents ? That right ? Correct me if i'm wrong ?

I have to use the script in production and i'm not confident to use it this way, i use int amount in cents (beacause no bcmath on my php conf)

monofone commented 10 years ago

Hi @Patafix, as mentioned in my second comment I think the solution is to only allow cents. But other thought it would be better to also allow cent amounts. I understand your concerns about using it in production. If you have a good Idea or a patch don't hesitate to send a pull request or a comment here.

Patafix commented 10 years ago

I don't hesitate, i just want to check if i have understood correctly how the amount work (int => CENTS, FLOAT => NO CENTS) before use it. Thank for you work. I think your solution of allow only cent is the good one, personnaly i send the amount only in cents.