Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ghost opened this issue Jan 8, 2014 · 11 comments
Open

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

ghost opened this issue Jan 8, 2014 · 11 comments

Comments

@ghost
Copy link

ghost commented Jan 8, 2014

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
Copy link
Contributor

monofone commented Jan 8, 2014

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
Copy link
Contributor

monofone commented Jan 9, 2014

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
Copy link

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
Copy link
Author

ghost commented Jan 9, 2014

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

@monofone
Copy link
Contributor

monofone commented Jan 9, 2014

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
Copy link
Author

ghost commented Jan 9, 2014

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 referenced this issue in php-sepa-xml/php-sepa-xml Jan 11, 2014
Added test for bcmath in case the provided amount is a float
ianare added a commit that referenced this issue Jan 13, 2014
changed handling of transfer amounts as this lead to an error #28
@monofone
Copy link
Contributor

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

@markuswolff
Copy link

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.

capocasa pushed a commit to Altruja/php-sepa-xml that referenced this issue Apr 15, 2014
…k#28

Added test for bcmath in case the provided amount is a float

Conflicts:
	lib/Digitick/Sepa/TransferInformation/BaseTransferInformation.php
capocasa pushed a commit to Altruja/php-sepa-xml that referenced this issue Apr 15, 2014
@Patafix
Copy link

Patafix commented Aug 25, 2014

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
Copy link
Contributor

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
Copy link

Patafix commented Aug 25, 2014

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants