REF Update CiviCRM default PEAR Error handling to be exception rather… #19323
Conversation
… than just PEAR_Error object and remove useException overirdes as no longer needed and patch necessary places to handle with Exceptions coming back
|
(Standard links)
|
|
One advantage of this also is that it means that our unit tests error handling will now be the same as general runtime |
|
If jenkins is OK with this I'd be keen to merge once the rc is cut. I think this is definitely where we want to wind up on this |
| @@ -1398,9 +1398,6 @@ public static function sendSMSMessage( | |||
|
|
|||
| $providerObj = CRM_SMS_Provider::singleton(['provider_id' => $smsProviderParams['provider_id']]); | |||
| $sendResult = $providerObj->send($recipient, $smsProviderParams, $tokenText, NULL, $sourceContactID); | |||
| if (PEAR::isError($sendResult)) { | |||
| throw new CRM_Core_Exception($sendResult->getMessage()); | |||
| } | |||
There was a problem hiding this comment.
I guess I can test this but I think most SMS extensions specifically call PEAR::raiseError, so does that internally now get converted automatically to an exception or would that still be a pear error?
There was a problem hiding this comment.
It would get converted because this is changing the PEAR error callback https://github.com/civicrm/civicrm-core/pull/19323/files#diff-dc1c0506257784ef60b0b3f296a5e64c633c0a19d47dbd87885eb21852836fd6R89 to be this function instead of this one
There was a problem hiding this comment.
So no more familiar yellow screen? 😢 I'll get over it...
Tested and yes it errors with a PEAR_Exception now.
There was a problem hiding this comment.
It will because if Exception isn't handled it should wind up here https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Error.php#L438
There was a problem hiding this comment.
Oh I see, you just have to scroll down to see it. 😀
|
@eileenmcnaughton now that we have RC should we merge this? |
|
OK let's do it. I re-read the code and I think it should all be OK & we get a nice long lead time |
|
One thing I'm noticing about this is that api calls fail more silently because they catch the PEAR_Exception. It still logs it, but there might not be any visual clues that something went wrong. An example is a db call during an api call that fails with No Such Field. The main problem though is that in unit test runs such a thing now succeeds, whereas before it error'd. Hmm. |
|
@demeritcowboy can we replicate that? if so we should dig |
|
Ok have put up #19532. I'm sure I could think of a better one to use for real, but should work for demo purposes. |
Following up on civicrm#19323 this converts APIv3 in Smarty and Ajax to use exceptions instead of overriding the PEAR exception handler. Now that PEAR throws exceptions there is no need to do so and it was interfering with try/catch handlers within the api call.
… than just PEAR_Error object and remove useException overirdes as no longer needed and patch necessary places to handle with Exceptions coming back
Overview
This is a substantial change but an important one and I think we can do this now. This changes the default error handling of PEAR Errors to be rather than just processing the PEAR_Error object that we now convert it into throwing an Exception. This has always been the case for our unit tests but not in general run time. I took a look by greping for isError and also PEAR_Error and didn't see many instances where we were directly relying on PEAR_Error and in those instances I have changed to use try catch versions
Before
During normal run time PEAR_Error emitted
After
Exception Emitted containing the PEAR_Error object during normal runtime
Technical Details
Note this doesn't touch a number of places in the MailingJob / Mailing Code as those explicitly set the temporary Error scope to be ignoreException so shouldn't be impacted by this change
ping @totten @eileenmcnaughton @mattwire @demeritcowboy