mollie / PrestaShop

iDEAL, Creditcard, Bancontact, SOFORT, Bank transfer, PayPal & paysafecard for Prestashop
http://www.mollie.com
BSD 2-Clause "Simplified" License
68 stars 41 forks source link

Wrong HTTP status (1) on failure to create order #894

Open mariuszsienkiewicz opened 4 months ago

mariuszsienkiewicz commented 4 months ago

What I have found?

I noticed unusual behavior when selecting Klarna Pay Later (and probably other payments): if there is an error in the data passed to the API (such as a too short family family name of the customer), NGINX will return 502 Bad Gateway error in the payment.php controller.

I found this line in error logs:

<redacted> [error] 3223#0: *37279672 upstream sent invalid status "1" while reading response header from upstream, client: <redacted>, server: <redacted>, request: "GET /module/mollie/payment?method=klarnapaylater&rand=<redacted> HTTP/2.0"

Reason

The payment.php controller utilizes MollieOrderCreationService. When an error occurs in the createMollieOrder method (such as: Error executing API call (422: Unprocessable Entity): The 'billingAddress.familyName' field should have more than 1 character.), it is handled by ErrorHandler's handle method:

https://github.com/mollie/PrestaShop/blob/1d60186fba42433f0a02eb6ca8c0485365b6cd3e/src/Service/MollieOrderCreationService.php#L67-L69

The handle method sets HTTP code relative to the code given in the Exception:

https://github.com/mollie/PrestaShop/blob/1d60186fba42433f0a02eb6ca8c0485365b6cd3e/src/Handler/ErrorHandler/ErrorHandler.php#L107

For example, OrderCreationException will have $code equal to 1:

Mollie\Exception\OrderCreationException Object
(
    [message:protected] => '[<redacted>] Error executing API call (422: Unprocessable Entity): The 'billingAddress.familyName' field should have more than 1 character.'
    [code:protected] => 1
)

The issue arises because this HTTP code is later used by the PrestaShop controller without being altered to a proper HTTP code. As a result, we receive an HTTP code that is not in the RFC, so NGINX treats this situation as an error in the PHP code and returns a 502 Bad Gateway because the upstream provided an incorrect response code.

In the result our customers see error 502 Bad Gateway instead of being informed that there was an error in data passed to Mollie API.

justelis22 commented 4 months ago

Hi there @mariuszsienkiewicz,

Thank you for a detailed explanation of the issue. I have forwarded the information to our development team and as soon as we have some additional information, I will get back to you!

Best Regards, Invertus Support team.

mariuszsienkiewicz commented 4 months ago

@justelis22 Could you please provide any updates on this topic?

justelis22 commented 4 months ago

Hi @mariuszsienkiewicz,

The improvement will be added to the next release 6.1.1. It is planned to be done in the next couple of weeks. I would suggest following releases here on GitHub.

The beta should be available next week.

Best Regards, Invertus Support team.

justelis22 commented 3 months ago

@mariuszsienkiewicz Hi there,

Release 6.1.1 is out and the fix was included.

I will be waiting for your feedback!

Best Regards, Invertus Support team.

mariuszsienkiewicz commented 3 months ago

Thank you for the provided fix @justelis22.

The problem is that the customer still does not know what the issue is, now the only thing showed by the mollie module is:

"Unknown exception in Mollie"

Wouldn't it be better to implement more non-technical message for the customer? I know it should be implemented here:

https://github.com/mollie/PrestaShop/blob/master/src/Service/ExceptionService.php

Even just a generic message to alert the customer that they may have entered something incorrectly in the address? This error occurs even when the customer enters a single character in the family name - I know the probability of a family name consisting of a single letter is very low.

I don't want to open a new issue so please open this one again - I think that there is still a problem that has to be in some way fixed.

justelis22 commented 3 months ago

Hi @mariuszsienkiewicz,

Thank you for the additional information.

I'll create a task for our team to improve the error messages. We have the upcoming 6.2.0 and 6.2.1 releases already planned, so this most likely will be investigated a bit later on.

Best Regards, Invertus Support team.