The addReplyTo doesn't take an array as argument in contact form

[roland-d]

Summary of Changes

When sending out an email via the contact form and selecting to also send an email to yourself an error will be thrown

Testing Instructions

1. Get the latest staging version
2. Install the latest staging with sample data
3. Go to the Single contact menu item
4. Open the contact form and fill in all fields
5. Make sure you check the box for sending a copy to yourself
6. Send the email
7. The error will show up
8. Apply this patch
9. Send the form again and it will send the emails

[brianteeman]

I can not replicate this error on 3.5 and there have been no pull requests to staging since 3.5 that effects this file. If it is really broken in staging then the cause must be one of those very few pr

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9577.}

[brianteeman]

I can however confirm this issue with staging but surely the problem is somewhere else
https://github.com/joomla/joomla-cms/pulls?q=is%3Apr+milestone%3A%22Joomla+3.5.1%22+is%3Aclosed

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9577.}

[mbabker]

It's a legit issue, one that's been there for a while. The other case a few lines above was fixed last year. #6127

[mbabker]

More specifically, it's a 4 year old bug assuming nothing in JMail changed in the meantime: 67bdb4f

[brianteeman]

So how can I not replicate it on 3.5 but replicate it on staging - makes no
sense to me if this is the fix

On 24 March 2016 at 22:11, Michael Babker [email protected] wrote:

It's a legit issue, one that's been there for a while. The other case a
few lines above was fixed last year. #6127
#6127

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9577 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

[mbabker]

Beats me, but just reading the code I see why it's wrong but no idea why the wrong way would work.

[brianteeman]

Thats what concerns me most - that one of the very few changes that have been made have either created or exposed this issue.

The pull request works but thats not the issue

[roland-d]

Let me explain what is going on.

First we comitted PR #9528 which has this change:
https://github.com/joomla/joomla-cms/pull/9528/files#diff-98e863dabf8033c2313bc92e9c99cc2fR42

As the comment above it says, the true turns on the exception handling in PHPMailer. So instead of returning false which is simply ignored on this line:
https://github.com/joomla/joomla-cms/pull/9577/files#diff-7178e53b18e4d0dc8db925e8b1be7906L220

PHPMailer now throws an exception which is caught by the exception handler in Joomla and shows the error page.

This is why this bug is now exposed, why it worked before and why it no longer works now.

[brianteeman]

Thank you for the explanation which 100% answers my concerns

On 24 March 2016 at 22:29, RolandD [email protected] wrote:

Let me explain what is going on.

First we comitted PR #9528
#9528 which has this change:

https://github.com/joomla/joomla-cms/pull/9528/files#diff-98e863dabf8033c2313bc92e9c99cc2fR42

As the comment above it says, the true turns on the exception handling
in PHPMailer. So instead of returning false which is simply ignored on this
line:

https://github.com/joomla/joomla-cms/pull/9577/files#diff-7178e53b18e4d0dc8db925e8b1be7906L220

PHPMailer now throws an exception which is caught by the exception handler
in Joomla and shows the error page.

This is why this bug is now exposed, why it worked before and why it no
longer works now.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9577 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

[brianteeman]

I have tested this item ✅ successfully on c65b560

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9577.}

[roland-d]

I did a scan through the codebase, this was the only place where the addReplyTo has been used incorrectly.

[schultz-it-solutions]

Am I understanding this correctly, that for any third-party extension, that uses the JFactory::getMailer()->addReplyTo () with an "array (address, name) " this will also break the code? If so, is this not also the case for the *_JFactory::getMailer()->setSender () *_method?

[mbabker]

Any addXXX method will not work with that array format. You'll have to look at setSender but last I recall it doesn't use the same code as the add methods so that format MAY work.

[schultz-it-solutions]

just looked it up in Joomla Api:

setSender
Set the email sender
setSender(mixed $from) : \JMail
since 11.1
throws \UnexpectedValueException
Arguments $from
mixedemail address and Name of sender array([0] => email Address, [1] => Name) or as a string

[schultz-it-solutions]

Actually, my own extension jDBexport seems to run into this issue...

Maybe the JED should issue some kind of warning to the developers? Is that possible?

[mbabker]

I wouldn't trust the doc blocks as source documentation to be quite frank. You're better off reading the actual method code to decipher what it does and doesn't support. JMail::Send() doesn't actually return a JError object and that's what the doc block says it does as of 3.5.1.

[schultz-it-solutions]

which I just now did (/libraries/joomla/mail/mail.php (line 169ff)
It allows an array OR a string.
So I am understanding this right:
If I submit an array to setSender($from), this function will accept it, but PHPMAILER will throw the exeption?
If so, I strongly belief this should be communicated actively to the extension developers, dont you agree?

[mbabker]

PHPMailer doesn't have a setSender method. That's a Joomla custom thing. Just like the addXXX methods in PHPMailer don't accept arrays as the arguments or implement fluent interfaces like the JMail class does. PHPMailer is throwing an exception if the e-mail address that gets provided into setFrom (either through setSender and whatever format you've injected your param(s) into it or direct calling setFrom) isn't valid. That doesn't need a special message to anyone, that's always been the behavior. The problem is JMail has NEVER, EVER, handled ANY error condition raised by PHPMailer EXCEPT in the Send method. JMail has ALWAYS been silently ignoring errors and continuing operations like nothing could ever possibly go wrong. No amount of communication to extension developers can address that unless you intend on telling them to bypass JMail and JFactory::getMailer() completely in favor of directly using PHPMailer, which to be quite frank that IS my recommendation right now.

[roland-d]

@schultz-it-solutions

If I submit an array to setSender($from), this function will accept it, but PHPMAILER will throw the exeption?

It will throw the exception if the first argument in your array is not a valid email address, not because you are supplying an array to setSender().

setSender() itself will throw an exception if you supply it with something else than an array or string.

[baconbrad]

Since this has not only broke my custom plugins, other developer plugins, and widely used plugins why didn't anyone think this fix for the mailer would be a big breaking change prior to pushing it? I am not against fixing it. But this should of been a major version change. Not a minor version change.

[roland-d]

@baconface Who could have expected we all wrote such bad code or perhaps copy/pasted as much? It broke my stuff as well as I found out later than sooner.

[baconbrad]

@roland-d I wouldn't consider what I wrote as bad code when I written it based off the official Joomla API documentation.

[mbabker]

Because that syntax hasn't worked correctly since 3.0.0 but because JMail does jack nor crap for error handling the errors PHPMailer was rightfully raising were being blissfully ignored and everything looked like it was working beautifully when in fact it hasn't been for the last 3 and a half years.

[roland-d]

@mbabker Agree with you there and I think we are in for quite a treat when we turn everything into exception handling.

@baconface The way I see it that at least for my own code, I made the mistake of not doing proper error handling. Just assumed the mail went out all was good :)

[baconbrad]

@mbabker That is where I am getting at. It is good to fix this behavior. I am not faulting the Joomla team for that. But the fix resulted in broken plugins/codebases. It would of made more since to push the fix off for a major version. At least a better heads up about this breaking change than finding it out by updating. To me, it was just a bad way to approach this fix. Especially since the old/wrong method was encouraged to developers by Joomla documentation.

[mbabker]

I can't agree with that assessment. We know an API is completely disregarding errors, IMO it isn't acceptable to punt addressing that to a major version bump. Yes, it has probably broken every line of code written for Joomla 3.x that interfaces with the JMail API, but I honestly can't in good faith suggest that until a major version bump happens that JMail should just discard every error that gets raised by it or PHPMailer.

You can argue the fact that throwing exceptions is a B/C break. That argument has validity. But the behavior of JMail or PHPMailer didn't change between 3.5.0 and 3.5.1 to start causing the errors that exceptions are being thrown for. If you take a 3.0.0 install and turn on exception throwing in PHPMailer the same errors with the same datasets should be occurring.

[schultz-it-solutions]

I think we all agree that fixing the issue was the right thing to do (and following up on this in all relevant 3p-extensions is crusial). Only as a remark on process, a respective note actively pushed by the Joomla team could have prevented a lot of headache for a lot of developers. Maybe for future cases, this could be implemented "somehow"?

[brianteeman]

Active participation in the beta and rc testing would have saved you any
heartache
On 19 Apr 2016 9:24 pm, "Ruediger Schultz" [email protected] wrote:

I think we all agree that fixing the issue was the right thing to do (and
following up on this in all relevant 3p-extensions is crusial). Only as a
remark on process, a respective note actively pushed by the Joomla team
could have prevented a lot of headache for a lot of developers. Maybe for
future cases, this could be implemented "somehow"?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9577 (comment)

[baconbrad]

Yes, it has probably broken every line of code written for Joomla 3.x that interfaces with the JMail API,

This is exactly why this update should of been treated more seriously than a minor update.

[mbabker]

Except nobody realized how FUBAR JMail was until well after the release. It took a day for me to realize JMail::Send() actually has error handling but it only returns boolean false if the mailer is explicitly disabled (JException objects are returned otherwise, so if (!$mail->Send()) would never get triggered if that was your error checking mechanism). It took me longer to realize that every other part of JMail breaks the PHPMailer interface and has fully discarded all errors. No matter how JMail changed to handle errors, it would have broken more extensions than it would have "fixed" bugs. So I maintain there is/was no way to implement error handling into JMail without some kind of B/C break, and I maintain it isn't acceptable to sit on that until 4.0 because to me there is a higher risk in an API blindly discarding errors than "fixing" extensions to make it look like everything was working correctly when it isn't/wasn't.

[baconbrad]

@mbabker Was it not possible to allow both usages and make the previous/wrong usage deprecated?

[baconbrad]

@mbabker I am not trying to discount your work and I know you meant well. But if I understand what you are saying we went from risking breaking things to breaking things. And that is what I am having trouble making sense of.

[mbabker]

The wrong usage which hasn't been in the API since 3.0.0? That'd be re-introducing a feature which was already phased out of the API and even then it wouldn't have fixed everyone's issues.

addReplyTo() hasn't changed since 3.0.0. The syntax accepted in 2.5 only appeared to work because of the fact that errors were going unhandled. Any change which caused JMail to return errors would have caused that syntax to break. That's the underlying issue.

[baconbrad]

The wrong usage which hasn't been in the API since 3.0.0?

If this has been the case I apologize. The API guide for 3.X does show you should send two strings instead of an array opposed to the API guide for 2.5 which instructs you to use an array. However I have not found any migration notes or deprecation notes that identified this as a change. And the API guide doesn't state that it's usage is different unless you just so happened to realize you are no longer sending an array. So if it was deprecated it never made it to any noticeable documentation. Even the largest plugin developers that used JMail were caught off guard by the change. So even though the wrong usage wasn't officially supported it still appeared supported and developers were under the impression it was still supported.

[mbabker]

There was a PR on the old Platform repo which caused the break when it
added the protected add method to JMail to cut the code duplication that
all the addXXX methods call into. The behavior had no test coverage around
it and between that and the lack of error handling it's easy to assume that
nobody realized that change happened until 3.5.1 exposed
phpmailerExceptions.

On Tuesday, April 19, 2016, Brad Metcalf [email protected] wrote:

The wrong usage which hasn't been in the API since 3.0.0

If this has been the case I apologize. The API guide for 3.X does show you
should send two variables instead of an array opposed to the API guide for
2.5 which instructs you to use an array. However I have not found any
migration notes or deprecation notes that identified this as a change. And
the API guide doesn't state that it's usage is different unless you just so
happened to realize you are no longer sending an array. So if it was
deprecated it never made it to any noticeable documentation. Even the
largest plugin developers that used JMail were caught off guard by the
change. So even though the wrong usage wasn't officially supported it still
appeared supported and developers were under the impression it was still
supported.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9577 (comment)