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

fix cc mailaddress display name Garbled on multibyte character #1329

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ken-takagi
Copy link

@ken-takagi ken-takagi commented Feb 5, 2020

Purpose

fix cc mailaddress display name Garbled on multibyte character

Background Context

same bcc list approach?

References

fix cc mailaddress display name Garbled on multibyte character
@to-lz1
Copy link

to-lz1 commented Jul 7, 2021

Hello. We use this play1 for our production, and want this PR to be merged.

https://lighthouseapp.com/dashboard seems to be unavailable,
so we can't figure out how to contact the reviewer of this project ...

We really appreciate if you notice and make any response. Thank you.

@tomparle
Copy link
Contributor

tomparle commented Jul 7, 2021

Hi @to-lz1

Play developers definitely still see what's happening here, and this is the right place to discuss about it !
I do not see anything preventing from this PR to be merged, but releases are indeed not frequent.
Maybe @xael-fry or @asolntsev could plan a next release soon ?

Meanwhile, you could also use a custom build of Play which would include this PR and use it. Tell us if you need some tips to do so.

@asolntsev
Copy link
Contributor

That's right, we do see pull requests and comments.

In this pull request, I would like to see a unit-test showing that the change does work. I expect to see the email sample that did need such a change.

Then @xael-fry could make a release.

@to-lz1
Copy link

to-lz1 commented Jul 7, 2021

@tomparle @asolntsev

Thank you very much for your quick response!

I was relieved to hear that Play developers still check here, and I agree with @asolntsev. I think it would be better to add some unit-test and email samples to this PR. I'll talk to @ken-takagi about that (in fact, he is my colleague).

@to-lz1
Copy link

to-lz1 commented Jul 11, 2021

@asolntsev Hi, I took a look at this issue.

Without this change, mail address handling will be slightly different from what we expect.

jshell> import org.apache.commons.mail.SimpleEmail;
jshell> import javax.mail.internet.InternetAddress;

jshell> Email sampleMail = new SimpleEmail();
jshell> String multibyteCharAddress = "Eva Nováková <[email protected]>"

# current code's approach
jshell> sampleMail.addCc(multibyteCharAddress);
jshell> sampleMail.getCcAddresses().get(0)
==> "Eva Nováková" <[email protected]>

# this PR's approach
jshell> InternetAddress iAddress = new InternetAddress(multibyteCharAddress);
jshell> sampleMail.addCc(iAddress.getAddress(), iAddress.getPersonal());
jshell> sampleMail.getCcAddresses().get(1)
==> =?UTF-8?Q?Eva_Nov=C3=A1kov=C3=A1?= <[email protected]>

As we can see, in the second example, non-ASCII characters are encoded based on RFC2047.

Without this, our mailer can't know how to decode recipients' addresses, especially when they include some multibyte characters. We sometimes get garbled addresses such as ������ <[email protected]> displayed.

I considered adding tests about this. But I think this problem is rather related to the behavior of Apache Commons' Email class.

Moreover, in this(12 years old) commit, other (From, ReplyTo, and Bcc) addresses already experienced this change. Probably this change list has no bad effect, doesn't it?

@asolntsev
Copy link
Contributor

@to-lz1 Yes, I understand that this test seems more like a test for Apache Commons' Email class. But not only. It verifies that Play does use the right library.
Anyway, it's better to have such a test than not to have.

@yuba
Copy link
Contributor

yuba commented Sep 26, 2022

@asolntsev
Now that the UT you pointed out has been ready, could you consider merging this PR?

@asolntsev
Copy link
Contributor

@yuba I clicked "Accept" in code review. The last word is usually from @xael-fry - could you please merge this pull request and inform us about release plans?

@yuba
Copy link
Contributor

yuba commented Oct 3, 2022

@xael-fry How about including this PR in release 1.8.0?


static void setAddresses(Email email) throws EmailException {
email.setCharset("utf-8");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why forced the charset to utf-8 here ? in the upper code you only set the charset if define ?
Do you mean that a default value is need ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xael-fry
This encoding fix was originally written, in line 543.
This line appears to have been moved to the setAddresses () method as it relates to setting addresses.

Copy link

@irxground irxground Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to keep the behavior of the original code.

In the original code, the encoding is changed to UTF-8 before setting the from, reply-to, to, cc, and bcc addresses, and is changed to charset before setting the subject.

(I'm a co-worker of @ken-takagi and @yuba )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants