? Pending

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
12 Apr 2016

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.

avatar mbabker mbabker - open - 12 Apr 2016
avatar mbabker mbabker - change - 12 Apr 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 12 Apr 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 12 Apr 2016
Category Libraries
avatar 810 810 - test_item - 12 Apr 2016 - Tested successfully
avatar 810
810 - comment - 12 Apr 2016

I have tested this item :white_check_mark: 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.

avatar mbabker
mbabker - comment - 12 Apr 2016

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: :white_check_mark:] successfully on
821c0cd
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
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)

avatar 810
810 - comment - 12 Apr 2016

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.

avatar OctavianC
OctavianC - comment - 18 Apr 2016

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 boolean false).

As far as testing goes, the patch works as described.

avatar mbabker
mbabker - comment - 18 Apr 2016

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.

avatar mbabker
mbabker - comment - 2 May 2016

@joomla/cms-maintainers someone care to make a decision with this regarding the last couple of comments? @OctavianC makes a valid point...

avatar zero-24 zero-24 - change - 7 May 2016
Status Pending Needs Review
avatar zero-24
zero-24 - comment - 7 May 2016

Moving to Needs Review than :smile:


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9881.

avatar wilsonge wilsonge - change - 7 May 2016
Status Needs Review Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-05-07 11:31:48
Closed_By wilsonge
avatar wilsonge wilsonge - close - 7 May 2016
avatar wilsonge wilsonge - merge - 7 May 2016
avatar wilsonge wilsonge - reference | a7081c4 - 7 May 16
avatar wilsonge wilsonge - merge - 7 May 2016
avatar wilsonge wilsonge - close - 7 May 2016
avatar wilsonge wilsonge - change - 7 May 2016
Milestone Added:
avatar wilsonge
wilsonge - comment - 7 May 2016

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...

Add a Comment

Login with GitHub to post a comment