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

Small email sending code improvements #2532

Merged
merged 2 commits into from Jun 4, 2022
Merged

Small email sending code improvements #2532

merged 2 commits into from Jun 4, 2022

Conversation

paolobarbolini
Copy link
Sponsor Contributor

@paolobarbolini paolobarbolini commented Jun 4, 2022

Changes building Address by going through the FromStr implementation. Also removes the manual MultiPart build implementation, which must have been a workaround for a bug in a previous version.

Depends on #2531

@BlackDex
Copy link
Collaborator

BlackDex commented Jun 4, 2022

Thanks @paolobarbolini , looks nice.
Regarding the Punycode, i have no clue actually, i tested it without the conversion, and it seems to work just fine.
@dani-garcia do you remember why you added that maybe?

@dani-garcia
Copy link
Owner

We changed it to use punycode because of a user having trouble using it with an internationalized domain: #829.

Not sure if that issue is applicable still.

@BlackDex
Copy link
Collaborator

BlackDex commented Jun 4, 2022

Looking at the changes done on the lettre side, i think this should be fixed now right @paolobarbolini ?

@paolobarbolini
Copy link
Sponsor Contributor Author

paolobarbolini commented Jun 4, 2022

Looking at the changes done on the lettre side, i think this should be fixed now right @paolobarbolini ?

I'll have to test it somehow because I don't think we ever got any issues reported about IDNs, so unless we happened to fix it in the middle of doing other changes we're still handling them wrong.

@BlackDex
Copy link
Collaborator

BlackDex commented Jun 4, 2022

@paolobarbolini Well you changed the email verifying now right? So that might have solved it right now?
I did some testing with several unicode domains, and it looks like not generating an issue.

@paolobarbolini
Copy link
Sponsor Contributor Author

@paolobarbolini Well you changed the email verifying now right? So that might have solved it right now? I did some testing with several unicode domains, and it looks like not generating an issue.

Ah right. The error is in the fact that verification was broken, not that we didn't encode the email properly 😅

@paolobarbolini
Copy link
Sponsor Contributor Author

paolobarbolini commented Jun 4, 2022

Changed it. I didn't realize when the issue came up lettre 0.10.0-alpha.1 hadn't even been released yet, so it was still using the ancient email builder. This 0.10 release has been in the making for a long time 😄

@dani-garcia dani-garcia merged commit 6b6f5b8 into dani-garcia:main Jun 4, 2022
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.

None yet

3 participants