Implement error handling in JMail#9881
Implement error handling in JMail#9881wilsonge merged 2 commits intojoomla:stagingfrom mbabker:JMail-error-handling
Conversation
|
I have tested this item ✅ successfully on 821c0cd #testing instructions
After Patch: This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9881. |
|
It shouldn't be fixed. Just any errors being thrown are now caught in On Tuesday, April 12, 2016, Jelle Kok notifications@github.com wrote:
|
|
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:
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... |
|
Moving to Needs Review than 😄 This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9881. |
|
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... |
Summary of Changes
JMailhas 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 toJMailthat 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
JMailshould 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
JMailmethods can now return a boolean false value in addition to returning$thisinstance for method chaining. Short of addingJErrorstyle error references, this has to happen. Personal rant, the implementation ofJMailis 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.