Closed
Bug 502625
Opened 15 years ago
Closed 10 years ago
Replace Email::Send with Email::Sender
Categories
(Bugzilla :: Email Notifications, enhancement)
Bugzilla
Email Notifications
Tracking
()
RESOLVED
FIXED
Bugzilla 5.0
People
(Reporter: cmr, Assigned: LpSolit)
References
Details
Attachments
(1 file, 1 obsolete file)
8.88 KB,
patch
|
dylan
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•15 years ago
|
||
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?
Reporter | ||
Comment 2•15 years ago
|
||
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."
Comment 3•15 years ago
|
||
Yes, he would recommend it. He recommended it to me already in the CPAN RT.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 4•15 years ago
|
||
(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.
Assignee | ||
Comment 5•15 years ago
|
||
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.
Comment 6•15 years ago
|
||
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.
Assignee | ||
Comment 7•13 years ago
|
||
For the record, note that Email::Sender requires Moose, which we currently don't want in Bugzilla.
Comment 8•11 years ago
|
||
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; ...
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Comment 10•11 years ago
|
||
Maybe Mail::Krohn can be a possible candidate, it was developed because the Author even dislike Moose.
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
That's a pretty good news! :)
Assignee | ||
Comment 14•11 years ago
|
||
(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
Comment 15•11 years ago
|
||
(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! ;)
Assignee | ||
Comment 16•11 years ago
|
||
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.
Comment 17•11 years ago
|
||
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 :)
Comment 18•11 years ago
|
||
Oh btw: The patch has been tested with Bugzilla 4.2.5.
Assignee | ||
Comment 19•10 years ago
|
||
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-
Assignee | ||
Comment 20•10 years ago
|
||
(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.
Assignee | ||
Comment 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
(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.
Assignee | ||
Comment 23•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: LpSolit → email-notifications
Status: ASSIGNED → NEW
Attachment #8412210 -
Flags: review?(glob) → review?(dylan)
Assignee | ||
Comment 24•10 years ago
|
||
dylan: ping? Have time to review this patch, please?
Assignee | ||
Updated•10 years ago
|
Attachment #8412210 -
Flags: review?(dylan) → review?(dkl)
Comment 25•10 years ago
|
||
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...
Assignee | ||
Comment 26•10 years ago
|
||
(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.
Comment 27•10 years ago
|
||
Fair enough, but it's not like dkl is historically any quicker to respond to review requests, in general, given his review load. :)
Updated•10 years ago
|
Target Milestone: Bugzilla 5.0 → ---
Comment 28•10 years ago
|
||
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 29•10 years ago
|
||
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+
Updated•10 years ago
|
Flags: approval?
Target Milestone: --- → Bugzilla 5.0
Assignee | ||
Updated•10 years ago
|
Assignee: email-notifications → LpSolit
Status: NEW → ASSIGNED
Comment 30•10 years ago
|
||
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 }
Assignee | ||
Comment 31•10 years ago
|
||
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 917ede9..5d96fa7 master -> master
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 32•10 years ago
|
||
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
Assignee | ||
Comment 33•10 years ago
|
||
(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.
Comment 34•10 years ago
|
||
(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.
Comment 35•10 years ago
|
||
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?
Assignee | ||
Comment 36•10 years ago
|
||
(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.
Comment 37•10 years ago
|
||
I downloaded bugzilla 4.4.6 from Mozilla directly however.
Comment 38•10 years ago
|
||
My apologies, I misread. Where can I get Bugzilla 5.0? Is it released?
Comment 39•10 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•