Print details from error messages returned by paypal

Let's first wait for Centarro feedback, what they think, before investing time here.

Here is a list of all available issue descriptions:
https://github.com/paypal/paypal-js/blob/a2e716e0a0982fe79c8c51e8c04e187... (note, that 422 is UNPROCESSABLE_ENTITY).

IMO, the error message descriptions are fairly safe in an admin context.

Of course we have to be careful to not publish sensitive information and should have a look at PayPals own implementations, in which cases they espose these messages.

There are no user information in these error descriptions.

Interestingly, I can't even refund a payment on latest dev release.

I get the following error:

Error: Call to a member function generate() on null in Drupal\commerce_paypal\Plugin\Commerce\PaymentGateway\Checkout->refundPayment() (line 669 of modules/contrib/commerce_paypal/src/Plugin/Commerce/PaymentGateway/Checkout.php).

Here a first concept how this could look like. Of course, we should streamline this approach through moving this code into its own service and using the service, whenever we catch a "BadResponseException".

Inside the service we could also filter out, for which error we want to display further information and for which not (if needed). We can also differentiate between what error should be thrown for an admin user vs. what should be shown for a normal user (using a dedicated permission).

What are your thoughts on this @jsacksick? Would this be worth pursuing? Here is a screenshot of the current MR (although the Markup is escaped):

Just my 2 cents: I think handling the JSON returned from PayPal is essential, especially for cases, where buyer interaction is needed, like in 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active .

Putting the handling into a dedicated service also makes a lot of sense to me, to being able to centrally handle such messages and related actions. For example, showing the informational link or even redirecting users. Otherwise, such logics may mess up the code around.

Passing a formattable markup to an exception doesn't look very clean... Perhaps we just need to use the messenger?

@jsacksick thanks - yes I also think we should use the messenger for such details. What do you think about the general architecture - putting this into a dedicated service to handle the different types of messages etc.?

An error handler service or helper would make sense if we were doing some mapping between a code and a human readable message which isn't the case here? We're just spitting the error returned right?

But what's shown in the screenshot looks kinda buggy (as we see the tags etc)

Yea just quick concept code, I wanted to hear your feedback first.

A service would be preferable if we want to hide information about specific errors or specific roles.

List of the available errors and their messages: https://github.com/paypal/paypal-js/blob/a2e716e0a0982fe79c8c51e8c04e187...

An error handler service or helper would make sense if we were doing some mapping between a code and a human readable message which isn't the case here? We're just spitting the error returned right?

Nope, I think we need to do exactly that - handle messages differently based on their type and eventually perform additional actions, so there's some complexity.

For example, see here: 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active Paypal expects to (re)direct the user to a certain action URL.

For example, see here: #3489875: Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Paypal expects to (re)direct the user to a certain action URL.

But this issue is just about refunds for now, right?

This is meant as a general solution for all cases where a "BadResponseException" is being processed. Sorry if the MR confused you. I just picked out one place where this is the case.

@jsacksick sadly no, I think there are cases where Paypal asks for further verifications, see the linked issue:

Using PayPal Checkout with the buttons in cart can sometimes lead to a PayPal error with
name: UNPROCESSABLE_ENTITY
details: issue: PAYER_ACTION_REQUIRED

So PAYER_ACTION_REQUIRED is one of the possibly more complex examples. Refunds might be a simpler one.

Ok, that should be it! Please review!

Note that this service is currently pretty "barebones", but can be extended in follow-up issues like 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active . We think it only makes sense to do the error handling in one place, which we can further refine in the future.

I'm not sure it makes sense to call it BadResponseExceptionHandler as it means we're deriving the name on the service based on the exception thrown. What if tomorrow we simply catch Request exceptions instead? Or if Guzzle renames this?

Also, Braintree defines an ErrorHelper for example with 2 static methods (See https://git.drupalcode.org/project/commerce_braintree/-/blob/8.x-1.x/src...).

Maybe we can call the service ErrorHelper too? Or perhaps PaymentExceptionHandler? Also, with what we're doing, wouldn't that mean that we get boh the generic Drupal Commerce error + this one? Which isn't necessarily good for UX?

Yeah, I had similar thoughts.

I like:

• PayPalErrorHandler
• PayPalErrorResponseHandler
• PaymentExceptionHandler
• PaymentErrorHandler

PayPalErrorHandler or PayPalErrorResponseHandler are most clear and descriptive IMHO.

Anyway the service gives us wonderful options for future extensions and changes. Nice work @Grevil! :)

I'd prefer "PaymentExceptionHandler". None of the other classes have a "PayPal" prefix.

Also, with what we're doing, wouldn't that mean that we get boh the generic Drupal Commerce error + this one?

Yea, we would get the error + a message with the same error. I agree, that this wouldn't be very nice UX wise. I'd say, we throw a generic Error Text and let the message be more specific.

Adjusted accordingly. Please review now.

Ok, back to needs work. Newest developments in 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active , show, that we need to properly handle the details and especially the links in a loop instead of resetting to the first issue / link available.