-
-
Notifications
You must be signed in to change notification settings - Fork 814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dev/core#2159 Handle exceptions in Mail:send class #18905
Conversation
(Standard links)
|
This is kind of related to #18466 and should probably be reviewed in parallel |
I am happy with the fix. @eileenmcnaughton can you add a unit test which try-catch the expection and assert the error message for using wrong paramter or something else which could result in an exception? |
b9ffa87
to
fa9514a
Compare
@monishdeb that was a bit of a curly one to write a test for - I wasn't sure for a minute there I could figure it out but I'm happy with the result - as long as jenkins is! |
Linking to #18466 |
This adds handling for exceptions - allowing the class to be more effectivel swapped out - need to think a bit more about the messages though
fa9514a
to
c9d8809
Compare
Test fail was because it couldn't serialize my setting (because it had an object in it). I think it's fixed. In terms of how this helps with #18466 - it removes one of the steps en-route to removing ignoreException mode. If we could be sure that we were always throwing exceptions (the thing ignoreException mode messes with) we could be sure that #18466 is safe |
@seamuslee001 yay it passed |
This makes sense to me and ensures we are generating a consistent return and I did some r-run and confirmed the backtrace didn't seem to change and this functionally ensures that we are returning the same thing if our source threw an Exception or a Pear Error, I'm going to merge this as I think this is a good improvement |
Overview
In theory the mail class can be easily swapped out - but expecting a replacement to return a PEAR_ERROR is a bit much! This adds handling for exceptions
https://lab.civicrm.org/dev/core/-/issues/2159
Before
PEAR_Error is required on error
After
An exception can be thrown
Technical Details
Mostly in https://lab.civicrm.org/dev/core/-/issues/2159
Comments