Exceptions - What could possibly go wrong?

[totten]

Overview

An observation from @eileenmcnaughton:

...we have THREE types of common generic CiviCRM exceptions (CRM_Core_Exception civicrm_api3_Exception, ApiException) which are rarely usefully different. The v3 exception exends the Core exception but for no apparent reason the api exeption doesn't. The net effect of us having this differentiation in garden-variety exceptions is that our comment blocks are rarely up-to-date...

I agree these classes basically all implement the same pattern (ie general-purpose exception class, using $code to represent the actual error condition).

This PR is a trial-balloon to see what would happen if simply combined the three into the same class.

Before

• Three classes: CRM_Core_Exception, API_Exception, CiviCRM_API3_Exception

After

• One class: CRM_Core_Exception
• Two class aliases API_Exception, CiviCRM_API3_Exception

Technical Details

Is this safe? Who knows? I can think of a couple general questions:

1. Are the semantics and the method signatures within the classes compatible?
2. Will the various users of the classes (throwers, catchers, subclasses) still work as expected?

For #1, I think we can analyze the classes directly. (Dunno if current draft is perfect - but I think we can be optimistic about this one.) Observations about similarities/differences:

• The name CRM_Core_Exception is more generic (ie it would fit in more places than API_Exception or CiviCRM_API3_Exception)
• They all have the same constructor signature and the same properties ($message, $error_code = 0, $errorData = [], $previous = NULL)
• The name of the freeform array differs ($errorData vs $extraParams). In each case, the field is private, so callers would only care about getter/setter methods.
• The methods getErrorData() and getExtraParams() can simply do the same thing.
• The handling of $message is slightly different. API_Exception and CiviCRM_API3_Exception have the formulation parent::__construct(ts($message)..., which is generally suspect on l10n grounds.
• API_Exception has more developed $errorCode -- it has a list of known-codes, and it explicitly bridges the int|string discrepancy. (IIRC, top-level Exception always suggested int but didn't enforce it, and so it was common to find Civi callers that gave strings? But as type-checking gets stronger on Exception::$code, that gets ill-defined. API_Exception explicitly allows either.) IMHO, it's sensible for all of them to bridge int|string situation this way.

I think #2 is the harder question. Brainstorming a bit... here are some foreseeable effects of class_alias() approach:

• (2T) All three symbols can still be thrown the same way.

  throw new CRM_Core_Exception("Oops");
  
  throw new API_Exception("Uh oh");
  
  throw new CiviCRM_API3_Exception("Egad");

• (2E) All three symbols can still be extended the same way.

  class OopsException extends CRM_Core_Exception {}
  
  class UhOhException extends API_Exception {}
  
  class EgadException extends CiviCRM_API3_Exception {}

• (2C) All three symbols can still be caught the same way

  try { ... } 
  catch (CRM_Core_Exception $e) {
    printf("Oops (%s): %s\n", $e->getErrorCode(), $e->getMessage());
  }
  
  try { ... }
  catch (API_Exception $e) {
    printf("Uh oh (%s): %s\n", $e->getErrorCode(), $e->getMessage());
  }
  
  try { ... } 
  catch (CiviCRM_API3_Exception $e) {
    printf("Egad (%s): %s\n", $e->getErrorCode(), $e->getMessage());
  }

• (2I) There would be functional-changes in code which checked one type but ignored the other types.

  try {
    ...
  }
  catch (CRM_Core_Exception $e) {
    // This codepath previously only caught generic exceptions that were stylized as `CRM_Core_Exception`.
    // Now this codepath also catches generic exceptions that are stylized as `API_Exception` or `CiviCRM_API3_Exception`.
    // Examination of the error-code/message/error-data should work the same way.
  }

• (2M) There would be functional-changes in code which specifically discriminates between these types (ie you expected both CRM_Core_Exception and API_Exception, but for different reasons, and wanted to do something different with each)

  try {
    ...
  }
  catch (CRM_Core_Exception $e) {
    echo "Oops";
  }
  catch (API_Exception $e) {
    echo "Uh oh";
  }
  catch (CiviCRM_API3_Exception $e) {
    echo "Egad";
  }

• (2S) There would be functional-changes in string-literal reflection on the exception-type.

  if (get_class($e) === 'API_Exception') { /* change behavior */
    echo "Uh oh";
  }
  
  if (get_class($e) === API_Exception::CLASS) { /* no change behavior */
    echo "Uh oh";
  }

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

Apparently this is what could wrong....

Test Result (4 failures / +4)Civi\API\KernelTest.testResolveExceptionCRM_Core_BAO_MappingTest.testGetCreateMappingValuesapi_v3_APITest.testV3WrapperExceptionapi_v3_ActivityTest.testActivityCreateWithInvalidPriority

Test Result (4 failures / +4)
Civi\API\KernelTest.testResolveException
CRM_Core_BAO_MappingTest.testGetCreateMappingValues
api_v3_APITest.testV3WrapperException
api_v3_ActivityTest.testActivityCreateWithInvalidPriority

[totten]

Apparently this is what could wrong....

It looks like these 4 failures all stemmed from the same (2M) multi-catch statement within Kernel::runSafe(). New push should fix that part... (It did locally.)

[eileenmcnaughton]

@totten @colemanw @seamuslee001 @demeritcowboy @mattwire - assuming we merge this then the protocol going forwards would be to always throw CRM_Core_Exception & slowly phase the others out.

Aside from the issue where this came up I would say there is a good handful of bugs I've seen over time when the exeption thrown changed between these 3 - by-passing a Catch so I think this will prevent regressions in future

[totten]

I have conflicting intuitions about this. From a general OOP POV, it seems intuitively bad (BC-breaky). But from a Civi-specific POV (a "feel" for the classes), it feels intuitively good.

Here's the best rationalization (in support) that I can come up with:

The classes CRM_Core_Exception, API_Exception, and CiviCRM_API3_Exception are all the same thing; they are generic exception classes with flexible payloads. There are on-going refactors (eg swap APIv3=>APIv4; eg swap BAO=>API) which can arbitrarily change one exception to another. Even today, anyone who catches one of these exceptions would be better off if they caught all three. The existing class-names give a false sense of error specificity - you only get real specificity from the payload ($message, $errorCode, $errorData).

In this view, things are already unstable. Someone who needs broad compatibility (eg say 5.0 to 5.60) should probably use more adaptive code (valid on php71+) like this:

try {
  ...
}
catch (CRM_Core_Exception|API_Exception|CiviCRM_API3_Exception $e) {
  $errorData = is_callable([$e, 'getErrorData']) ? $e->getErrorData() : $e->getExtraParams();
  if ($e->getErrorCode()... or... $e->getMessage()...) { 
    ...
  }
}

The patch here improves upon the status quo because:

• Going forward, anyone who wasn't clever enough to use CRM_Core_Exception|API_Exception|CiviCRM_API3_Exception will effectively get it.
• Going forward, anyone who was clever enough can actually simplify their code (only one catch needed).

But does that outweigh the other intuition (from a general OOP POV) that messing with exception-types is always a breaky signature change?

[eileenmcnaughton]

Hmm - so it's not breaky in the sense of the functions stopping working (getErrorData, getErrorCode) just in that the type of exception in a given scenario will change - but that happens all the time anyway.

I've made a note to put in dev-digest

[artfulrobot]

I like the concept. Instead of class alias though, couldn't we extends CRM_Core_Exception and override the __construct method to issue a deprecated notice?

[totten]

(@artfulrobot) Instead of class alias though, couldn't we extends CRM_Core_Exception and override the __construct method to issue a deprecated notice?

I interpret that to mean:

class API_Exception extends CRM_Core_Exception { ... } 

The effect would be similar-but-different. Aliasing means that catch(CRM...) and catch(API...) have strictly equivalent effects. Subclassing produces an asymmetric effect on existing catch()es. That's very abstract, so here are two examples for compare+contrast:

• (Compare) For this snippet, subclassing and aliasing produce the same change:

  try {
    doSomethingSketchy();
  } catch (CRM_Core_Exception $e) {
    handleException();
  }

  Scenario	Outcome (status quo)	Outcome (with subclass)	Outcome (with class alias)
  doSomethingSketchy() throws CRM_Core_Exception	Runs handleException()	Runs handleException()	Runs handleException()
  doSomethingSketchy() throws API_Exception (The Other)	Unhandled	Runs handleException()	Runs handleException()

• (Contrast) But in this snippet, subclassing is the same as the status-quo. It differs from aliasing.

  try {
    doSomethingSketchy();
  } catch (API_Exception $e) {
    handleException();
  }

  Scenario	Outcome (status quo)	Outcome (with subclass)	Outcome (with class alias)
  doSomethingSketchy() throws API_Exception	Runs handleException()	Runs handleException()	Runs handleException()
  doSomethingSketchy() throws CRM_Core_Exception (The Other)	Unhandled	Unhandled	Runs handleException()

I struggle to find a basis for saying that either formulation is functionally better or administratively easier for downstream (eg if you have 100 existing use-cases with try/catches, maybe one approach works well for 91% and the other works well for 87%; but I can't give reasoned numbers for either approach).

For me, the appeal of class_alias() is that the overall model is flatter (simpler). For a "subclass" relation, I have to look-up the direction ("which X is subclass of which Y"); for "alias", I don't. On the flipside, "aliasing" is probably less familiar. (class_alias() is a niche tool that doesn't show up in a greenfield model; you only use it to reorganize a brownfield model.)

[eileenmcnaughton]

I think at the moment we throw all 3 variants a lot - so it will take a long time to consolidate - but we could at least encourage catching only CRM_Core_Exception & standardise on that over time (although a 'permanent' class would be better named Civi\Exception I suppose

[totten]

The main risk I see is from multi-catch statements. So I grepped universe for each kind of catch(FooException) statement, made a list of files that had multiple kinds of catches, and then skimmed them. All of these cases seemed consistent with my preconception that aliased classes would give something closer to the intended behavior. Just to note a few:

There was one true case of multi-catch expression (ewayrecurring/CRM/eWAYRecurring/Page/VerifyPayment.php):

try {
  // This function will do redirect if the payment failed
  $response = validateEwayContribution($paymentProcessor, $contributionInvoiceID);
} catch (CRM_Core_Exception $e) {
} catch (CiviCRM_API3_Exception $e) {
}

There was one (obvious) case of nested try-catch (uk.co.vedaconsulting.outlookapi/api/v3/CiviOutlook.php), eg

try { 
  $a = civicrm_api3();
  try { 
    $b = civicrm_api3();
  } 
  catch (CiviCRM_API3_Exception $e2) { log($e2); }
}
catch (CiviCRM_API3_Exception $e1) { log($e1); }

Most files had independent try/catch blocks, where each involved a different API call (eg in civirules and civicrm.drush.inc). This required them to switch back/forth between the corresponding exceptions, eg:

try { ... civicrm_api3() ... } catch (CiviCRM_API3_Exception $e) { ... }
try { ... civicrm_api4() ... } catch (API_Exception $e) {... }

I didn't imagine any of these behaving differently with the patch -- and, if anything, there might be some opportunities to simplify if each API call uses the same kind of exception.

[eileenmcnaughton]

yep - that's consistent with my experience

[demeritcowboy]

I'll add a +1. I'd need to spend more time with it though to see if it makes it easier to get the actual location/error of api exceptions (but out of scope for this).

[artfulrobot]

Aliasing means that catch(CRM...) and catch(API...) have strictly equivalent effects

That makes good sense to me and thank you for explaining it. Since people throw whatever and catch whatever the class alias approach seems safer.

[totten]

In light of the affirmative comments, I raised the question of timing with Eileen, Seamus, and Coleman. We agreed it's best to merge this one next week (after freezing 5.51, when we start the 5.52 cycle).

[eileenmcnaughton]

we have frozen