go-gomail / gomail

The best way to send emails in Go.
MIT License
4.33k stars 573 forks source link

Initialize msg.encoding before applying msg settings #8

Closed OVYA closed 9 years ago

alexcesaro commented 9 years ago

The tests fail with this PR. What is the issue you're trying to fix?

OVYA commented 9 years ago

a msg.SetAddressHeader in a MessageSetting func generates an "invalid memory address or nil pointer dereference" error. I'll push a best fix…

alexcesaro commented 9 years ago

What is the MessageSetting you want to add? Before adding more code to the package. I need to understand your use case and check that it is a valid one.

Using a MessageSetting is a way to set unexported fields when building a message. Since msg.SetAddressHeader is an exported function, it is not supposed to be in a MessageSetting.

OVYA commented 9 years ago

Can you try this simple code please ?

func TestMessageSetting(t *testing.T) {
    NewMessage(func(msg *Message) {
        msg.SetAddressHeader("From", "foo@bar.com", "Renée")
    })
}

My pull request https://github.com/go-gomail/gomail/pull/9 fix the problem and pass the unit tests.

OVYA commented 9 years ago

Here is a real use case :

package main

import (
    "github.com/go-gomail/gomail"
)

func getSetting(domain string) gomail.MessageSetting {
    return func(msg *gomail.Message) {
        msg.SetAddressHeader("From", "foo@"+domain, "Renée")
        msg.SetHeader("Reply-To", "no-reply@"+domain)
        msg.SetHeader("Return-Path", "bounce@"+domain)
    }
}

func main() {
    gomail.NewMessage(getSetting("foo.fr"))
    gomail.NewMessage(getSetting("bar.fr"))
    // etc
}
alexcesaro commented 9 years ago

Ok I see.

However as I said this is not the intended use of MessageSetting. MessageSetting should only be created in the Gomail package to provide settings that users of the package cannot do like modifying unexported fields of Message. Users of the package are not supposed to create MessageSetting.

In you case you should do something like:

package main

import (
    "github.com/go-gomail/gomail"
)

func createMessage(domain string) *gomail.Message {
    msg := gomail.NewMessage()
    msg.SetAddressHeader("From", "foo@"+domain, "Renée")
    msg.SetHeader("Reply-To", "no-reply@"+domain)
    msg.SetHeader("Return-Path", "bounce@"+domain)

    return msg
}

func main() {
    fooMsg := createMessage("foo.fr")
    barMsg := createMessage("bar.fr")
    // etc
}

That is much simpler.

OVYA commented 9 years ago

I understand your point of view. However the possibility to the end user to produce an "invalid memory address" seems tricky a bit, to my mind. Do the best… Regard.