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

Immutable mailer instance #63

Merged
merged 2 commits into from
Sep 1, 2015
Merged

Conversation

ggrossetie
Copy link
Member

This is a proposition to replace the configure method.
Currently the mailer instance is mutable, when it gets reconfigured, it gets reconfigured globally.
The goal is to have an immutable mailer instance. See: #51 (comment)

Principle

  • New module to provide a SMTPConfiguration from the Configuration object
    • This module can be overridden with play.modules.disabled (not sure if this the right way to do it) to provide a custom SMTPConfiguration (see samples)
  • CommonsMailer injects a SMTPConfiguration

Note: I think it would be better to rename CommonsMailer to SMTPMailer and SMTPMailer to CommonsMailer.

Pros

  • Immutable mailer instance
  • If the mailer configuration is wrong, we get an error at the start of the server (i.e. when Guice initializes objects) rather than when the code is executed
  • It's easy to (manually) instantiate a mailer with a custom configuration val mailer = new CommonsMailer(SMTPConfiguration("typesafe.org", 1234))

Cons

  • It's less straight forward to configure the injected instance

    • write custom module and provider + disable the default module and enable the custom module
  • "Cryptic" error message when the mailer configuration is wrong (because Guice is unable to create a SMTPConfiguration)

    ProvisionException: Unable to provision, see the following errors:
    
    1) Error in custom provider, java.lang.RuntimeException: host needs to be set in order to use this plugin (or set play.mailer.mock to true in application.conf)
    while locating play.api.libs.mailer.SMTPConfigurationProvider
    while locating play.api.libs.mailer.SMTPConfiguration
      for parameter 0 at play.api.libs.mailer.CommonsMailer.<init>(MailerPlugin.scala:86)
    while locating play.api.libs.mailer.CommonsMailer
    while locating play.api.libs.mailer.MailerClient
      for parameter 0 at controllers.ApplicationJava.<init>(ApplicationJava.java:18)
    while locating controllers.ApplicationJava
      for parameter 1 at router.Routes.<init>(Routes.scala:32)
    while locating router.Routes
    while locating play.api.inject.RoutesProvider
    while locating play.api.routing.Router
    Caused by: java.lang.RuntimeException: host needs to be set in order to use this plugin (or set play.mailer.mock to true in application.conf)
    
  • Not binary compatible

@ggrossetie ggrossetie force-pushed the immutable branch 2 times, most recently from 71d5b2f to a1c84c8 Compare August 24, 2015 08:23
@schmitch
Copy link
Contributor

I'm also not a guice Guy, however on my side this looks really good, since that enables configuration via the database (which is what I want) and it is also immutable.

Also it's really hard to use it to create multiple Immutable mailers (you need to do a https://github.com/google/guice/wiki/AssistedInject for that (which is hard especially without any example code, for the most people)). But I don't think that too many people will need it without having that knowledge, so we could just ignore this for a while.

For the binary incompatible change, I didn't understand the rise to 3.0 (way too early, so I think not many people are on 3.0 yet so we could also just force them to ignore 3.0 or raise to 4.0)

@ggrossetie
Copy link
Member Author

Also it's really hard to use it to create multiple Immutable mailers

It's pretty simple to do it without Guice:

val email = Email("Simple email", "Mister FROM <[email protected]>")
new SMTPMailer(SMTPConfiguration("typesafe.org", 1234)).send(email)
new SMTPMailer(SMTPConfiguration("playframework.com", 5678)).send(email)

@ggrossetie
Copy link
Member Author

I've added a detailed explanation on how to configure the mailer instance: https://github.com/Mogztter/play-mailer/blob/immutable/user-manual.adoc

@jroper If this looks good to you, I would like to proceed with this pull request and release a 4.0.0-M1 version to get user feedback on this new API.

@ggrossetie
Copy link
Member Author

@loicdescotte If you have some time to do a review that would be awesome ! 😄

@ggrossetie ggrossetie force-pushed the immutable branch 3 times, most recently from 09e1865 to cc1859b Compare August 30, 2015 11:56
val email = Email("Simple email", "[email protected]", Seq("[email protected]"))
val id = mailer.configure(Configuration.from(Map("host" -> "typesafe.org", "port" -> 1234))).send(email)
def sendWithCustomMailer = Action {
val mailer = new SMTPMailer(SMTPConfiguration("typesafe.org", 1234))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I think it adds a lot of flexibility.

@loicdescotte
Copy link
Contributor

@Mogztter I've just read the code and it seems great :)

jroper added a commit that referenced this pull request Sep 1, 2015
@jroper jroper merged commit 1375cc3 into playframework:master Sep 1, 2015
@ggrossetie
Copy link
Member Author

@loicdescotte Thanks for the review
@jroper \o/

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.

4 participants