Right now, bugzilla requires that the machine it is running also be running an SMTP server. It would be nice if there were a configuration item to define which server and which port bugzilla should connect to, rather than trusting that the user has installed Sendmail correctly.

this is possible using any number of perl sendmail modules, but that would involve dragging in more dependencies into the main product

->Bugzilla product...

It may be convenient to group the SMTP server setting with that server setting of each account.

Reassigning all of my "future" targetted bugs to indicate that I'm not presently working on them, and someone else could feel free to work on them.

Reassigning all of my "future" targetted bugs to indicate that I'm not presently working on them, and someone else could feel free to work on them. (sorry for the spam if you got this twice, it didn't take right the first time)

Net::SMTP is a good, server-agnostic abstraction of the SMTP protocol, and it seems popular whenever the topic of moving away from sendmail dependency is brought up. So here it is. I'm more intersted in seeing what everyone has to say first. I tested this patch briefly and it seems OK. If it works for everyone, well, maybe we should use it. I'll poke at it a little more in the coming days. Two possible issues with this patch that I will enumerate before someone else finds them: 1. There is no more sendmailnow. Deferred mail delivery is no longer available if you use this. I was talking to justdave and he mentioned the idea of building an internal queue for bugmail, which would take out a big chunk of the need for that. If mail queues are a priority because of this, I can create a new bug for that and work on it or, better, delegate it. 2. If multiple messages are sent, one session is opened for each one, in sequence. It goes against my instincts, but I decided to not have single sessions in this on the off chance that one malformed address or message (or who-knows-what) trashes the single session. At least the rest of the mail gets through. Also, the gain for having one session per message should be marginal on most installations. I'm writing this with an ulterior motive. I want to get inbound mail worked into the main distribution. I want to use the Bugzilla::Mail directory (created here), and this seemed simple enough (famous last words) to throw in while I'm at it. And it might help give email reform a little nudge along its way.

Hmmm.... I like. 84876 is definitely trying to bite off too much at once. Baby steps are the way to do it. Initiating a TCP connection actually seems to take a fair bit of time on most sites, due to tcpwrappers checking and so forth. What I'd suggest doing (long term -- what you have on here now should be fine until we get the back-end queueing in place) is expand on the error checking within the mail sending, and as long as no errors occur, go ahead and keep sending on the same connection. Perhaps add a param for how many messages to attempt to send in the same session, since a lot of servers have limits to prevent spamming. Default should be something sane line 20 (I hear most cut you off at 25). Once you've sent that many, drop the connection and reopen it. If any error at all is encountered during sending of the message, drop the connection after cleaning up, then reopen for the next message. As mentioned, that's not worth trying to get into this one. Startup time for sendmail on the command line is probably about as bad on a lot of systems right now, and we do invoke that once per message already now.

Whoops. There was a typo in Flag.pm that caused it to not compile. Also, an issue I've already found: checksetup isn't checking for Net::SMTP yet. I will add that soon. Don't consider this a finished patch yet. I'm just looking for an answer as to whether this is the right way to go.

(In reply to comment #7) > Hmmm.... I like. Cool. > Initiating a TCP connection actually seems to take a fair bit of time on most > sites, due to tcpwrappers checking and so forth. What I'd suggest doing (long > term -- what you have on here now should be fine until we get the back-end > queueing in place) is expand on the error checking within the mail sending, and > as long as no errors occur, go ahead and keep sending on the same connection. > Perhaps add a param for how many messages to attempt to send in the same > session, since a lot of servers have limits to prevent spamming. Default should > be something sane line 20 (I hear most cut you off at 25). Once you've sent > that many, drop the connection and reopen it. If any error at all is > encountered during sending of the message, drop the connection after cleaning > up, then reopen for the next message. You're right. If it checks its errors more intelligently, it could pull that off. Also, as much as it pains me to say it, I think I'd like to see an smtp object with send_message as a method. That should make checking for error messages easier, since right now all we get is the return value of send_message. > As mentioned, that's not worth trying to get into this one. Startup time for > sendmail on the command line is probably about as bad on a lot of systems right > now, and we do invoke that once per message already now. Well, um, actually, some of the invocations of sendmail ran once for a list of cc's, and I broke those out into individual sessions. I think I'd like to see this accept an array of to: addresses to simplify sending to multiple addresses. Again, though, this is just a baby step.

What do we do about 4xx error codes??

bah. [Fri 22:27:03] <joel> justdave: we will probably need to keep the "user last got mail about this bug" entry if we start handling SMTP 4xx replies [Fri 22:27:13] <joel> otherwise we wont know to try again [Fri 22:27:35] <justdave> aw crap. [Fri 22:27:46] <joel> queuing [Fri 22:27:46] <justdave> forgot about that. sendmail just queues it if it can't get through :) [Fri 22:28:03] <justdave> if we go to direct SMTP, we're going to need to do our own queueing [Fri 22:28:22] <joel> yup The queuing stuff is either going to need to go in before this, or as part of this.

(In reply to comment #11) > bah. > > [Fri 22:27:03] <joel> justdave: we will probably need to keep the "user last got > mail about this bug" entry if we start handling SMTP 4xx replies > [Fri 22:27:13] <joel> otherwise we wont know to try again > [Fri 22:27:35] <justdave> aw crap. > [Fri 22:27:46] <joel> queuing > [Fri 22:27:46] <justdave> forgot about that. sendmail just queues it if it > can't get through :) > [Fri 22:28:03] <justdave> if we go to direct SMTP, we're going to need to do our > own queueing > [Fri 22:28:22] <joel> yup > > > The queuing stuff is either going to need to go in before this, or as part of this. Bah is right. OK, well, we can have a table with the pertinent information-- from, to, message, and last-attempted-timestamp, and use that for generic queueing. Dealing with messages that wind up in the queue could either be a matter of running a cron job, having a maintenance option for admins that sends pending mail, or have each following smtp call try re-sending up to a certain number of pending messages. Have these all happen only after a certain interval, of course (15 min. minimum?)-- then bounce anything that sat in the queue for too long get bounced to the administrator. And this was going to be *so* easy, too. Actually, some of the above might be fairly simple. Then we add a do-not-queue option for routines that handle it themselves, like bugmail.

So we've been having discussions in IRC about queueing mail, and we almost have a consensus or two about this. Almost. Here I go. 1. The current (local sendmail executable) implementation eats mail if something happens to that local sendmail. 2. It is assumed in most cases, especially because history has required sendmail to be installed locally, that the mail server people will be using will be the local host 3. The effect of the local mailserver being down in this implementation is the same as if the sendmail program became unavailable, if you squint your eyes and tilt your head a little, that means a new problem isn't introduced. Now, I'm not saying that an internal mail queue wouldn't be a feature that we need, but does it hold this patch up, or should we make a new bug for that one just to keep everything nice and bite-sized?

The previous patch didn't have checksetup looking for Net::SMTP.

Comment on attachment 151971 [details] [diff] [review] Net::SMTP patch (with checksetup) I think this might be ready for prime time.

In checksetup.pl the entry in the %ppm_modules table is mission for new module.

(In reply to comment #16) > In checksetup.pl the entry in the %ppm_modules table is missing for new module. you need: 'Net::SMTP' => 'libnet',

Comment on attachment 151971 [details] [diff] [review] Net::SMTP patch (with checksetup) If the return address it totally bogus and the smtp server rejects it, this carps with an unhelpful error message

(In reply to comment #18) > (From update of attachment 151971 [details] [diff] [review]) > If the return address it totally bogus and the smtp server rejects it, this > carps with an unhelpful error message I didn't alter any of the error conditions. It's the same case as the local sendmail executable rejecting. I don't think there is any difference in behavior.

Also, kind of an afterthought: It is possible that Net::SMTP is more sensitive to bad addresses, but the helpfulness of the error message (or lack thereof) is no different than it was before. I'm going to test the old code to see if this is a new problem or not.

This addresses the above. Have at you.

Comment on attachment 152037 [details] [diff] [review] Net::SMTP patch (with more checksetup and better error reporting) Missed MailPassword in CGI.pm Replace the die() call with ThrowCodeError and define the appropriate error code. While we're at it, do we want to use example.com as the default or should we try to populate it using Sys::Hostname which does seem to be standard.

> While we're at it, do we want to use example.com as the default or should we > try to populate it using Sys::Hostname which does seem to be standard. we need the domain name, not the hostname, and there's no standard cross-platform way to get the domain name. i think that "localhost" is a better default.

If Net::SMTP suport SMTP server authentication why not adding a login and password parameters in the config file to use it ?

(comment 22) > Missed MailPassword in CGI.pm Fixed. > Replace the die() call with ThrowCodeError and define the appropriate error > code. There are some places where that isn't appropriate, like whineatnews, and some places where mail delivery errors are being ignored. Otherwise, done. > While we're at it, do we want to use example.com as the default or should > we try to populate it using Sys::Hostname which does seem to be standard. See below. (comment 23) > we need the domain name, not the hostname, and there's no standard > cross-platform way to get the domain name. > > i think that "localhost" is a better default. That's what I have it using now. It should be fairly adequate for the frighteningly large number of un(der)-configured Bugzilla installations. (comment 24) > If Net::SMTP suport SMTP server authentication why not adding a login > and password parameters in the config file to use it ? Is this something that is needed, or something that would be a nice feature? Most (?) SMTP servers aren't configured to require authentication, or at least can be configured to not require it from certain hosts. If it's all the same, I'd like to add features like that at a later date, especially after editparams gets its much-needed overhaul.

> > If Net::SMTP suport SMTP server authentication why not adding a login > > and password parameters in the config file to use it ? > Is this something that is needed, or something that would be a nice feature? > Most (?) SMTP servers aren't configured to require authentication, or at least This is just a request but my ISP is now using authentication to protect against spam. and it is probably not the only one.

> This is just a request but my ISP is now using authentication to protect > against spam. > > and it is probably not the only one. The current release version of Bugzilla still requires you to have a locally-installed copy of Sendmail, so if you still have that, your upstream ISP's SMTP requirements are probably not an issue since you can connect to localhost. I think this and other SMTP features should be included, just not in this patch.

Comment on attachment 154150 [details] [diff] [review] Cleaned a little bitrot >Index: defparams.pl >=================================================================== >+ name => 'mailhost', >+ desc => 'Host or IP address to use for outbound mail delivery', >+ type => 't', >+ default => 'localhost' How about "smtphost" and "outbound SMTP mail delivery"? (just to be ready for the next generation mail protocol) >+ name => 'mailtimeout', >+ desc => 'Number of seconds to wait on SMTP connections before giving up', Also, "smtptimeout" >+ name => 'emaildelivery', 1) "outboundmailenabled"? (or something like that; booleans should sounds like booleans) >+ desc => 'Is bug and other email from Bugzilla currently \'on\'?' . >+ '(site-wide toggle)', 1) A space between the lines is missing. 2) The desc is confusing... "Is outbound email from Bugzilla currently enabled?" perhaps? >Index: globals.pl >=================================================================== >- my $msg = PerformSubsts(Param("voteremovedmail"), >+ my $msg = PerformSubsts(Param("voteremovedmail"), > \%substs); Nit: Realign the \%substs under the Param. >Index: Bugzilla/Flag.pm >=================================================================== >+ Bugzilla::Mail::Outbound::send_message($mailparams) or >+ ThrowCodeError("smtp_error", >+ { "error" => $Bugzilla::Mail::Outbound::error }, >+ ); ... I wonder, since we're repeating this ThrowCodeError in pretty many places, shouldn't we have a send_message_or_throw_up version which would have the error handling built in? Also, since we're practically always constructing the mail hash with the constant three params, how about if the wrapper method took three (or perhaps even two - from value seems to be pretty constant here) scalar params instead of that full hash? I have nothing against the current param structure, but constructing the hash manually every time if unnecessarily verbose. I'd love it if we could simplify this mail stuff once and for all. See my comment about CreateMessage sub in bug 84876 comment 158. >Index: Bugzilla/Token.pm >=================================================================== >+ Bugzilla::Mail::Outbound::send_message($mailparams); What about failure here? (repeat two more times for this file) >Index: Bugzilla/Mail/Outbound.pm >=================================================================== >+# ...and it sends a single message in a single SMTP session Nit: Space after "..." >+# I like the idea of having a separate session for each outbound message >+# because a single failure in a series of messages has a chance of messing >+# things up for the whole connection, thereby poisoning the whole queue. Nit: I agree, but don't write "I like" in source, because nobody knows who wrote it. Just say "The idea here is to have a separate..." >+# It is assumed that a nearby MTA is being used, so the performance gains for >+# using a single connection are probably marginal. Nit: "reusing the same connection" is way more clear. Also use "would be" instead "are" so it's absolutely clear what you're talking about. >+ unless (&::Param('emaildelivery')) { >+ return 1; >+ } Err... This is a matter of opinion, but in a case like this I'd find "if (!...)" much more readable than "unless (...)". >+ # As for the validity of their content, that's going to be the MTA's >+ # problem, since we have to check it for error messages no matter what, >+ # and there are innumerable strange things in email that actually work at >+ # some sites. Eww. How about: We don't validate the parameters but rather leave it up to the MTA to avoid coding for site-wide specifics here. We'll check for SMTP errors later on. >+ for (qw(to from message)) { >+ unless (length($params->{$_})) { >+ $missing_params = join("", ($missing_params, $_)); >+ } >+ } Umm... This is pretty verbose (and besides, it causes warnings when length hits an undef value). How about just my @required_params = qw/to from message/; my $missing_params = join(", ", grep { !length($params->{$_} || '') } @required_params); >+ $error = $smtp->code() . " " . $smtp->message(); A single space should probably suffice (Internet protocols usually only have that one space separating error codes and messages anyway). You should also fix whine.pl. As you can tell, those were pretty small problems. Looks very good indeed :-)

+ unless ($smtp) { + $error = "Could not create a Net::SMTP Object"; + return 0; + } shouldn't the error message include $!

See also bug 270611 on the sendmail path problenm. Not trying to preempt anything here on on bug 84876, just trying to bite off a tiny piece to start with.

Now that bug 84876 is a meta-bug once again, does anybody have any code that accomplishes this? It would be nice to use SMTP, although it's definitely more difficult to deal with error conditions than with sendmail.

And you know, maybe I should stay out of this, bug doesn't it seem like bug 65101 would be an easier way to accomplish what the summary of this bug says the goal is? (That is, if Mail::Sendmail is still a current, useful module from CPAN.)

OK, I just actually *read* all of bug 84876. It looks like at one point we were going to use Mail::Mailer and Mail::Send (both part of the MailTools package), which are modern and actively maintained (last CPAN release on 2004-11-24). Those seem like good abstractions that also handle interfacing with Net::SMTP if necessary. I know that bbaetz had some issues with Mail::Send when he wrote it into his own patch, but it's been a few *years* since then, and perhaps some of thos issues have been resolved since then? With the simplicity of Mail::Send and Mail::Mailer, I'd imagine this would be all somewhat easier than having Bugzilla manually do the SMTP handling?

*** Bug 277106 has been marked as a duplicate of this bug. ***

OK, bug 277437 appears to be an exact duplicate of this bug, but went about fixing it in a different way (and there is a "review+" patch there). Should we dupe this to that one?

Well, it would be nice to have this one still show up in the bug list when we're doing release notes. So let's just say that that one will fix this one.

(In reply to comment #37) > Well, it would be nice to have this one still show up in the bug list when we're > doing release notes. > > So let's just say that that one will fix this one. I'm not sure why that one wasn't resolved as a duplicate of this one and the work wasn't done here.

Added to the release notes in bug 286274.