Closed Bug 502625 Opened 15 years ago Closed 10 years ago

Replace Email::Send with Email::Sender

Categories

(Bugzilla :: Email Notifications, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: cmr, Assigned: LpSolit)

References

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.1) Gecko/20090630 Fedora/3.5-1.fc11 Firefox/3.5
Build Identifier: 

The perl module Email::Send uses Return::Value wich is deprecated. The author recommends using Email::Sender instead.

Reproducible: Always
rjbs, would you recommend that the Bugzilla project moves to Email::Sender? And if yes, what kind of changes would be required in our code? Maybe it's backward-compatible?
Actually i don't really know but Email::Send docs say:

"Email::Send is going away... well, not really going away, but it's being officially marked "out of favor." It has API design problems that make it hard to usefully extend and rather than try to deprecate features and slowly ease in a new interface, we've released Email::Sender which fixes these problems and others. As of today, 2008-12-19, Email::Sender is young, but it's fairly well-tested. Please consider using it instead for any new work."
Yes, he would recommend it. He recommended it to me already in the CPAN RT.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #1)
> rjbs, would you recommend that the Bugzilla project moves to Email::Sender? And
> if yes, what kind of changes would be required in our code? Maybe it's
> backward-compatible?

It would be useful for you to move.  You'll have better testability, for one
thing.  You'll also be able to do better bounce detection more efficiently if
you use a VERP technique.

Anyway, it's not entirely backwards compatible, which is why the name changed. 
Among other things, the default behavior will be to throw an exception rather
than return a failing Return::Value object if a message can't be sent.  (You
can avoid this with Email::Sender::Simple, but it's not suggested.)

I don't know just how you're doing it now, but transitioning should be fairly
easy.  I'd be happy to answer specific questions as they arise.

If you don't want to switch right now, I'm still going to keep fixing bugs
found in Email::Send, but switching is a good idea anyway, for many reasons.
The single file using Email::Send is Bugzilla/Mailer.pm:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/webtools/bugzilla/Bugzilla/Mailer.pm

I hope some hacks we have there can go away, such as stuff related to UTF8.
It is more likely, given our experience with Email::Send (and Mail::Mailer before it), since Email::Sender is somewhat in its infancy, that our various hacks for Email::Send will simply be replaced with other hacks for Email::Sender, developed over time in response to various bugs or differences found in the mail-handling code. However, I think with Email::Sender it's more likely for things to be fixed or changed upstream, so though the short-term outlook may not be great, the long-term outlook could be better.
For the record, note that Email::Sender requires Moose, which we currently don't want in Bugzilla.
After a regular update I notice Return::Value sends out deprecated messages
(bugzilla whine/news cron status mails ...).

Going back to Return-Value-1.666001 fixed the issue for me but I suspect you should write a note for new installations until Email::Send is replaced in bugzilla.

http://search.cpan.org/dist/Return-Value/
==========================================
  Modules
  Return::Value 	(deprecated) polymorphic return values

http://cpansearch.perl.org/src/RJBS/Return-Value-1.666002/Changes
==================================================================
  1.666002  2012-12-24
            finally started issuing long-promised warnings


source Return-Value-1.666002: Return/Value.pm
==============================================
  ...
  Carp::cluck "Return::Value is deprecated" unless $NO_CLUCK;
  ...
(In reply to olli hauer from comment #8)
> Going back to Return-Value-1.666001 fixed the issue for me but I suspect you
> should write a note for new installations until Email::Send is replaced in
> bugzilla.

No need for a note. We will simply add $Return::Value::NO_CLUCK = 1 in Bugzilla::Mailer to disable the warnings. As said in comment 7, we won't move to Email::Sender (at least in the near future) because we don't want to depend on Moose.
Maybe Mail::Krohn can be a possible candidate, it was developed because the Author even dislike Moose.
(In reply to olli hauer from comment #10)
> Maybe Mail::Krohn can be a possible candidate

It seems to only support sendmail, its documentation is inexistent, and it has been released this week only (version 0.01), so I wouldn't trust it for an application like Bugzilla.
There is a branch on GitHub to convert Email::Sender to Moo, which may or may not make everyone here happy.  I expect it to land sometime this spring, if not sooner.
That's a pretty good news! :)
Depends on: 826678
(In reply to Frédéric Buclin from comment #9)
> No need for a note. We will simply add $Return::Value::NO_CLUCK = 1 in
> Bugzilla::Mailer to disable the warnings.

I filed bug 826678 for that. As we cannot change dependency requirements on stable branches, we have no other choice than disabling warnings. For Bugzilla 5.0, I hope Email::Sender will stop depending on Moose soon enough so that we have time to migrate from Email::Send to Email::Sender and give it enough time for testing.

I see that Email::Sender 1.3.0 has been released yesterday, which dropped its dependency on Moose in favor of Moo. But it's not marked as stable yet, so we have to wait a bit before using it.
Target Milestone: --- → Bugzilla 5.0
(In reply to Frédéric Buclin from comment #14)
> I see that Email::Sender 1.3.0 has been released yesterday, which dropped
> its dependency on Moose in favor of Moo. But it's not marked as stable yet,
> so we have to wait a bit before using it.

...but your results testing with it will help along a stable release! ;)
I see that Email::Sender 1.3.3 has been released and is marked as stable. One could start to work on this bug and see how things go.
Hi,

the attachment contains a first draft of the Email::Sender implementation.
It has been tested on Linux only so one would have to test it on Windows as well.

The patch contains a more detailed description so I'll keep it brief in the comment.
It works pretty well for me and the "Reply-To" that we set in the header template will be finally respected. :D
I tested both methods, SMTP and Sendmail, oh, Test as well.

Please test and give me some feedback :)
Oh btw: The patch has been tested with Bugzilla 4.2.5.
Comment on attachment 751255 [details] [diff] [review]
0001-A-first-implementation-draft-of-the-Email-Sender-mod.patch

Thanks for your patch, Christian! Nobody seems to have paid attention to it because you didn't ask for review (set the review flag to '?' to put it into reviewers' radar). I found it "by accident". I looked at your patch, and here are my preliminary comments.


>It has been tested with Email::Sender[2] version 1.300004 so it set it as a
>minimum required version.

Email::Sender 1.300005 fixes a missing dependency and has been released 5 days after 1.300004. You should require it instead.


>There will be no NNTP, Qmail and probably some other transports anymore.
>For now it's just SMTP, Sendmail, Test (Bugzilla internal) and None.

This seems fine for now. Other transports can probably be implemented later. rjbs would be the one to ask.


>It will add back? the smtp_ssl option wich will establish a SSL/TLS connection
>to the SMTP server.

Not sure what you mean here, but smtp_ssl is already implemented and Email::Sender supports SSL.


>The "smtpserver" parameter has been renamed to "smtp_server" to match with the
>naming scheme of the other SMTP parameters.

The renaming is fine except that you must move the value of the old parameter to the new one, and propose a migration path for those using NNTP or Qmail (probably to None?). See the "REMOVE OLD PARAMS" section of Bugzilla::Config::update_params().


>The "smtp_debug" parameter is gone. Sender::Email[2] has no such flag it seems.

@rjbs: is that true? Is there no debug mode with Email::Sender?


>Do we need the Maildir[3]/Mbox[4] transports by chance?

I think our own implementation of "Test" emulates Email::Sender::Transport::Mbox, so we should use it and remove our hacks if they are similar.


>Could a setup with e.g. mod_perl of fast_cgi benefit from the
>SMTP::Persistent[5] transport?

Per the documentation, it's better to use Email::Sender::Transport::SMTP::Persistent as we send several emails in a row. No need to reconnect each time.



>diff --git a/Bugzilla/Config.pm b/Bugzilla/Config.pm

>-                $param->{'smtpserver'} = $smtp;
>+                $param->{'smtp_server'} = $smtp;

You will have to fix the migration path to the new param name, see above.



>diff --git a/Bugzilla/Config/MTA.pm b/Bugzilla/Config/MTA.pm

>+  {
>+   name => 'smtp_ssl',
>+   type => 'b',
>+   default => 0,
>+  },

smtp_ssl has already been implemented. You will have to port your patch to work with Bugzilla 5.0 instead of 4.2.



>diff --git a/Bugzilla/Install/Requirements.pm b/Bugzilla/Install/Requirements.pm

>+        package => 'Email-Sender',
>+        module  => 'Email::Sender',
>+        version => '1.300004',

Should be 1.300005 to fix a missing dependency.



>diff --git a/Bugzilla/Mailer.pm b/Bugzilla/Mailer.pm

>+    # Sendmail will automatically append our hostname to the From
>+    # address, but other mailers won't.

>+    # Sendmail adds a Date: header also, but others may not.

Are these hacks still needed with Email::Sender? If yes, why executing this code even when "$method eq "Sendmail" as the comment says sendmail does it for us already?


>-        print TESTFILE "\n\nFrom - " . $email->header('Date') . "\n" . $email->as_string;
>+        print TESTFILE "\nFrom - " . $email->header('Date') . "\n" . $email->as_string . "\n";

You could simply write |say $foo| instead of |print "$foo\n"|. Bugzilla 5.0 requires Perl 5.10.1, so say() is available for free.



>diff --git a/docs/en/html/Bugzilla-Guide.html b/docs/en/html/Bugzilla-Guide.html
>diff --git a/docs/en/html/api/Bugzilla/Hook.html b/docs/en/html/api/Bugzilla/Hook.html
>diff --git a/docs/en/html/glossary.html b/docs/en/html/glossary.html
>diff --git a/docs/en/html/installation.html b/docs/en/html/installation.html
>diff --git a/docs/en/html/parameters.html b/docs/en/html/parameters.html
>diff --git a/docs/en/txt/Bugzilla-Guide.txt b/docs/en/txt/Bugzilla-Guide.txt

You must not edit any of the HTML or TXT files. Those are generated from the XML files using makedocs.pl.



>diff --git a/docs/en/xml/administration.xml b/docs/en/xml/administration.xml
>diff --git a/docs/en/xml/glossary.xml b/docs/en/xml/glossary.xml
>diff --git a/docs/en/xml/installation.xml b/docs/en/xml/installation.xml

Note that there is a pending migration work to convert XML files into RST ones, see bug 912064. Your changes in the documentation would conflict with this migration, but there is no reason to prevent this bug from being fixed if the XML to RST migration stalls.
Attachment #751255 - Flags: review-
(In reply to Frédéric Buclin from comment #19)
> >The "smtp_debug" parameter is gone. Sender::Email[2] has no such flag it seems.

The debug mode is back in 1.300010, which has been released 2 months ago. We should require this version.
Attached patch patch, v2Splinter Review
glob: could you test this, especially on Windows? It seems to work fine for me using SMTP and Sendmail on Linux.

As I said in a previous comment, using Email::Sender::Transport::SMTP::Persistent should be a better choice, but I first want to know how it works currently. Using ::Persistent will probably require us to store $transport in Bugzilla->request_cache and destroy it at the end of the script. This is not a big deal, but I can implement that after your first review.
Assignee: email-notifications → LpSolit
Attachment #751255 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8412210 - Flags: review?(glob)
(In reply to Frédéric Buclin from comment #21)
> glob: could you test this, especially on Windows?

i don't currently have a windows bugzilla environment, so this may take me a while to get around to.
(In reply to Byron Jones ‹:glob› from comment #22)
> i don't currently have a windows bugzilla environment, so this may take me a
> while to get around to.

How long?
Assignee: LpSolit → email-notifications
Status: ASSIGNED → NEW
Attachment #8412210 - Flags: review?(glob) → review?(dylan)
dylan: ping? Have time to review this patch, please?
Attachment #8412210 - Flags: review?(dylan) → review?(dkl)
This was next on Dylan's to-do list, but your choice if you want to wait for dkl to finish his other 10 reviews...
(In reply to Mark Côté ( :mcote ) from comment #25)
> This was next on Dylan's to-do list, but your choice if you want to wait for
> dkl to finish his other 10 reviews...

I was supposed to guess? I asked Dylan last week and he never replied.
Fair enough, but it's not like dkl is historically any quicker to respond to review requests, in general, given his review load. :)
Target Milestone: Bugzilla 5.0 → ---
Comment on attachment 8412210 [details] [diff] [review]
patch, v2

dylan is the correct reviewer for this bug.
Attachment #8412210 - Flags: review?(dkl) → review?(dylan)
Comment on attachment 8412210 [details] [diff] [review]
patch, v2

Review of attachment 8412210 [details] [diff] [review]:
-----------------------------------------------------------------

Confirmed working on Windows 7, running under Apache 2.2.25, mod_cgi, running Active State Perl 5.16.3. All dependencies installed with ppm.
It works with and without username and password (provided there is an smtp server on the network configured this way), with username and passwords, and with ssl.

::: Bugzilla/Mailer.pm
@@ +117,5 @@
> +        $transport = Email::Sender::Transport::SMTP->new({
> +            host  => Bugzilla->params->{'smtpserver'},
> +            sasl_username => Bugzilla->params->{'smtp_username'},
> +            sasl_password => Bugzilla->params->{'smtp_password'},
> +            helo => $hostname, 

nit: trailing newline on the line above.
Attachment #8412210 - Flags: review?(dylan) → review+
Flags: approval?
Target Milestone: --- → Bugzilla 5.0
Assignee: email-notifications → LpSolit
Status: NEW → ASSIGNED
Keywords: relnote
Comment on attachment 8412210 [details] [diff] [review]
patch, v2

Review of attachment 8412210 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Config/MTA.pm
@@ +23,3 @@
>     type => 's',
>     # Bugzilla is not ready yet to send mails to newsgroups, and 'IO'
>     # is of no use for now as we already have our own 'Test' mode.

this comment is no longer relevant and should be deleted

::: Bugzilla/Mailer.pm
@@ +164,2 @@
>          local $ENV{PATH} = SENDMAIL_PATH;
> +        eval {sendmail($email, { transport => $transport })};

nit: add space after eval's { and before }
Flags: approval? → approval+
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   917ede9..5d96fa7  master -> master
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1070640
Blocks: 1073380
I upgraded tot 4.5.6 and it fails in chdecksetup.pl because of the missing Email::Sender package.
When I try to install the package 

> perl install-module.pl Email::Sender

it installs with a number of warnings, but I can see that it is in the lib directory.

But checksetup still fails.


ENVIRONMENT OSX
(In reply to Peter Florijn from comment #32)
> it installs with a number of warnings, but I can see that it is in the lib
> directory.
> 
> But checksetup still fails.

Type this:

  perl -MEmail::Sender -wE 'say Email::Sender->VERSION'

If this fails, then the module or one of its dependencies hasn't been installed correctly.
(In reply to Frédéric Buclin from comment #33)

Module files are in lib folder, but installation is not OK.

Trying to install the module from a tar fails also because of a lot of missing dependencies.

perl -MEmail::Sender -wE 'say Email::Sender->VERSION'
Can't locate Email/Sender.pm in @INC (@INC contains: /Library/Perl/5.16/darwin-thread-multi-2level /Library/Perl/5.16 /Network/Library/Perl/5.16/darwin-thread-multi-2level /Network/Library/Perl/5.16 /Library/Perl/Updates/5.16.2/darwin-thread-multi-2level /Library/Perl/Updates/5.16.2 /System/Library/Perl/5.16/darwin-thread-multi-2level /System/Library/Perl/5.16 /System/Library/Perl/Extras/5.16/darwin-thread-multi-2level /System/Library/Perl/Extras/5.16 .).
BEGIN failed--compilation aborted.
Blocks: 1082557
On Debian jessie/sid, checksetup.pl is saying I need Email::Send, which I don't have, but I do have installed Debian's libemail-sender-perl which ought to satisfy the requirements. For example, this command;

$ perl -MEmail::Sender -wE 'say Email::Sender->VERSION'

says;

> 1.300016

Maybe checksetup.pl needs to be updated to look for Email::Sender?
(In reply to Jeremiah Foster from comment #35)
> On Debian jessie/sid, checksetup.pl is saying I need Email::Send

That's right. Because Bugzilla uses Email::Sender only since Bugzilla 5.0, and Debian has Bugzilla 4.x.
I downloaded bugzilla 4.4.6 from Mozilla directly however.
My apologies, I misread. 

Where can I get Bugzilla 5.0? Is it released?
It's not out yet; there a couple blockers that need to be fixed.  We're hoping for a release candidate in a few weeks.
Added to relnotes for 5.0rc1.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: