Magento2: LocalizedException is catched and overwritten with a generalized one in placeOrder

[rzinnatullin]

Preconditions

1. Magento 2.1.6
2. Php 7
3. Ubuntu 16.04

Description

I'm creating a module that is subscribed to 'checkout_submit_before' event. In the event handler I'm doing some address validations and in case of issues I'm throwing LocalizedException exception.
However, my exception is catched and overwritten with CouldNotSaveException exception:

As the result, user sees that generalized exception:

I suppose that the catch block should rethrow LocalizedException. There are a lot of places inside placeOrder where LocalizedException could be thrown:

Note that 'Please check the shipping address information' wouldn't be displayed to the user.

If I'm doing this wrong, can you please suggest a good way to show validation errors to the user.

Expected result

I did very simple fix to get what I want:

After that I'm getting exactly the behaviour I wanted:

Any thought?

[joni-jones]

Hi, @rzinnatullin, Magento provides the only general error message for exceptions. On the one hand, it allows making an application more secure on the other hand customer can't see useful error details like incorrect address or credit card number.

If I'm doing this wrong, can you please suggest a good way to show validation errors to the user.

You can add own implementation of \Magento\Checkout\Api\PaymentInformationManagementInterface and \Magento\Checkout\Api\GuestPaymentInformationManagementInterface, override preference for them and show error messages at your own risk.

This issue the same as https://github.com/magento/magento2/issues/9003.

[hostep]

I think this duplicates https://github.com/magento/magento2/issues/6795 and https://github.com/magento/magento2/issues/6246
In the second one, you can find a commit where they have it already fixed on the develop branch, but I don't think it's already backported to 2.1.x

[joni-jones]

@hostep, the develop branch does not have any fixes for this behavior only logging of original exception was added.

[hostep]

@joni-jones: Ok thanks for the clarification, good to know!

[rzinnatullin]

Hi @joni-jones,

You can add own implementation of \Magento\Checkout\Api\PaymentInformationManagementInterface and \Magento\Checkout\Api\GuestPaymentInformationManagementInterface, override preference for them and show error messages at your own risk.

Is there any other way to do Address validation during Checkout? I don't want to replace Magento's functionality, I want to extend it.

[joni-jones]

You can try to use plugins for savePaymentInformationAndPlaceOrder to analyze original exception and process customizations.

[rzinnatullin]

Thanks @joni-jones, I've got your idea.

Regarding the original issue - as I understand Magento 2 developers are not going to do something similar to what I did (rethrow LocalizedException)? I'm asking because I thought that LocalizedException has been introduced specifically to add an ability to send a error message to frontend (a user).

[piotrekkaminski]

It is a kind of a bug - affects many developers and increases debugging time.

[scottsb]

Thanks @piotrekkaminski!

[joni-jones]

@piotrekkaminski, the current implementation rethrows an original LocalizedException and logs all \Exceptions, it allows showing safe error messages and logging all other, so customers would see errors like validation, etc. and developers can get an original exception from the exception log.

[scottsb]

@joni-jones Hmm, that does seem to be reasonable behavior. I think @hostep and myself were thrown off by this comment you had made earlier:

@hostep, the develop branch does not have any fixes for this behavior only logging of original exception was added.

I guess there was more than logging added: as it is, a new exception is throw, but with the original message. That will fix this issue in most cases for safe messages.

The one side case which was reported in #9003 is that Braintree validation messages will sometimes reach savePaymentInformationAndPlaceOrder() as a Braintree SDK exception class (Braintree_Exception) rather than a LocalizedException. Given the change you point out above, would the correct fix be to update the Magento_Braintree module to make sure that any Braintree SDK exception are caught & rethrown as a LocalizedException when safe to do so?

[joni-jones]

@scottsb, about my comment:

@hostep, the develop branch does not have any fixes for this behavior only logging of original exception was added.

I meant, the general behavior wasn't reworked and original errors are still not available for customers but an original exception can be found in the exception log, sorry if it was not clear.

Given the change you point out above, would the correct fix be to update the Magento_Braintree module to make sure that any Braintree SDK exception are caught & rethrown as a LocalizedException when safe to do so?

If you open Braintree errors codes list, you will find what a lot of codes should not be shown for a customer because containing sensitive or integration-specific information.

Another point, the current Payment Gateway infrastructure designed to catch all validation failures and throws a general exception. This behavior could be reworked and Magento could "trust" built-in payment integrations, which would return only "safe" errors, but there a lot of payment extensions and in that case, it could be a place for potential vulnerabilities.

[scottsb]

Yes, I understand you wouldn't want to pass all Braintree executions through. I will to try to reproduce again, but I believe there were Braintree validation exceptions that weren't intercepted with a CommandException.

[magento-engcom-team]

@rzinnatullin, thank you for your report.
We've created internal ticket(s) MAGETWO-84248 to track progress on the issue.

[joni-jones]

Added customization point in 2.2.4 release.