[#9881] - Implement error handling in JMail
- Fixed in Code Base
- 7 May 2016
- Medium
- Build: staging
- # 9881
- Diff
- mbabker:JMail-error-handling
- Pending JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed. Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Summary of Changes
JMail has absolutely zero sense of error handling built into it at all right now, so all APIs assume all transactions can only be completed successfully. This lack of error handling has in large part played into the number of extensions with mail related errors in 3.5.1 (exposed by the throwing of exceptions). This PR adds a layer of error handling to JMail that is desperately needed.
Testing Instructions
JMail should now correctly catch error states (either thrown exceptions or boolean returns) for any methods it wraps. When an error occurs, a boolean false value is returned. This should eliminate most of the thrown exceptions, however, as of 4.0 JMail should stop catching these exceptions and let them bubble up to the caller (extension code) to handle these errors.
B/C Concerns
Technically, this is a break of B/C as the JMail methods can now return a boolean false value in addition to returning $this instance for method chaining. Short of adding JError style error references, this has to happen. Personal rant, the implementation of JMail is totally screwed by breaking the interface of PHPMailer (adding additional allowed arguments in methods and changing the return types) and having never implemented an error handling layer. This way at least forces some sense of error handling (and required checking) into implementor's uses, but anyone relying on method chaining gets screwed if a false gets returned.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
brianteeman
- | Category | ⇒ | Libraries |
It shouldn't be fixed. Just any errors being thrown are now caught in
JMail. Presumably whatever method was throwing the error before still is,
but now you're getting a Boolean false return.
On Tuesday, April 12, 2016, Jelle Kok notifications@github.com wrote:
I have tested this item [image:
] successfully on
821c0cd
821c0cdOur issue with sending report email, is fixed by this pr.
#testing instructions
1) Install kunena
2) Setup kunena with email on config
3) Create different messages
4) Send with other account, a "report this message"
Before Patch:
5) with Blue Eagle Kunena template: Invalid address: admin
with Crypsis template: successfull sendAfter Patch:
5) with Blue Eagle template: successfull send
with Crypsis template: successfull send
This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/9881
https://issues.joomla.org/tracker/joomla-cms/9881.—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#9881 (comment)
At least we deceasing this "Blue eagle template" this month, with our next big update. Other template doesn't have this issue. So the old users are still safe with the next joomla update. When this pr is merged.
I would have agreed with this PR before launching Joomla! 3.5.1 (remember my questions in #9530?), but by now developers should have updated their code to catch thrown errors (or else their extensions would no longer work, and I'm sure their customers / users noticed that).
Seeing the glass half full, even after being bombarded with support requests from customers, it's now far easier to debug wrong email settings thanks to exceptions. My personal opinion is that reverting this once again will create more confusion.
Rant over, here are the test results:
- Before patch: using
JFactory::getMailer()->setSender(array('wrongemail', 'Wrong Email'));would throw an exception - After patch: using
JFactory::getMailer()->setSender(array('wrongemail', 'Wrong Email'));does not throw an exception (the function returns booleanfalse).
As far as testing goes, the patch works as described.
Removing all the catches is actually pretty easily, but the boolean returns do need to stay in place for conditions when exception throwing is turned off so you don't get back into a 3.5.0 state of JMail blissfully ignoring errors.
@joomla/cms-maintainers someone care to make a decision with this regarding the last couple of comments? @OctavianC makes a valid point...
| Status | Pending | ⇒ | Needs Review |
Moving to Needs Review than 
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9881.
| Status | Needs Review | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-05-07 11:31:48 |
| Closed_By | ⇒ | wilsonge |
| Milestone |
Added: |
||
I think @OctavianC 's point is valid too - but sadly I think the majority of extension developers care more about b/c than having good quality maintainable code - so merging as is...
I have tested this item
successfully on 821c0cd
Our issue with sending report email, is fixed by this pr.
#testing instructions
1) Install kunena
2) Setup kunena with email on config
3) Create different messages
4) Send with other account, a "report this message"
Before Patch:
5) with Blue Eagle Kunena template: Invalid address: admin
with Crypsis template: successfull send
After Patch:
5) with Blue Eagle template: successfull send
with Crypsis template: successfull send
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9881.