We multiply the amount by 100 regardless of how many fraction digits a currency has.

Same bug as the one in Stripe: #2913605: Yen currency is processed incorrectly (100x the input value). Once that one is fixed, the fix can probably be copied here.

CommentFileSizeAuthor
#24 drupal-commerce_ingenico-use_tominorunits_in_operations_trait-2928926-23.patch1.56 KBpstewart
#6 2928926-6.patch1.58 KBflocondetoile
#3 2928926-3.patch1.6 KBflocondetoile

Comments

bojanz created an issue. See original summary.

bojanz’s picture

bojanz commented

Commerce 2.2 now has a $this->toMinorUnits() helper on the payment gateway base, let's use it.
Reference: #2928972: Add PaymentGatewayBase::toMinorUnits().

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Status: Active » Needs review
StatusFileSize
new 2928926-3.patch1.6 KB

Attached the related tiny patch.

bojanz’s picture

bojanz commented
Status: Needs review » Needs work 
-    $directLinkRequest->setAmount((int) ($payment->getAmount()->getNumber() * 100));
+    $directLinkRequest->setAmount((int) $this->toMinorUnits($payment->getAmount()));

The method already returns an int, the cast is unnecessary.

-    $ecommercePaymentRequest->setAmount((int) ($payment->getAmount()->getNumber() * 100));
+    $amount = $payment_gateway_plugin->toMinorUnits($payment->getAmount());
+    $ecommercePaymentRequest->setAmount((int) $amount);

toMinorUnits() is a protected function, so this can only crash. We'll need to patch Commerce to make it public: #2944281: PaymentGatewayBase::toMinorUnits() should be a public method.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

toMinorUnits() is a protected function, so this can only crash. We'll need to patch Commerce to make it public

Yes, I saw this after but didn't find time to report/fix it. Deadline was very short. I will update the patch.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Status: Needs work » Needs review
StatusFileSize
new 2928926-6.patch1.58 KB

Patch updated.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Status: Needs review » Needs work

Tested #6.
Got an error when submitting a payment

InvalidArgumentException: Integer expected. Amount is always in cents in Ogone\AbstractPaymentRequest->setAmount() (line 160 of .../vendor/marlon-be/marlon-ogone/lib/Ogone/AbstractPaymentRequest.php).

Looks like the cast (int) is necessary

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

Seems that toMinorUnits return à float value (the round method()) and not an integer.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Status: Needs work » Needs review

Tested #3 with patch from #2944281: PaymentGatewayBase::toMinorUnits() should be a public method.
Works fine.

  • bojanz committed 56386fa on 8.x-1.x authored by flocondetoile 
    Issue #2928926 by flocondetoile: Incorrect amount set when the currency...
bojanz’s picture

bojanz commented

Won't have time to commit the Commerce patch in the next day or two, committed the first portion of the fix:

diff --git a/src/Plugin/Commerce/PaymentGateway/DirectLink.php b/src/Plugin/Commerce/PaymentGateway/DirectLink.php
index 35f07f309..12f739f23 100644
--- a/src/Plugin/Commerce/PaymentGateway/DirectLink.php
+++ b/src/Plugin/Commerce/PaymentGateway/DirectLink.php
@@ -260,7 +260,7 @@ public function createPayment(PaymentInterface $payment, $capture = TRUE) {
       'PAYMENT_ID' => $payment->id(),
     ]);
     // Ingenico requires the AMOUNT value to be sent in decimals.
-    $directLinkRequest->setAmount((int) ($payment->getAmount()->getNumber() * 100));
+    $directLinkRequest->setAmount($this->toMinorUnits($payment->getAmount()));
     $directLinkRequest->setCurrency($payment->getAmount()->getCurrencyCode());
     $directLinkRequest->setLanguage($this->configuration['language']);

This at least reduces the bug for people.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

Hi Bojanz,

As reported in #7, the cast (int) seems necessary in fact. Even if the round method should return an integer because of the precision 0, the value returned is typed as a float (or a string ?) and the AbstractPaymentRequest->setAmount() method check this value with the is_int() method which expect strictly an integer (https://github.com/marlon-be/marlon-ogone/blob/eefe735d7113a3ce8023d7c0a...).

The patch #3 is the patch to use. Not #6

  • bojanz committed 7e0e868 on 8.x-1.x 
    Revert "Issue #2928926 by flocondetoile: Incorrect amount set when the...
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Status: Needs review » Postponed

Postpone the issue until #2944281: PaymentGatewayBase::toMinorUnits() should be a public method is committed.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Related issues: +#2944281: PaymentGatewayBase::toMinorUnits() should be a public method
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

Once this patch committed, I would release a first beta.

eric_a’s picture

eric_a commented
Status: Postponed » Needs work

So #2944281: PaymentGatewayBase::toMinorUnits() should be a public method was fixed in October and is in Commerce 2.15 (released 6 November 2019).

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

Yes I want first release a first beta without this patch. And then commit it, release a new beta with a dependency on commerce ^2.15. But we have currently others (greater) issues to fix.

  • flocondetoile authored add59a1 on 8.x-1.x 
    Issue #2928926 by flocondetoile, bojanz: Incorrect amount set when the...
flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented
Status: Needs work » Fixed

A first beta release follow

Status: Fixed » Closed (fixed)

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

pstewart’s picture

pstewart commented

This issue needs to reopen as #6 only fixes payments amounts set during checkout in ECommerceOffsiteForm and DirectLink::createPayment - it misses other operations such as post-checkout payment capturing and refunds in OperationsTrait. It also notewortht that the int cast is also actively harmful in the unmodified code due to the way php rounds in a cast (e.g. php -r 'print (int)(39.98 * 100);'), however is safe when toMinorUnits is used as the output of round() is reliable.

I need to put a fix for this into production as soon as I can so will prepare a patch against current dev.

flocondetoile’s picture

flocondetoile
Primary language French
Location Lyon
commented

Please create a follow up. This issue is closed.

pstewart’s picture

pstewart commented
StatusFileSize
new drupal-commerce_ingenico-use_tominorunits_in_operations_trait-2928926-23.patch1.56 KB

See attached patch.

pstewart’s picture

pstewart commented

Sorry, didn't see your post before uploading. Will open new issue now

pstewart’s picture

pstewart commented

Hiding patch from #24 as this has now been resubmitted to #3192390