Problem/Motivation

I was trying to debug why I was getting an Exception when accepting a Payment on an Order for a different currency.

My error log looked like:

"NOTICE: PHP message: Uncaught PHP Exception Drupal\Core\Entity\EntityStorageException: "" at /var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php line 786"

I not only had to dig down to see what SqlContentEntityStorage was doing, but had to break on the original exception trace to figure out that it was Price that was throwing the CurrencyMismatchException. It was a bit more effort than that as I also started digging through the issue queue to figure out why the exception could be thrown (understood now).

It would improve developer experience to provide messages for all exceptions. I also noticed empty exceptions thrown for InvalidArgumentException.

Proposed resolution

Provide error messages in exceptions or provide a default message in the initialization method of the exception class?

Remaining tasks

  • Patch review
CommentFileSizeAuthor
#6 3027073-6-price-exception-messages.patch2.69 KBbojanz
#4 commerce-3027073-add-exception-messages-4.patch831 bytesmradcliffe
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mradcliffe created an issue. See original summary.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented

This is concerning. We never expected core to eat any of our exceptions (and convert them into something obscure-sounding).

We generally have a policy of always providing exception messages, looks like the Price object is the only one going against that. No reason not to fix it.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Title: Provide messages to exceptions as the exception may be swallowed by core » Exceptions thrown by Price need to have a message
Category: Feature request » Bug report

So, let's clarify.

We need to fix assertCurrencyCodeFormat and assertSameCurrency.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe at Kosada commented
Issue summary: View changes
Status: Active » Needs review
FileSize
commerce-3027073-add-exception-messages-4.patch831 bytes

Yeah, I agree that it's a bit concerning with respect to core behavior.

Here's a patch. Since it's only adding strings I didn't bother with any additional test assertions.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Needs work

It would be great for debugging if we included the known data in the message.

1) "Invalid currency code USDF"

2)
"The provided prices (10 USD, 20 EUR) have mismatched currencies"
or
"The provided prices have mismatched currencies (USD, EUR)"

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
FileSize
3027073-6-price-exception-messages.patch2.69 KB

Let's wrap this issue up.

Attaching patch that addresses #5.

bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs work » Needs review

  • bojanz committed 65a4a0b on 8.x-2.x 
    Issue #3027073 by mradcliffe, bojanz: Exceptions thrown by Price need to...
bojanz’s picture

bojanz CreditAttribution: bojanz at Centarro commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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