353711 - Move to Email:: modules for email sending

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[Max Kanat-Alexander]

I think it might be a good idea to change from using the Mail:: modules, to using the Email:: modules.

There are a few reasons, the first being that the Email:: modules are simple. 

I'm already using them for the inbound email interface, so we'd eliminate some duplication of required modules there.

Also, when Email::Send fails, it returns a value with more detail on why it failed, instead of Mail::Mailer's "undef error in CGI.pm".

Comment 1

[:glob ✱]

> Also, when Email::Send fails, it returns a value with more detail on why it
> failed, instead of Mail::Mailer's "undef error in CGI.pm".

that alone makes it worth while :)

Comment 2

[Max Kanat-Alexander]

Okay. :-) I'm actually working on this now. I've been able to delete most of the code in Bugzilla::Mailer, since the Email:: modules also provide an encoding_set method and an easy way to iterate through the various MIME parts of a message.

Comment 3

[Max Kanat-Alexander]

Attachment: Work In Progress

Okay, here's what I have so far. It does everything correctly except encode the headers.

I also haven't written the parameter migration code yet.

Comment 4

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. Glob, do you think you'll have time to look at this?

This is pretty straightforward. I've tested it with various sorts of UTF8 data, and it works fine. I haven't tested SMTP, Qmail, or Windows Sendmail.

Comment 5

[:glob ✱]

> Okay, here we go. Glob, do you think you'll have time to look at this?

Email-MIME-Modifier isn't on activestate's ppm repository, so i can't install or test :(  Email-MIME is, as is Email-Send.

is encoding_set small enough to inline?


so i can't really test it, so here's some comments instead of a review..

  open TESTFILE, '>>', $filename;
  print TESTFILE "\n\n---\n\n" . $email->as_string;
  close TESTFILE;


  $part->encoding_set('quoted-printable') if !is_7bit_clean($part->body);

you've removed the code that uses base64 if more than half the message isn't 7bit clean.  such emails will be larger than their base64 encoded counterpart.


wouldn't it be better to use File::Handle?

  choices => [Email::Send->new()->all_mailers(), 'None'],

we're relying on the case of the items returned here, i'm worried that if it changes from Sendmail to SendMail it'll break things.

also, all_mailers() calls "which", even on windows, which results in an error during checkout.

heh, here's the problem -- Email::Send::Qmail

  sub is_available {
    return   `which $QMAIL
           ? success
           : failure;
  }

oh, wait, Email::Send::Sendmail assues the path seperator is ':' on all platforms.  should be using File::Spec->path();

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> Email-MIME-Modifier isn't on activestate's ppm repository, so i can't install
> or test :(  Email-MIME is, as is Email-Send.

  Oh, all right. Is it possible to build it for Windows, the same way we do for the other things in the landfill ppm repository?

> is encoding_set small enough to inline?

  I'd really rather not, since it's performing its magic in ways that make the general use of Email::MIME much better.

> you've removed the code that uses base64 if more than half the message isn't
> 7bit clean.  such emails will be larger than their base64 encoded counterpart.

  That's fine with me.

  Headers have to be in the same format as the message, technically, and there are many mailers that don't understand headers encoded in Base64.

  We really don't send very large emails, usually, to the point that this would be an issue.

> wouldn't it be better to use File::Handle?

  Yes--I used IO::File, but it threw a weird taint error for no reason inside of IO::File->new. So now I'm using the normal filehandle methods.

>   choices => [Email::Send->new()->all_mailers(), 'None'],
> 
> we're relying on the case of the items returned here, i'm worried that if it
> changes from Sendmail to SendMail it'll break things.
> 
> also, all_mailers() calls "which", even on windows, which results in an error
> during checkout.

  Okay, good point. I'll change this to be a manual list. That will eliminate NNTP from the list anyhow, which we don't support yet, really.

> oh, wait, Email::Send::Sendmail assues the path seperator is ':' on all
> platforms.  should be using File::Spec->path();

  That's fine--$ENV{PATH} is deleted by Bugzilla, and replaced right before the call to Email::Send, in Bugzilla::Mailer.

Comment 7

[Max Kanat-Alexander]

Attachment: v2

Okay, so it turns out that all the problems you encountered are fixed in a later version of Email::Send. They use File::Spec-path now, and don't use "which."

You should be able to get all the modules you need from here:

http://theoryx5.uwinnipeg.ca/ppms/

In fact, I bet we could recommend that repo instead of landfill, from now on.

Comment 8

[:glob ✱]

> http://theoryx5.uwinnipeg.ca/ppms/
> In fact, I bet we could recommend that repo instead of landfill, from now on.

good idea.

fodder for another bug, but we'll have to get theoryx5 used in preference of activestate:

  ppm> rep
  Repositories:
  [1] ActiveState Package Repository
  ppm> rep add theoryx5 http://theoryx5.uwinnipeg.ca/ppms/
  Repositories:
  [1] ActiveState Package Repository
  [2] theoryx5
  ppm> rep up 2
  Repositories:
  [1] theoryx5
  [2] ActiveState Package Repository

because activestate's has v2.05 of Email::Send, so it'll always be installed in preference of the later version that theoryx5 has.

Comment 9

[:glob ✱]

Comment on attachment 239991 [details] [diff] [review]
v2

oops:

syntax error at Bugzilla/Install/Requirements.pm line 69, near "}"

oops:

Undefined subroutine &Bugzilla::Mailer::ThrowCodeError called at Bugzilla/Mailer.pm line 107.

editparams.cgi:

[Thu Oct 5 21:47:55 2006] editparams.cgi: Use of uninitialized value in split at D:/Perl/lib/File/Spec/Win32.pm line 111. [Thu Oct 5 21:47:55 2006] editparams.cgi: Use of uninitialized value in split at D:/Perl/lib/File/Spec/Win32.pm line 111.

this is because we're deleting $ENV{'PATH'} in Bugzilla::init_page().
setting it to an empty string instead of deleting it fixes this.

createaccount.cgi:

"There was an error sending mail to '[e-mail]':No valid recipients"

that's using smtp with a valid smtp server, all other settings at default.

(time passes)

it took me quite a while to track down the actual error here.  the smtp server is returning:
  504 <bugzilla-admin-daemon@localhost>: Sender address rejected: need fully-qualified address

it's a shame /that/ error isn't returned by Email::Send.

it would help a whole lot if there was some what of enabling Net::SMTP's Debug mode .. and wrapping $mailer->send() in <pre>


once that was fixed, smtp worked without issue.
windows sendmail also worked.

Comment 10

[Max Kanat-Alexander]

Attachment: v3

Okay, this fixes the issues that you mentioned.

I looked into the error message thing, and it's a fundamental problem with the way that Email::Send::SMTP is structured, and the way that modern mailservers work.

Send::SMTP does try to send a nice message if the MAIL FROM command fails, but most servers *always* accept the MAIL FROM command, and only fail after you put RCPT TO. Since there can be many RCPT TO commands, Email::Send only tells you if they *all* failed. However, if there was only one RCPT TO command, I agree that it would be nice for the module to send you the returned message.

It's still better than Mail::Mailer's totally cryptic error.

And yeah, I agree--in a future bug, we could add a way of turning on Debug for Net::SMTP. That's definitely possible.

I filed a bug in the perl RT about it:

http://rt.cpan.org/Public/Bug/Display.html?id=21990

Comment 11

[Max Kanat-Alexander]

Attachment: v3.1

I fixed the bitrot, and I also improved the error message slightly.

Comment 12

[:glob ✱]

Comment on attachment 244372 [details] [diff] [review]
v3.1

r=glob

works well with SMTP and sendmail on Win32.


>+   choices => [Email::Send->new()->all_mailers(), 'None'],
>+   default => 'Sendmail',

NNTP really should be filtered out of that list, fix on check-in.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Hmm, looks scary at this stage of the game.

Comment 14

[Max Kanat-Alexander]

I'll file a separate bug for removing NNTP and IO from the list.

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.52; previous revision: 1.51
done
Checking in Bugzilla/Config.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config.pm,v  <--  Config.pm
new revision: 1.69; previous revision: 1.68
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.58; previous revision: 1.57
done
Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/Config/MTA.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/MTA.pm,v  <--  MTA.pm
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.26; previous revision: 1.25
done
Checking in template/en/default/admin/params/mta.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/mta.html.tmpl,v  <--  mta.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.88; previous revision: 1.87
done