59351 - Move calls to sendmail to a central place

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P2)

Description

[Johan Vromans]

Currently, the facilities to send email messages are hardwired to the sendmail
program, and distributed through the sources.
The attached patch:
  - moves all sendmail handling code to globals.pl, routine MailMessage.
  - additionally, this routine is prepared to use PostFix instead of sendmail.
  - replaces all calls to sendmail to use the new routine.

As a result, changes to the MTA, for example to use PostFix instead of sendmail,
are now easy to apply, in just one central location.

Remark1: Some parts of the old code did not check for sendmail errors. The patch
keeps this 'feature'? intact.
Remark2: A BugZilla config param could be added to select PostFix instead of
sendmail.

Comment 1

[Johan Vromans]

Attachment: Patch to restructure bugzilla's sendmail interface

Comment 2

[Dave Miller [:justdave]]

Getting this on the radar.  I like this patch.  Not sure if we want to keep the 
pipes or use a Perl module that handles mail based on your Perl config (there's a 
couple bugs out there for that with patches, too).

Comment 3

[Stephan Niemz]

We could even do both: modularize the code to make it easier to maintain
(this patch) and use the proper Perl module (patch for bug 65101).

Comment 4

[Dave Miller [:justdave]]

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

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass moving to new product Bugzilla...

Comment 6

[Gervase Markham [:gerv]]

I'd go for bug 65101 as well.

Gerv

Comment 7

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 8

[Christian Reis]

Johan, can you get this patch updated for 2.16? I will be available to review
this for the following week if you can, I _really_ like it.

Comment 9

[Dave Miller [:justdave]]

Changing default owner of Email Notifications component to JayPee, a.k.a.
J. Paul Reed ([e-mail]).  Jake will be offline for a few months.

Comment 10

[David D. Kilzer (ddk)]

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

Comment 11

[J. Paul Reed [:preed]]

Resolving all bugs complaining about sendmail-specific/hard-coded MTA problems
as dupes of bug 84876.

Fixing these issues is inclusive in the fix for that bug now.

Cc yourself to that bug if you care to watch the madness.

*** This bug has been marked as a duplicate of 84876 ***

Comment 12

[Dave Miller [:justdave]]

clearing milestone on all DUPLICATE/WONTFIX/WORKSFORME/INVALID so they'll show
up as untriaged if they get reopened.  "Jiminy Cricket!" for the filters (and I
don't care if it's spelled wrong ;)

Comment 13

[Max Kanat-Alexander]

Now that bug 84876 has been acknowledged as impossible to resolve, and has once
again become a meta-bug, this is a valid and good goal to have in an individual bug.

Comment 14

[Max Kanat-Alexander]

Attachment: Modern patch to do only what the summary says

OK! Here's a patch to do EXACTLY and ONLY what the summary of this bug wants.
It was the FASTEST patch I've ever written in my life; it was practically just
a dumb series of vi commands to get this thing. :-)

Amazing that it's taken four years.

Anyhow, this should be relatively simple to review -- it's "obviously correct,"
and I do assert that I tested it and it behaves properly.

It does touch everybody's code in the whole world, though, so I figured
justdave might want to check it out.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 169349 [details] [diff] [review]
Modern patch to do only what the summary says

Arrggh. Because of the broken Test Installs on landfill (they try to use
:pserver:justdave to access cvs.mozilla.org for checkoiut), this patch is
incomplete. I'll re-post an actually complete patch.

Comment 16

[Max Kanat-Alexander]

Attachment: Modern patch (corrected)

OK, this patch is actually correct, and complete.

Comment 17

[Dave Miller [:justdave]]

Comment on attachment 169351 [details] [diff] [review]
Modern patch (corrected)

OOoooohhhhhh...   centralizes the freaking $enableSendMail check, so it
actually stops ALL mail and not just bugmail, too. :)  I like.
Haven't had a chance to test it yet though, so not marking review yet.

Comment 18

[David Lawrence [:dkl]]

(In reply to comment #16)
> Created an attachment (id=169351) [edit]
> Modern patch (corrected)
> 
> OK, this patch is actually correct, and complete.


I like it but would go one step further. Remove $enableSendMail from BugMail.pm
and replace with a param (i.e. Param('enablesendmail')). This way it can be
switched on and off from remote rather that having to hand edit the code.

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> I like it but would go one step further. Remove $enableSendMail from BugMail.pm
> and replace with a param (i.e. Param('enablesendmail')). This way it can be
> switched on and off from remote rather that having to hand edit the code.

See bug 178370, which this bug blocks.

Comment 20

[Gervase Markham [:gerv]]

Comment on attachment 169351 [details] [diff] [review]
Modern patch (corrected)

r=gerv, except that "MessageToMTA" seems a bit specific. One day, this function
call might just call a Perl email-sending module. Can we just call it
sendMessage()?

This also clashes with my bug 73665 patch :-(

Gerv

Comment 21

[Shane H. W. Travis]

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

Comment 22

[Max Kanat-Alexander]

(In reply to comment #20)
> (From update of attachment 169351 [details] [diff] [review] [edit])
> r=gerv, except that "MessageToMTA" seems a bit specific. One day, this 
> function call might just call a Perl email-sending module. Can we just call it
> sendMessage()?

  Yeah, I was concerned about that, too. The reason that I called it
MessageToMTA was that we already have a function called "Send" in BugMail.pm,
and I didn't want to confuse people.

  When we have a perl mail-sending module, like Mail::Send, it will actually
still be sending to the MTA, so I figure it's an accurate statement. That is,
the end result of MessageToMTA will always be that the message gets to the MTA.

> This also clashes with my bug 73665 patch :-(

  Oh. I hope it's not a terrible clash, and that it's easy to fix.

Comment 23

[Vlad Dascalu]

There's a space identation issue below, but otherwise looks good.

+sub MessageToMTA ($) {
+   my ($msg) = (@_);
+
+    my $sendmailparam = "";

Comment 24

[Gervase Markham [:gerv]]

> When we have a perl mail-sending module, like Mail::Send, it will actually
> still be sending to the MTA, so I figure it's an accurate statement. That is,
> the end result of MessageToMTA will always be that the message gets to the MTA.

Mail::Send contacts the remote SMTP server directly, right? Now you could argue
that this is an MTA, and it is, but I think that using messageToMTA when the
message is going to a remote MTA is confusing - that's what people normally call
"sending a message".

Don't worry about the name clash in BugMail.pm - my patch changes some of those
function names anyway. I'll make sure it's not confusing when I'm finished.

Gerv 

Comment 25

[Max Kanat-Alexander]

Hey, I don't have checkin authority. Could somebody check this in for me?

It should apply cleanly to 2.18, but I didn't actually try it on the branch, as
I recall.

Comment 26

[Gervase Markham [:gerv]]

Checked in on trunk and branch. whine.pl does not exist on the branch, so that
part of the patch was not applied.

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.218; previous revision: 1.217
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.279; previous revision: 1.278
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.37; previous revision: 1.36
done
Checking in move.pl;
/cvsroot/mozilla/webtools/bugzilla/move.pl,v  <--  move.pl
new revision: 1.28; previous revision: 1.27
done
Checking in whine.pl;
/cvsroot/mozilla/webtools/bugzilla/whine.pl,v  <--  whine.pl
new revision: 1.3; previous revision: 1.2
done
Checking in whineatnews.pl;
/cvsroot/mozilla/webtools/bugzilla/whineatnews.pl,v  <--  whineatnews.pl
new revision: 1.15; previous revision: 1.14
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.18; previous revision: 1.17
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.26; previous revision: 1.25
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.24; previous revision: 1.23
done


Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.211.2.4; previous revision: 1.211.2.3
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.270.2.4; previous revision: 1.270.2.3
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.36.2.1; previous revision: 1.36
done
Checking in move.pl;
/cvsroot/mozilla/webtools/bugzilla/move.pl,v  <--  move.pl
new revision: 1.26.2.2; previous revision: 1.26.2.1
done
Checking in whineatnews.pl;
/cvsroot/mozilla/webtools/bugzilla/whineatnews.pl,v  <--  whineatnews.pl
new revision: 1.14.2.1; previous revision: 1.14
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.13.2.2; previous revision: 1.13.2.1
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.18.2.5; previous revision: 1.18.2.4
done
Checking in Bugzilla/Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.22.2.2; previous revision: 1.22.2.1
done

Gerv

Comment 27

[Vlad Dascalu]

The docs section in which we describe how to use Net::SMTP needs to be updated.

Comment 28

[Shane H. W. Travis]

(In reply to comment #27)
> The docs section in which we describe how to use Net::SMTP needs to be
> updated.

I'm not fully conversant with this patch, but looking at the diffs it seems 
that the lines of code discussed in http://www.bugzilla.org/docs/2.18/html/os-
specific.html (specifically "2.4.1.3. Code changes required to run on win32") 
doesn't seem to have changed. What needs updating?

Comment 29

[Vlad Dascalu]

Travis, in reply to comment #28:

> doesn't seem to have changed. What needs updating?

$to is no longer available because we're sending to MessageToMTA the whole
message. The documentation is practically impossible to be updated.

Commiting bug 277437 would give qmail and SMTP support for all OSes and will
solve this documentation issue by removing the section altogether. But the hell
will be frozen if that gets approved for 2.18...

Comment 30

[Vlad Dascalu]

$to --> $person (in my previous comment)

Comment 31

[Dave Miller [:justdave]]

(In reply to comment #29)
> Travis, in reply to comment #28:
> 
> > doesn't seem to have changed. What needs updating?
> 
> $to is no longer available because we're sending to MessageToMTA the whole
> message. The documentation is practically impossible to be updated.

Aha!  This changes the perspective on that other bug that I made a dupe before
on the pretext that it wasn't needed.  This wasn't pointed out over there, but
this gives that code an excuse to exist.  On the other hand, we could just
remove that altogether from the docs, and suggest people use Byron's sendmail
replacement drop-in.  It is a lot easier than modifying code.

Comment 32

[Vlad Dascalu]

(In reply to comment #31)

> Aha!  This changes the perspective on that other bug that I made a dupe before
> on the pretext that it wasn't needed.  This wasn't pointed out over there, but
> this gives that code an excuse to exist.  On the other hand, we could just
> remove that altogether from the docs, and suggest people use Byron's sendmail
> replacement drop-in.  It is a lot easier than modifying code.

I never managed to get Byron's sendmail working. I'd still like to be able to
run Bugzilla 2.18 with a SMTP server. I use it internally in this way at my
workplace, with the hack from the docs applied, and I bet a lot of other people
do it too. If SMTP functionality would be lost, I could find myself in a
position where I'd need to research possible Bugzilla replacements. This could
loose some market-share for us.

Bug 277437 offers for a dozen liners or so a clean solution to this, that brings
also qmail and SMTP support, hacking-free, and it also works on any OS and platform.

Comment 33

[Vlad Dascalu]

s/loose/lose/ in my previous comment, per Gerv's suggestion :)

Comment 34

[Colin Ogilvie [:cso]]

This bug no longer needs documentation on the trunk, after the landing of bug
277437, which made it a paramater to send email. It probably does still need it
on the others though.

Comment 35

[Frédéric Buclin]

Per comment 34.