Duplicate logging not needed for PEAR_Errors anymore

[demeritcowboy]

Overview

Unnecessary log entries.

Before

One example is the recent status check for db timezone support. Even though there's a try/catch it still logs it in the log.

Another I've seen is related to activities - there's a performance improvement that does a try/catch because the error is expected, but it still makes log entries (e.g. #18566 (comment))

After

If the exception is later caught, then it's up to that catching code to decide to log/rethrow, as it should be.
If it's not caught later, then it will get logged, as it should. So don't need to log it here.

One way to test this is just add a line like CRM_Core_DAO::executeQuery('this is bad sql'); or PEAR::raiseError('boo'); in preprocess() on some form and then visit the form. You'll see it still logs the error.

Technical Details

I vaguely remember a few years ago that PEAR errors weren't converted to exceptions, or something like that, so the logging might have been more needed.

Comments

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

I think you are right here- we actually just removed something similar from our internal code - these backtraces create a lot of noise & this doesn't seem like the place it is known that something needs to be logged. I guess the risk is that there are places where we stop logging anything? i guess we could check if a generic form submit with error logs as well as fatalling

[demeritcowboy]

Yes I had in the AFTER description one way to test that it still logs is just add a line of code like PEAR::raiseError('boo') to force a PEAR error on a form.

[mlutfy]

I tested with a few scenarios and works for me:

• SQL error
• throw new Exception("test");

and it worked as expected.

Thank you @demeritcowboy !

[demeritcowboy]

Thanks!