Skip to content
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

Merged
merged 1 commit into from Dec 10, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 2, 2020

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

  • need to think a bit more about the messages though

@civibot
Copy link

civibot bot commented Nov 2, 2020

(Standard links)

@civibot civibot bot added the master label Nov 2, 2020
@mattwire
Copy link
Contributor

mattwire commented Nov 2, 2020

This is kind of related to #18466 and should probably be reviewed in parallel

@monishdeb
Copy link
Member

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?

@eileenmcnaughton
Copy link
Contributor Author

@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!

@mattwire
Copy link
Contributor

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
@eileenmcnaughton
Copy link
Contributor Author

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

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 yay it passed

@seamuslee001
Copy link
Contributor

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

@seamuslee001 seamuslee001 merged commit f107e5e into civicrm:master Dec 10, 2020
@seamuslee001 seamuslee001 deleted the mail branch December 10, 2020 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants