search

wneessen / go-mail

📧 Easy to use, yet comprehensive library for sending mails with Go
https://go-mail.dev
MIT License
679 stars 48 forks source link

WIP: client: use standard port when setting SSL/TLS #106

Closed muhlemmer closed 9 months ago

muhlemmer commented 1 year ago

See issue #105.

TODO:

  1. Update / run tests;
  2. In Client.DailWithContext() an error is potentially lost when using fallback. I have to look into appending or logging the error somehow;
  3. The default TLSPolicy is actually already TLSMandatory. However, the default port is 25. I left this unchanged as it would break package consumers depending on these defaults. Perhaps this can be better updated for the next major release?

A preliminary code review on this PR would be appreciated. And if you have suggestions for item 2, it would be welcome.

wneessen commented 1 year ago

See issue #105.

TODO:

1. Update / run tests;

2. In `Client.DailWithContext()` an error is potentially lost when using fallback. I have to look into appending or logging the error somehow;

3. The default `TLSPolicy` is actually already `TLSMandatory`. However, the default port is 25. I left this unchanged as it would break package consumers depending on these defaults. Perhaps this can be better updated for the next major release?

A preliminary code review on this PR would be appreciated. And if you have suggestions for item 2, it would be welcome.

Code looks great. I love the "port is choses automatically" parts. I've added one suggestion for a cosmetical change, but functionallity wise I think this is all fine.

For the error in 2... I was thinking of implementing a go-mail specific error handling, simliar to the SMTPError we recently introduced, as well. In that we could set up combined errors. So for now maybe accept that we might lose the fallback-error but leave the TODO in the code, so we can address this again, once we have a proper custom error handling? I would assume the fallback case only happens occasionally.

For 3. totally agreed. Let's keep that for a separate PR.

wneessen commented 1 year ago

@muhlemmer Do you think you'll be able to work on the open issue sometime soon? I'm planning on releasing the next go-mail version soon and am currently checking on open PRs if I should wait for them to be included or not.

wneessen commented 10 months ago

Since there was no response from @muhlemmer over the last year, I'll merge the current state and then will take the open tasks, so that this PR can be closed.

wneessen commented 9 months ago

Superseded by #170