84876 - MTA re-write for enhanced configuration (sendmail/exim/postfix)

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P2)

Description

[Dave Miller [:justdave]]

I needed a tracking bug for this :)

Comment 1

[Dave Miller [:justdave]]

oops, I got that backwards...  and it's falsely detecting a dependency loop 
(there's another bug on that already) so you'll all get spammed by me one more 
time yet.

Comment 2

[J. Paul Reed [:preed]]

I have an uber-patch based upon Johan Vromans's (bug 59351) patch which
(attempts to) solve many of the problems discussed in bugs with the MTAConfig
keyword.

My patch builds on Vromans's by centralizing the function of sending mail into
one area; this function, in turn relies on "mail modules"; I've currently
written modules for sendmail, qmail, perl's Net::SMTP, and a debug module. With
this architecture, adding support for "new and exciting" MTAs should be very
easy (basically define a new bugzilla mail module to talk to the MTA, tell
bugzilla to use it, and that's it).

This patch addresses the various MTAConfig bugs as follows:

bug 37765 - perepherally, a module for exim could be defined; by centralizing
the mail function in globals.pl, we've also addressed some issues

bug 49893 - This is now possible via use of Net::SMTP

bug 51300 - This bug may or may not apply depending on which mail module you're
using; the sendmail module has some logic in it to determine the flags sendmail
should be called with.

bug 52782 - Problem addressed via mail function centralization

bug 52782 - Configurable via the mailmodargs parameter in localconfig

bug 59351 - This patch based upon the patch in this bug; a couple of notes to
the initial remarks: we still don't do a bunch of sendmail error checking; the
modules themselves return false upon failure... although we don't check it right
now... we probably should. :-) As for changing MTAs, that's now possible/very easy.

bug 65101 - Mail::Sendmail still requires reliance upon sendmail; my gut
reaction is if we're going to make someone install a perl module, just make them
install the MTA-agnostic Net::SMTP (which is from libnet, which gives them lots
of other goodies like Net::FTP); having said that, if there's a need/preference
to use Mail::Sendmail, either a new module can be made for it, or it can be used
to replace the current sendmail calling mail module.

--

Whew... that was a lot of text.

I don't have a running bugzilla installation that I have the luxery of tinkering
with, so I'll talk to Tara about getting access to landfill to test these
changes out.

Comment 3

[J. Paul Reed [:preed]]

Attachment: first revision of proposed MTAConfig patch

Comment 4

[J. Paul Reed [:preed]]

Attachment: New mailcore.pl file

Comment 5

[Dave Miller [:justdave]]

My comments:
  - your mail glue code assumes the site is using Param('email_suffix'). This
    won't necessarily be true.  $login could be a complete email address.
  - You make a system call to test if Net::SMTP is available.  But you also
    added a version test for it to the list of version checks.  You should be
    able to check the variable you set from the version test to tell if it's
    present without having to make another call to it.  Yes, it can be done
    without 'use' :)  The version checks do it that way.  If the version check
    succeeded, the module is loaded.

I *really* *really* like this idea.  I think this is tons better than the other 
things presented so far.

Comment 6

[Dave Miller [:justdave]]

Given the patch that's on this, removing the tracking off the summary so it'll 
show up on my bug lists.

Comment 7

[J. Paul Reed [:preed]]

Couple of things:

1. If the emailsuffix parameter is not being used, then won't it just be empty?
Won't the code $emailaddr = $login . Param('emailsuffix'); still work?

2. Well, DUH! :-) I should've done that. I'll change that in the next revision
of the patch.

I now have access to landfill and will begin testing this patch on there
tomorrow... when I'm done, I will install the patch on my 2.12 bugzilla
installation at work, and when both those look good, I'll submit a final patch
(I think we should be able to get this in 2.14).

Comment 8

[Dave Miller [:justdave]]

Re: emailsuffix - I think you're right.  I was thinking there was a boolean param 
for it, and the actual emailsuffix might still have something in it, but the 
boolean be off, but it looks like there isn't.  if there's an emailsuffix, it's 
used, otherwise it isn't.

Comment 9

[Tom Opgenorth]

Just tried to apply this patch against the 2.13 source I downloaded yesterday.  
Noticed that Token.pm still uses sendmail.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass moving to new product Bugzilla...

Comment 11

[J. Paul Reed [:preed]]

Comments noted from Tom; I need to update my tree and probably tweak the
attached patch, as this patch was from before 2.14/15.

Is 2.16 still the realistic release for this?

If so, I'll work on this some more and see if we can't get a test version
deployed somewhere; if not, Jake: can you please reassign a new target milestone?

Also, Jake: I will take this bug if you want.

Comment 12

[Jacob Steenhagen]

Enjoy :)

Comment 13

[Zach Lipton [:zach]]

Comment #1: mailcore.pl should be a module Mailcore.pm which works 
like perl modules should. Perl libraries went away when perl4 died. 

Comment 14

[Dave Miller [:justdave]]

preed: I notice a number of comments here about "this will be fixed in the next
version" but no new patches.  To reduce bit-rot, and because this is a
frequently-complained-about situation, I'd like to try to get this in for 2.16,
which we're now targetting at December 1st (which means anything major like this
needs to be in sooner than that to allow for settling time).

Comment 15

[J. Paul Reed [:preed]]

Dave:

I agree 100%; per our discussion on IRC, I'll try and get a new version of the
patch that is check-in-able by Nov. 1st so it has a month to settle.

Zack: bitch, bitch, bitch. :-) I'll see what I can do.

Comment 16

[Dave Miller [:justdave]]

This is now on the "we really want this for 2.16, but won't hold the release for
it if it's not done by then" list.

Comment 17

[J. Paul Reed [:preed]]

Attachment: second revision of MTAConfig patch

This patch sucks.

That means: it passes a perl -c, but I'll need some help patching a running
bugzilla system and actually testing it.

Also, this patch removes the sendmail -Odeferred crap... which probably isn't a
*good* idea, but I wanted to have that reviewed before it became a part of this
patch...

Comment 18

[J. Paul Reed [:preed]]

Attachment: New MailHandle.pm file

New MailHandle.pm file... replaces mailcore.pl just to make ZACH HAPPY! :-)

Well... actually, it turned out to be a lot cleaner this way... but... :-)

The same caveat for this patch goes for the above patch... it passed a -c,
folks... that's it!

This file also doesn't include a qmail-specific version, although the final one
will need to... I just a) didn't feel like writing it, and b) it's going to be
very similar to the sendmail version, because qmail includes a
not-quite-sendmail-compatible sendmail binary in a weird qmail-location.

Comment 19

[Matthew Barnson]

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

Comment 20

[Zach Lipton [:zach]]

bah, MailHandle.pm is a huge step in the right direction, but not all the 
way there (I really hate doing this). Let's have MailHandle.pm which works 
like it does now. and a  BugzillaMail/ directory which has files like:

Mailer.pm
Sendmail.pm
Qmail.pm
SomeOtherMailer.pm

We then default to Sendmail or Mailer or something and have a 
localconfig param to select the default module to send mail. 
MailHandle.pm then calls the selected module to actually send the mail.

Or we could just leave it as is... Comments?

Comment 21

[J. Paul Reed [:preed]]

Zach:

100% right thing to do... BUT... version 1.1, I'd say. I'd like to get this code
working/tested/integrated into the tree, and then go for the method you suggest.

I won't be that hard to implement what you're talking about.

Comment 22

[Gervase Markham [:gerv]]

Zach: are you planning to review this?

Gerv

Comment 23

[J. Paul Reed [:preed]]

Just an FYI, this patch is probably suffering from a bit of bit-rot; I'll be
happy to pull the current tree and update it as necessary. I thought Dave wanted
to get this into 2.16, but then no one reviewed it and I stopped pushing... I
suppose that branch is already cut.

I'd know if I were hanging out in IRC, but I'm currently home for the holidays,
so....

Let me know if I can do anything else to help...

Comment 24

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

[e-mail]: The release of Bugzilla 2.16 has been delayed until February
1 (at least).  Please do update your patch so folks can review it.

Zach: Are you planning to review it?

Comment 25

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

Comment on attachment 56998 [details] [diff] [review]
second revision of MTAConfig patch

Indeed this patch fails to apply, but only one hunk is rejected.

Comment 26

[J. Paul Reed [:preed]]

I'll look at updating/resubmitting, but if I do that, I need some guarantee that
it's going to actually be reviewed and applied... this will be the third time
I've done, and it's getting kind of tiring, especially on a student's schedule,
ya know?

Who wants to volunteer up front?

--jpr

Comment 27

[Zach Lipton [:zach]]

You have my word that I will review this within two weeks of posting and 
pressure a 2nd review as well.

Comment 28

[Gervase Markham [:gerv]]

Please re-add the review keyword when you post the new patch. Thanks :-)

Gerv

Comment 29

[Dave Miller [:justdave]]

You have until Friday this week to post a new patch if you want this in 2.16

Comment 30

[J. Paul Reed [:preed]]

Considering I have a midterm on Friday, it's not going to get done by then.

If, in the infinite wisdom of the project leaders :-) they'd like to grant
special exception status to this one patch over the three day weekend, I can
promise to have it done on Monday (which is a holiday).

Barring that, I guess it'll have to wait for 2.18.

Comment 31

[Dave Miller [:justdave]]

I'm just trying to kick some butts into gear. :)  The actual drop dead
has-to-be-in date is Feb 22, since we're aiming for March 1 now and we need a
week to kill the leftovers.

Comment 32

[J. Paul Reed [:preed]]

So in other words, I called your bluff. :-)

You'll have a patch for review by Monday... Sunday if I get off my big fat ass. ;-)

-jpr

Comment 33

[J. Paul Reed [:preed]]

Attachment: Patch, take 3

This patch is going to have to uber-checked for conflict-regressions; that is,
I "corrected" a number of conflicts in my local tree (which *was* updated), and
I corrected them... but I'm not sure if I did so correctly.

Comment 34

[J. Paul Reed [:preed]]

Okey doke... let's try this again.

I've cleaned up the "second revision" of the patch by cvs updating on tonight's
trunk and "correcting" all the conflicts.

Things that will definitely have to be scrutinized:
-- The checksetup.pl changes
-- The entire patch for regressions in recently fixed MTA-specific code
-- Stupid errors that I made because I'm really tired right now

No real changes other than the conflict management have been made to the second
rev of the patch.

This patch HAS NEVER BEEN DEPLOYED! I don't have a working bugzilla install to
play with, so that's also something that needs to be done.

Also, I would consider this an "initial" solution to the problem; there is an
entirely more elegant solution to the problem which I don't have time to code
right now.

Even still, I think it would be good to deploy this code, make sure it works,
yadda, yadda, yadda, and *then* go for the elegant solution, which really only
rearranges code, it doesn't substantially *change* code. I would like to see
this big honkin pile of code at least put through some stresses before we put
effort into making it "purddy." That's definitely 2.20 or whatever the plan
calls for now. :-)

Having said all that, if this patch looks stupid or icky, remember two things:
a) I'm not really this bad of a perl coder
b) This code has been resurrected from bit-rot three times now
and
c) I'm REALLY tired, even while writin this... so if it doesn't make sense...
well... yeah, just yell at me.

Let's see how this goes...

--jpr

Comment 35

[Bradley Baetz (:bbaetz)]

Comment on attachment 70690 [details] [diff] [review]
Patch, take 3

You need to use diff -u.
Your patch doesn't include the new file - cvs diff -N only works on cvs added
files, which requires write access.

You seem to have mixed up merges - eg the mysql version check in checksetup.pl
is from an incorrect version.

Comment 36

[Matthew Tuck [:CodeMachine]]

I believe this technique will work OK along with the email templatisation that
is being aimed for 2.18, in particular it looks like TT can output a template to
a string which can then be passed to sendmail.

I'd like confirmation that this is the best technique, and perhaps we should
think about the interaction here in case there are any issues.

Comment 37

[Dave Miller [:justdave]]

dangit, I want this in badly, but we're getting close to the wire.  Don't stop
working on it, if we get a new patch and get it reviewed before 2.16 goes RC
we'll still put it in.  But for now anything that's not a direct release goal is
getting dropped to 2.18 if it's not ready to roll.

Comment 38

[J. Paul Reed [:preed]]

I have a spiffy new version of this patch that was made against tonight's trunk
(so no merge conflicts should be present).

I'm going to need help from someone on IRC to get access to a working bugzilla
installation to apply the patch, test the 'upgrade' (since it messes w/
localconfig and checksetup.pl), and then test the Sendmail module to make sure
that we haven't *lost* any functionality.

Adding mail modules after that will be easy, so if we can get this first one
done, we'll be good to go.

If I can get some help with this on IRC on Thursday (tomorrow) night, we should
be able to get this one under the 2.16 RC-deadline, and everyone will rejoice.

Comment 39

[spyderz42]

Attachment: MIME::Lite patch

Patch that sets up bugzilla to use MIME::Lite which allows sending through a
smtp server with a one line config in globals.pl as well as using sendmail by
default. I am uncertain how to handle the DeliveryMode = deferred although I
think what I have set up will work (Have not had an oppurunity to test yet).
Please let me know  if you have any comments.

Comment 40

[J. Paul Reed [:preed]]

The MIME::Lite patch doesn't address what we do with other MTAs that might have
other requirements. It also *requires* the installation of another perl module,
which my patch does not.

Unfortunately, implementation of my patch has been put back by the fact landfill
has been hacked... so when landfill has been unhacked, I guess it'll be put in 2.18.

Comment 41

[Dave Mooney]

I don't deny that preed's patch has broader applicability. But if all it takes 
to avoid having to hack in windmail support in 10 different places is 
downloading another perl module, I'll take it. 

I'm looking forward to the templatization I'll get in 2.16, but I'm not looking 
forward to re-applying all the changes I had to make to get email working. 
Downloading another perl module will take far less time and effort.

Comment 42

[J. Paul Reed [:preed]]

My point was simply this: there's a quick way to do things, and a right way to
do things, and using MIME::Lite is the quick way... it's not the "right way,"
because it doesn't solve everyone's problem, and it requires everyone to install
a (possibly useless to them) perl module.

My patch is as ready as it's going to get, but until there's a place to deploy
it, I can't test it and get a final dist patch ready.

I'm making the assumption that it's too late to put anything in for 2.16, but
maybe the landfill hackage affected others as well and maybe that's no longer
correct... I'll find out on IRC. I've got some free time this week (and Spring
Break next week) to work on it and hopefully get it in on before the branch, or
the trunk at the very least. I'd be happy to do a patch against the 2.16-release
so that it'll be of use for those users as well.

Comment 43

[spyderz42]

I am not completely certain how exim/postfix work, however if they are just a
command line program that takes slightly different arguments from sendmail, then
MIME::Lite will fix those as well. Default sendmail setup with MIME::Lite is

MIME::Lite->send("sendmail", "/usr/lib/sendmail -t -oi -oem");

I am relatively certain exchanging exim for sendmail would work just fine. And
If it does not I am certain that MIME::Lite packages would be accepted by the
maintainer. And IMHO it would be better to patch MIME::Lite so the world can use
it rather then just patch bugzilla.

Comment 44

[J. Paul Reed [:preed]]

The following version of the patch is playing nicely and sending email on
justdave's box; if you'd like to play with it to see if you get mail (please
do), check out:

http://jdr200.justdave.net:8080/bz84876/

Create a dummy account, make some bugs, and see if you get the emails. Let me
know if you don't. It looks like this might just make it in time for 2.16;
because we now have a working copy deployed and can test/mess with it, I'm
changing the target milestone back to 2.16.

Patch follows; anyone care to r=?

-jpr

Comment 45

[J. Paul Reed [:preed]]

Attachment: Currently deployed/semi-working uber-patch (v.4!)

Comment 46

[J. Paul Reed [:preed]]

Attachment: MailHandle.pm driver base class

Comment 47

[J. Paul Reed [:preed]]

Attachment: MailHandle::Sendmail driver

Comment 48

[J. Paul Reed [:preed]]

Attachment: MailHandle::Debug driver

Comment 49

[J. Paul Reed [:preed]]

Couple of notes that I forgot to include:

-- by "semi-working" I mean that the only mail-sending functions I've directly
tested have been receiving mail on bug creation and bug editing; I did receive
the mail, though, so that's why the v.4 patch is labeled 'probably golden.'

-- The MailHandle drivers go in the MailHandle/ subdirectory

-- As soon as these two drivers are tested moreso than what I've done, I will
create the Net::SMTP driver; as you can see, the drivers are pretty simple, but
the way they're designed should give us an incredible amount of flexibility in
terms of pre-mail processing for different MTAs.

-jpr

Comment 50

[Bradley Baetz (:bbaetz)]

Why are we doing things this way instead of using one of the many perl modules
already available for doing things like this?

Comment 51

[J. Paul Reed [:preed]]

We're doing it this way because as you said there are 30 different modules to 
send mail, and not all of them do what is right for everyone.\

Some people will want to use the sendmail binary on the local machine (so, 
bugzilla should require Mail::Sendmail to be installed, right?) But then some 
people will want to use a remote server (so wait, require Net::SMTP too?) This 
method handles that problem by only requiring the module that the user wants.

Also, this allows us a clean easy interface to MTAs which allows us to do any 
pre-message processing that we may want (you'll note in the old code, there's a 
query of the Param("sendmailnow") all over the place... that's not the right 
way to do things; that stuff should be all centralized... which it is now.

If you have any specific concerns about this method, please post 'em and I'll 
address 'em.

Comment 52

[Andreas Franke (gone)]

Hm, when I tried to create an account at 
http://jdr200.justdave.net:8080/bz84876/createaccount.cgi
I got a "Document contained no data error", and no mail so far.

On the second try, the "Account Exists" message appeared. I clicked on the
"submit a request to change it" button, and then I got a mail, but in that mail
the urlbase of the tokens is still
http://cvs-mirror.mozilla.org/webtools/bugzilla ... ;-( 

Comment 53

[Bradley Baetz (:bbaetz)]

Well, Mail::Mailer would be an example, which apparently supports a test
destination, sendmail, qmail, smtp, and the unix 'mail' command out of the box.
I'd be surprised if that didn't cover about every unix sytem in existance, and
the smtp mailer covers everything else, I suspect. (The SMTP mailer does require
Net::SMTP, but so does your patch - I don't understand your objection on that point)

I don't think that 'we need to install another module' should be a concern. If
you don't have access on an instalation, installing it via cpan into the same
dir as the bugzilla installation should pick it up, although I haven't tested this.

Note that I don't have a problem with your patch, except that I get the feeling
that you're reinventing the wheel. A MailHandle.pm class is fine - we really do
need abstraction here. Its when you start writing your own output routines that
I start worrying, since I get the feeling that what you now have is identical to
Mail::Send, which is just a wrapper arround Mail::Mailer (using ->to and ->cc
rather than just taking a header hash directly, for example). It is part of the
same perl module as Mail::Mailer.

I'm not going to actually object to the patch on those grounds, but I do think
its something we want to think about. Note that this doesn't affected attachment
75762 [details] [diff] [review] (the patch), but does affect the implementation of the MailHandle class.

Something I do worry about though is that having a separate routine to print the
headers vs the body effectively means that admins can't add on extra headers via
templatisation, when we do end up doing that. Mail::Mailer doesn't seem to have
an interface to do this either. Is this a concern? Is adding extra headers
something we want to support?

Moving onto the patch itsself, why are the mail settings in localconfig, rather
than params? Apart from that, the patch itsself looks fine, just from a quick
glance. Note that .pm files shouldn't have a #! at the top.

Comment 54

[J. Paul Reed [:preed]]

Just a head's up:

I'm working on Andreas Franke's account creation problem; I've also noticed a
memory usage issue as well, which I'm trying to track down... fun fun.

-jpr

Comment 55

[J. Paul Reed [:preed]]

Ok:

--account creation bug fixed
--endless recursive call of AUTOLOAD: fixed

As for the other questions about the patch:

--I think the idea behind this whole patch was to abstract out a mail handling
class to get all the calls to sendmail out of the Bugzilla-proper code.

--We didn't use Mail::anything because when we started writing this, there
didn't seem to be a Mail::anything, and also the general feeling was that we
wanted to keep the number of required modules down. Net::SMTP isn't really
*required* unless you use it. In other words, I think the original thinking was
we wanted the pre-reqs for BZ to be the same.

--localconfig vs. Params; it was done that way because I didn't know any better,
and this way, I didn't have to create new UI hooks and such in the
editparams.cgi or whatever. I would really like to do that though, but I think
it's getting a bit late for this release; I'd like to get the actual code itself
stablized (which I think is pretty much done). I would like to concentrate on
that for the next release.

--addition of headers: yes, Yes, YES! It's discussed a bit in bug 70710 and I
know there are other bugs that reference customizable headers. I would love
input on how we might do that.

--

I guess we can discuss what to do on IRC; I made a couple of changes to the
patch you see above, so I can attach the new versions I guess, and then get it
all re-reviewed? Dave?

-jpr

Comment 56

[Dave Miller [:justdave]]

Sounds good to me, let's do it.

Comment 57

[J. Paul Reed [:preed]]

Attachment: deployed patch - v.5

Comment 58

[J. Paul Reed [:preed]]

Attachment: MailHandle.pm driver base class

Comment 59

[J. Paul Reed [:preed]]

Attachment: MailHandle::Sendmail driver

Comment 60

[J. Paul Reed [:preed]]

Attachment: MailHandle::Debug driver

Comment 61

[J. Paul Reed [:preed]]

Attachment: deployed patch - v.6

Fixed conflicts and general fallout from bug 23067. Also the 23067 patch used
the naughty way of calling sendmail directly to do its mailing.

Comment 62

[Bradley Baetz (:bbaetz)]

Comment on attachment 77193 [details] [diff] [review]
deployed patch - v.6

OK, after discussions on IRC, I'll live with using this, for now. I still think
its a bad idea, but noone else does ;)

However, you need to use params, not localconfig vars, and default $bzmailfrom
to 'bugzilla-daemon' for backwards compatability.

You should also update the default mail param to use this new mailFrom param as
a subst.

Comment 63

[Dave Miller [:justdave]]

swapping deps to make it accurate (part 1)

Comment 64

[Dave Miller [:justdave]]

swapping deps to make it accurate (part 2)

Comment 65

[J. Paul Reed [:preed]]

Attachment: deployed patch - v.7

Addresses the localconfig vs. defparam issues raised by bbaetz

Comment 66

[J. Paul Reed [:preed]]

Attachment: Updated MailHandle.pm driver base class

Modified the IsValidModule() method; the old version was flakey.

Comment 67

[Gervase Markham [:gerv]]

Attachment: Gerv's Patch v.1

Here's my entry :-)

It can use either sendmail, as now, or Mail::Bulkmail.

It has several good things about it:
1) It centralises mail sending (bug 59351)
2) It allows use of any SMTP server via Mail::Bulkmail (bug 49893, bug 65101)
3) It is therefore MTA-agnostic (bug 37765)
4) It always does the Right Thing with 'sendmailnow' (bug 51300)
5) It error-checks the mail send (bug 87801)
6) It's pretty simple as a patch - which is good for 2.16. A lot of the bulk is
text changes or simple template line removals.
7) It's low impact - sites can continue using their existing mail arrangements
without any extra or changed configuration (see the send_mail() sub for how
this works.) By default, an SMTP server is not configured, and things continue
to work as they did before. This means that if, for some horrible reason, it
turns out not to work under high load, or in some circumstances, there's a
fallback.

There is obviously potential for a lot of optimisation and use of the cool
capabilities of Mail::Bulkmail - but that can wait until after 2.16. For
example, we should be opening a single connection to the SMTP server when
sending a bunch of bugmail, and we should be using envelope_send.

Current known issues:
- processmail prints a page of error text due to something Perl doesn't quite
like about Mail::Bulkmail. It can be suppressed by a one-line patch to
Bulkmail.pm. (Change "or" to "||" on the line referenced.)

Gerv

Comment 68

[J. Paul Reed [:preed]]

I've looked through the patch, and I'm still not convinced.

-- your patch provides no support for a Debug (or (possibly not-so)similar target).

-- You made a big deal about how stupid my patch was because it still used the
sendmail binary, but then so does yours... so what's your point? With my
architecture, you can use Mail::Bulkmail if you want to (that's, in fact, not
such a bad idea... maybe use it instead of Net::SMTP), but you also aren't
limited to (sendmail, Mail::Bulkmail, the_highway).

-- You drive everything through a function called send_mail() in gloabls.pl;
this has the potential to become a very huge and ugly function when it gets more
complicated (which is why I created a new, extendable module for dealing with
mail sending issues).

As I said before, I'm not convinced... the more I think about it, using
Mail::Bulkmail as an implementation module isn't a bad idea, but I still don't
see that it *negates* the use of a clean, well separated, well designed Mail
Handling module.

I just get the feeling that with this patch, around 2.18 or 2.20, we're going to
be saying "Gee, this doesn't do X or Y or it's really hard to add Z or support
MTA Q."

As spiffy "Email Notification component owner" (which I really am by default,
since Jake left, so don't put much stock in that... I wonder what his opinion
is...), I'd vote for the MailHandle class with Mail::Bulkmail as a candidate for
MailHandle driver.

But, obviously I'm biased, and obviously there's much disagreement on this
one... so justdave is gonna have to call it.

Comment 69

[Bradley Baetz (:bbaetz)]

After looking at both patches, I no longer have a real preference one way or the
other. I like Mail::Bulkmail's header parsing stuff, but the Mailhandle stuff is
more extendable.

I'd claim that we should just merge all the individual sendmail calls into
globals.pl (to allow people to customize this for 2.16), but thats just going to
postpone the discussion and not solve anything.

While Mail::Bulkmail has useful features, we can't use ones which can't be
easily emulated via command line sendmail + friends, since injecting stuff that
way is useful (Eg to pass in the -Odeferred stuff)

As discussed on irc the other day, the current MailHandle patch has the problem
with headers in templates. One option is to use a separate header template,
which will make things easier when we eventually allow both html and text based
emails. I am not convinced by the argument that people doing custommised
templates may forget the blank line - we may as well argue that they may leave a
tag out of the html templates. Whilst true, its easily noticable when testing,
and so that argument doesn't help anything (unless someone wants to make a test
for requirements for email templates, of course) Regardless of that, some
headers (ie content-type) will need to be present in the general template.

So, whatever solution we choose must be able to deal with:

- templated and non templated mails
  - this includes dealing with headers which may want to be customised as a
custom template. Note that currently (2.14.1), people can have every mail ccd to
a particular person by editing the templates in the params. We shouldn't lose that.
  - ditto for a set of global headers (X-Bugzilla-Version ?)
  - Possibly an X-Template-Version for the email templates, similar to the html
comments?
- able to use sendmail, smtp, or extendable to anything else within reason. (I
prefer JP's OO solution to Gerv's, but I suspect that that is just personal
preference)
- The sendmailnow param, where appropriate

It must be able to be modified (Without rewriting from scratch) to be able to:

- deal with multipart emails, and so on (eg for sending attachments with mails,
should we eventually have a per-role pref for that)
- anything else which people have planned for the near future.

It does _not_ have to deal with installing a new module if required - while I
have no objection to rolling our own module if required, we shouldn't do so just
because doing otherwise would require another module. Note that this does not
preclude a wrapper to another module, similar to how the patches wrap Net::SMTP.

Does that about sum the situation up fairly inconclusively? ;)

Comment 70

[Gervase Markham [:gerv]]

Attachment: Patch v.2

If we are going to go with my patch (and Dave's mails indicate that), I
obviously want to fix it up so it's least objectionable to as many people as
possible, and we end up with something simple which achieves our goals for
2.16. What I don't want to do is get into a pros and cons argument again, or
annoy anyone. We're all on the same side here.

This version of the patch is updated to change the new templates, but otherwise
unchanged. I'm looking for initial review comments; one thing I do want to do
is make it possible to turn off email completely, or write it all to a file; as
Paul points out, these are useful abilities. The difficulty is how to present
an interface to those options to the administrator using the Params interface,
which doesn't support such useful UI as e.g. a set of radio buttons, two of
which have text boxes associated.

Gerv

Comment 71

[J. Paul Reed [:preed]]

Per a discussion brought up on reviewers@... I'm just gonna call it: pushing
back to early 2.18 land.

justdave/et al. can thwap me if they disagree.

Comment 72

[J. Paul Reed [:preed]]

One last thing: whenever this patch finally does get checked into 2.18, if
anyone *really* wanted it for 2.16, I'll backport the patch to the 2.16-release.

That should appease those who really wanted it for 2.16 (and give us something
to say on IRC when people ask).

Comment 73

[Dave Miller [:justdave]]

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

Comment 74

[Johannes Pellenz]

Gerv,

is Patch v.2 (attachment 80823 [details] [diff] [review]) supposed to work with 2.16 RC 2? I tried it
today (Win2k, using cygwin's patch version 2.5) and got three errors (see
below). Also, the file CGI.pl wasn't touched at all. Any ideas?

I want to replace a 2.14.1 installation which is using NTsendmail, so it would
be great to have a working patch for 2.16.

Johannes

$ patch -i patch_84876_80823.txt -u -l -F 50000 checksetup.pl
patching file `checksetup.pl'
Hunk #1 FAILED at 775.
1 out of 1 hunk FAILED -- saving rejects to checksetup.pl.rej
patching file `globals.pl'
Hunk #1 succeeded at 1386 (offset -56 lines).
Hunk #2 succeeded at 1822 with fuzz 2 (offset 29 lines).
patching file `importxml.pl'
patching file `move.pl'
patching file `defparams.pl'
Hunk #1 succeeded at 221 (offset -136 lines).
Hunk #3 succeeded at 264 (offset -136 lines).
Hunk #5 succeeded at 465 (offset -137 lines).
patching file `Token.pm'
Hunk #1 succeeded at 83 with fuzz 1 (offset 15 lines).
Hunk #2 FAILED at 168.
Hunk #3 succeeded at 206 with fuzz 3 (offset 38 lines).
Hunk #4 FAILED at 251.
2 out of 4 hunks FAILED -- saving rejects to Token.pm.rej
patching file `whineatnews.pl'
patching file `processmail'
patching file `checksetup.pl'
Hunk #1 succeeded at 234 (offset 25 lines).
Hunk #2 succeeded at 225 with fuzz 3.
Hunk #3 succeeded at 2960 (offset 195 lines).
...

Comment 75

[Gervase Markham [:gerv]]

Attachment: Patch v.3

After a discussion with justdave and JayPee this evening on IRC, here's a new
version of my patch. It adds the ability to log to file, and decent prefs for
configuring what you want. It also seperates out the different methods of
sending mail into functions, so another one could be added pretty easily.

It's not quite finished yet; we've got a chicken-and-egg problem using
ThrowCodeError() in globals.pl that we need to solve (which I hacked around).
But it shows the concepts.

Gerv

Comment 76

[Bradley Baetz (:bbaetz)]

You're using Mail::BulkMail :)

Which, apart from the other stuff, hasn't been updated since Oct 2000, two
months after the 'total re-write' which resulted in version 2.00

Comment 77

[J. Paul Reed [:preed]]

Just so everyone else on this bug isn't in the dark, Gerv and I, as usual,
couldn't come to any sort of agreement on the majors issues involved with this bug.

Since the patches here have bitrotted a bit, and both Gerv and I had changes
that we'd like to make to our initial submission, we agreed with justdave on IRC
that we would submit our new versions by this Monday, and those patches would
either be evaluated by reviewers@ or justdave and we would go from there.

So... you can all await with baited breath my version for comparison/contrast. ;-)

Comment 78

[J. Paul Reed [:preed]]

Attachment: ProcessMail.pm, v1

This is a proof-of-concept version of my proposed Bugzilla::ProcessMail.pm

As I was looking at Gerv's Patch v.3, I noticed a lot of things that will end
up going into either patch (all of the work to remove the direct calls to
sendmail and some of the templating stuff), so I didn't bother repeating those
diffs in this patch.

As noted, this patch is merely for discussion and so we can make a final
decision between using Gerv's patch or mine; if we decide to use mine, then
I'll shore this up so it can go through the normal review process.

Comment 79

[Jouni Heikniemi]

As I promised earlier, here are my comments about this issue:

I don't know Mail::Bulkmail, but the interface to it seems a bit clumsy;
especially since we really don't bulkmail anything. I didn't test either patch,
but they both look like they're trivial to get working on Win32 as well, so I
have no strong opinion about this. With the strong support bulkmail's author
promised, I don't think there's much difference.

Wrt the discussion about mailing modularity Jaypee and Gerv had in reviewers@ in
the beginning of June, I think Gerv's listed three alternatives do not cover
100% of the possibilities. Therefore, modularity is fine. Fighting over whether
to implement it with an if block or a modules hash is waste of our precious
time, get over it. The latter is cooler IMO, but there's not really that much
difference in practice.


All in all, I can't find a dramatic difference between these patches,
quality-wise. Since I'm not aware of many good arguments to support the use of
Mail::Bulkmail, I'm slightly tilted towards Jaypee's solution. However, I
reserve the option to change my mind if somebody presents sufficient new
information to support the other conclusion. :-)

Comment 80

[Joel Peshkin]

Both these patches now seem to share the key improvemnts.   (Which may explain
why I'm having a hard time generating a worthwhile opinion on the subject)  I'd
suggest taking the patch from whoever plans to really own the resulting support
and do a quick audit of the benfit list of the other to make sure that no
valuable idea is left behind.

Comment 81

[J. Paul Reed [:preed]]

From an email review of the patches by bbaetz:

I think that using Mail::Mailer is the best way to go, and I've said this 
repeatably.

Not only because it is being activly maintained, but mainly because a 
small module which inhertied from Mail::Mailer (so that the constructor's 
params were read from the config, rather than specified in the cgi)

You then ->open a hash of headers, and pass the object in as the third 
argument to $template->process, or p0rint to it, or whatever. Then you 
->close it.

The module is then 15 lines of code, and you're done:

package Bugzilla::Mailer;

use strict;
use base qw(Mail::Mailer);

use Bugzilla::Config;

sub new {
  my ($invocant, @args) = @_;
  my $class = ref($invocant) || $invocant;
  my $type = Param("mail_type");
  my $self = $class->SUPER::new($type, @args);

  bless $self, $class;
  return $self;
}

Thats it. Its really, really, really hard to get any simpler than that. 

(actually, the $class->SUPER may need to be $invocant->SUPER, or even
$class->Mail::Mailer in case we're called from a subclass of us, to avoid
a dependancy loop. Thats not important here, though)

Now, thats not all there is. For example, you may want to be able to pass
headers in one at a time, rather than all at the same time. So you'd add
an 'add_header' sub which adds to $self->{'_bz_header_hash'}, and then
overload the open sub to add that to the front of whatever is passed in as
the argument.

Yes, this overloading is possible with Mail::Bulkmail, too, but because of
how almost everything is done in teh constructor, its not as easy. Note
that with this model, its easy to use Mail::Bulkmail under the hood even
without changing the external API - just keep the message which is printed
to us, and then on ->close, create/send the bulkmail module.

Mail::Bulkmail is nice, and if we were looking for a way to templatise
mail messages, then it would be great. But we're not. Similarly, if we
were going to use it to send to thosands of people at once, its probably
faster/better/etc. Bug again, we're not. It offers a lot more, but we
don't need that. I dunno - maybe I just don't like the fact that it
doesn't like foo@localhost as a valid email address.

Bradley

--

Upon asking him for clarification on which module he preferred, he clarified
that he was backing my general approach.

Comment 82

[Gervase Markham [:gerv]]

Why am I not CCed on this bug? :-)

Jouni said:
> I think Gerv's listed three alternatives do not cover 100% of the possibilities.

No-one has yet presented any others. There are two ways you can send mail from a
Perl script - invoke an external program to do it, or use a Perl module. My
patch covers both. 

Gerv

Comment 83

[J. Paul Reed [:preed]]

re: comment 82:

I've presented a number of others; you've just chosen to ignore them. There are
really two issues here: first, there are a multitude of ways to send mail in
Perl; you list the two conceptual ways. But there are a few call-a-binary ways
of doing it (which differ based upon the platform you're using, it would seem),
and then there's Net::SMTP, Mail::Mailer, Mail::Bulkmail, etc., etc., etc.

Second, there's the issue of "sending mail" really being a communications
mechanism; "sending mail" could, at some point be "sending the contents of a
mail message to an instant messaging client of some sort" or (more realistic if
you like) "printing the message to a file for debugging." Now, you've included
the debugging method after much prodding, but you ignore other methods that we
may not even have thought of. Worse, as I've explained before, your patch
doesn't even support other ways of doing things.

I think what Jouni was trying to say (and what I've been saying explain along)
is that your patch is entirely inflexible in accomodating those other possible
methods/modules, and any Win32 user or ambitious individual wanting to add
support for some other method of "sending mail," whatever that really means, is
going to have to tear your code apart to get it done, whereas they'll be able to
easily and cleanly extend mine.

I think that's the point (although, not wanting to put words in Jouni's mouth,
feel free to correct me if I'm wrong).

BTW, status on this bug: I'm flying back to Cali tomorrow, but will then take
another look at it and probably generate a full patch synced against HEAD.

It is at this point where I will become extremely annoying in order to get this
bug closed out.

It's time.

Comment 84

[Gervase Markham [:gerv]]

Comment on attachment 96012 [details] [diff] [review]
ProcessMail.pm, v1

>+use Bugzilla::ProcessMail;
>+
>+DefParam("mailmodule", 
>+         "The method this Bugzilla installation uses to send mail",
>+         "s",
>+         [ GetMailModuleList(), 'sendmail' ],
>+         &check_multi);

Because of the need to be "flexible", the text here can offer no advice as to
which of the possible mail sending methods is the one to use. As it stands, the
"netsmtp" option in this list will be the easiest one for 95% of admins - but
how are they to know that?

In my view, the interface we should be presenting to administrators, by
default, is:
Stick your SMTP server name here -> [			 ]
and then Just Work(TM). That's all a program should need to know to send mail.
Administrators with special needs should, of course, be able to use a local
sendmail-type program if it turns out to be faster and they need the speed.

>+DefParam("bzmailfrom",
>+         "The from address of mail this Bugzilla installation sends",
>+         "t"
>+         "bugzilla-daemon");

This should not be a param. Eventually, it should be changeable in some sort of
global mail header template, but until then it can stay hard-coded.

>+    my $templateOutput;
>+
>+    ## Process the global header; this gives us to/from/subject
>+    $template->process($globalMailHeader, $vars, $templateOutput)
>+        || ThrowTemplateError($template->error());
>+    
>+    $msgHash->{send} = $templateOutput;
>+
>+    ## Process the message-specific header; things like X-Bugzilla-Reason
>+    $template->process($emailHeaderTemplate, $vars, $templateOutput)
>+        || ThrowTemplateError($template->error());
>+
>+    $msgHash->{sendbody} .= $templateOutput;
>+
>+    ## Process the message header; actual message gets created here
>+    $template->process($emailMessageTemplate, $vars, $templateOutput)
>+        || ThrowTemplateError($template->error());
>+
>+    $msgHash->{sendbody} .= $templateOutput;

It would be more efficient to accomplish this using the Template Toolkit's
INCLUDE mechanism.

>+sub DoSendmailMailing {
>+    my $mailHash = shift;
>+
>+    my $sendmailLocation = "/usr/lib/sendmail";
>+    my $sendmailArgs;
>+
>+    if (Param("sendmailnow")) {
>+        $sendmailArgs = " -ODeliveryMode=deferred";
>+    }
>+    elsif ($mailHash->{modflags} eq "bg") {
>+        $sendmailArgs = " -ODeliveryMode=background";
>+    }

If they configure it to use a different mail program, the above options make no
sense. Or does this design only support sendmail?

>+sub DoNetSMTPMailing {
>+    my $mailHash = shift;
>+    use Net::SMTP;
>+
>+    my $smtp = Net::SMTP->new($::mailmodargs) or return;
>+
>+    $smtp->mail("$mailHash->{from}");
>+    $smtp->to("$mailHash->{to}");

Other headers?

>+    ## Assumes body is a fully formatted message; check if Net::SMTP adds
>+    ## an SMTP time or not...
>+
>+    $smtp->data();
>+    $smtp->datasend("$mailHash->{sendbody}");
>+    $smtp->dataend();
>+    $smtp->quit();
>+   
>+    return 1;
>+}

There are a few issues with this:
- no connection pooling
- no envelope send (if we ever aggregate CC list mail, for example)
- no error logging for the connection (at least, not that I can see)

I believe this will make using the Perl Module approach much slower than if we
used one of the modules more suitable for the task. I have no deep attachment
to Mail::Bulkmail, but it does provide all of the above, and the author has
promised to support it for us.

This version is quite a bit closer to my latest version; there appears to be
some convergence going on. But I still think that this "extensibility" gives us
only illusory benefits, and has real downsides (see above.)

Gerv

Comment 85

[J. Paul Reed [:preed]]

A couple of other things, since you reviewed my patch:

-- The sendmail support has a bunch of old deferred/sendmailnow crap in it; that
will be removed (probably), or at least dealt with in a cleaner way; we
currently pass a couple versions of that flag to sendmail (deferred vs.
background), so there's some confusion there.

-- I picked Net::SMTP just to pick a module; I wouldn't be (necessarily) opposed
to Mail::Bulkmail, BUT, as bbaetz points out, Mail::Bulkmail isn't really
necessary because we're not sending bulkmail. And Mail::Mailer is a better win
in terms of MTA support.

-- I'm not a TT expert; in fact, this was my first TT-experience; expect me to
get it wrong.

-- bzmailfrom shouldn't be hardcoded in the template either; see bug 126151

-- Other headers: in Net::SMTP, those are set in the creation of the message;
the to()/from() methods concern the envelope.

-- The easiest/most common config should be the default; I left it as sendmail,
but I had no reason for doing so (I didn't think about it, really).

Comment 86

[Gervase Markham [:gerv]]

> But there are a few call-a-binary ways
> of doing it (which differ based upon the platform you're using, it would 
> seem),

Yes. So you need the ability to enter a command-line to run. Which my patch has.
(But, because this requires more setup, I anticipate only a few, large Bugzilla
installations will choose this method.)

> and then there's Net::SMTP, Mail::Mailer, Mail::Bulkmail, etc., etc., etc.

For every other function Bugzilla needs, the developers pick a module which
performs that function and which best suits their needs, and then use it. Mail
should not be any different. There is no earthly reason why we should offer the
"choice" of achieving exactly the same end using more than one different Perl
module.

> Second, there's the issue of "sending mail" really being a communications
> mechanism; "sending mail" could, at some point be "sending the contents of a
> mail message to an instant messaging client of some sort" or (more realistic 

That sort of blue-sky thinking just leads to over-engineering. Which is what is
happening here.

> support for some other method of "sending mail," whatever that really means, 
> is going to have to tear your code apart to get it done, whereas they'll be 
> able to easily and cleanly extend mine.

There is really very little difference IMO. Mine requires an extra branch on an
IF test; yours, an extra function in a lookup table. Or, if this IM system can
be communicated with via a command-line program, my patch requires no changes at
all.

> -- I picked Net::SMTP just to pick a module; I wouldn't be (necessarily) 
> opposed to Mail::Bulkmail, BUT, as bbaetz points out, Mail::Bulkmail isn't 
> really necessary because we're not sending bulkmail. 

If we plan to amalgamate mails to the CCs of a bug, then envelope-send will be
very useful, and reduce the load on the relevant Bugzilla server.

> And Mail::Mailer is a better win in terms of MTA support.

Again, this makes no sense. If you use a Perl module to send mail, it's talking
SMTP to some SMTP server (MTA) somewhere. Given that all MTAs support SMTP, how
can such a module have "better" or "worse" MTA support?

> bzmailfrom shouldn't be hardcoded in the template either; see bug 126151

I don't agree that mail should come "from" the user whose change generated it;
this will merely encourage newbies to mail that person instead of commenting in
the bug. We get enough of that already.

Gerv 

Comment 87

[Bradley Baetz (:bbaetz)]

Sigh. OK everyone, lets take a deep breath.

This bug is for sending mail. Sending <anything else> shouldn't matter.

Mail::Bulkmail may have some advantages. But lets be honest - we aren't using
any of those features, and we aren't going to be doing so in the foreseeable
future. We are not planning to combine cc's, and there are a couple of WONTFIX
bugs on this. This is because of personalisation of message format, and so on,
mainly.

I have yet to see _anyone_ claim that the act of bugzilla sending the mail for a
bug takes a noticable ammount of time. (Remember that about 1 second of
processmail's time on my computer is spend loading various modules brought in
from globals.pl, which this patch won't affect- theres another bug on moving tha
tinto a module). This bug is not a perf bug; its a 'make this work on win32' bug.

As for extendability, my pseudo-patch requries an extra param type added to
defparams, and thats it....

Comment 88

[J. Paul Reed [:preed]]

This is gonna be my last response to this "thread" as it were; I've got better
things than to rehash all this stuff with you again. As I said before, I will be
writing the final patch to sync with HEAD, and then we'll go from there. To be
sure, it will probably incorporate parts of your patch; as you note, they are
starting to converge, which I think is a sign of some light at the end of the
tunnel.

-- Re: MTA modules: *we* (i.e. the BZ team) may only support one module, but
some company *may* want to use another perl module for mail; we should make that
easy for them to do, without having to hack in another branch to the if, or deal
with DefParam issues to get it all right. My patch: the only modification you
make is to add your function that does what you want, and add one line, and the
rest "just works." *That's* the point: a nice, clean interface to mail handling.

-- Re: sending "other things"/overengineering. I would make the claim that  your
patch, Gerv, is *under*engineered. One of the reasons that NS4 had to be chucked
is because people made decisions like yours: "Well, we'll only have to support
it this one way, and if we need to support something else, we can just tack on
some code here and there, and it'll all work." What happened? Buggy code that
didn't lend itself to modfications when the requirements changed, and which was
thrown away because it wasn't reusable/extendable (in any easy fashion, anyway).
You admit that my code isn't all that dissimilar from yours, but it is a whole
lot more modular, and while we won't be using it to send instant messages right
now, the fact that it lends itself to everything from that to supporting the
specific needs of Win32 users illustrates its ability to handle changing
requirements with minimum fuss, so we don't have to chuck the whole module
because something fundamentally changed a year down the road.

-- Re: Mail::Bulkmail: bbaetz has explained why Mail::Bulkmail is not such a big
win afterall (with the CCs). I don't need to reiterate, except to say that all
of the features that make Mail::Bulkmail supposedly so cool, we won't really be
using; I'm not saying "Let's not use it," I'm simply saying we should evaluate
the other modules; Mail::Bulkmail doesn't do anything for us.

-- Re: Mail::Mailer/MTA support: it makes perfect sense; go look for bugs on BZ
not working with exim or postfix (specifically, -Odeferred not meaning anything
to /usr/bin/exim-send or whatever); who said anything about SMTP?

-- Re: bug 126151: just because *you* or bmo doesn't want it, doesn't mean that
a small consulting firm with 20 BZ users wouldn't. It would most certainly be a
selectable param.

Comment 89

[Gervase Markham [:gerv]]

> -- Re: MTA modules: *we* (i.e. the BZ team) may only support one module, but
> some company *may* want to use another perl module for mail; we should make 
> that easy for them to do, without having to hack in another branch to the if, 
> or deal with DefParam issues to get it all right.

No, we don't. We don't make it "easy" for them to use another XML parser, or
another graph-plotting module, by having pluggable interfaces for those items.
We choose the most appropriate module to be part of Bugzilla, and if you use
Bugzilla, you use them. Mail is not different.

> -- Re: sending "other things"/overengineering. I would make the claim that  
> your patch, Gerv, is *under*engineered. One of the reasons that NS4 had to be 
> chucked is because people made decisions like yours:

The decisions which led to the writing of Netscape 4 were absolutely the right
decisions for that company at the time. The fact that eventually they had to
throw the code away doesn't change that. But this is nothing like on that scale.

> now, the fact that it lends itself to everything from that to supporting the
> specific needs of Win32 users illustrates its ability to handle changing
> requirements with minimum fuss, 

Please don't invoke the Win32 boogie-man. My patch works perfectly well with
Win32, as you well know. That's the point of Perl modules - they are cross-platform.

> I'm simply saying we should evaluate
> the other modules; Mail::Bulkmail doesn't do anything for us.

Connection pooling. Discounting one of my points for using Mail::Bulkmail
doesn't discount all three.

> -- Re: Mail::Mailer/MTA support: it makes perfect sense; go look for bugs on 
> BZ not working with exim or postfix (specifically, -Odeferred not meaning 
> anything to /usr/bin/exim-send or whatever); who said anything about SMTP?

Your claim is that Mail::Mailer has "better MTA support" than Mail::Bulkmail. 

This assertion is both false (both speak SMTP to any MTA), and nothing to do
with our current incompatibility problems, which are related to command-line
options.


I have two requirements from whichever solution Dave chooses, from an _admin's_
usability point of view:

- the default install of Bugzilla should make it so that there's a Param marked
in big letters "insert SMTP server here" and, if you do that, mail Just Works.
This is perfectly possible to achieve, and such a setup is suitable for all
Bugzilla installations under a certain (quite large) size. "localhost" is
obviously a valid value for this parameter.

- If the admin instead chooses to use a command-line program, they should be
able to choose any path to that program, and use any options to that program.
(So we shouldn't add options ourselves - because then we might break it.)

And, from a code point of view, the solution should:

- conform to coding standards, or Bugzilla prevalent style, in the naming of
modules, identifiers etc., the layout of comments and so on, and make proper use
of framework-supplied services.

Gerv

Comment 90

[J. Paul Reed [:preed]]

> Mail is not different.

Yes it is. You don't have to communicate with an "XML server," of which there
might be 30 different varieties.

> The decisions which led to the writing of Netscape 4 were absolutely the right
> decisions for that company at the time.

I know. I was there, remember?

That doesn't mean that we shouldn't learn from some of their engineering
mistakes, especially since we're not constrained by their marketing parameters.

> Your claim is that Mail::Mailer has "better MTA support" than Mail::Bulkmail.

Maybe bbaetz can convince you; I'm obviously failing.

> "localhost" is obviously a valid value for this parameter.

Ironically enough, bbaetz found out that Mail::Bulkmail won't take email
addresses that end in 'localhost' because it doesn't end in \.\w{2,3}.

"Oops."

Comment 91

[Gervase Markham [:gerv]]

> > Mail is not different.
> 
> Yes it is. You don't have to communicate with an "XML server," of which there
> might be 30 different varieties.

Yer wot? We're not communicating with XML servers. Like the man said, "This bug
is for sending mail. Sending <anything else> shouldn't matter."

> > "localhost" is obviously a valid value for this parameter.
> 
> Ironically enough, bbaetz found out that Mail::Bulkmail won't take email
> addresses that end in 'localhost' because it doesn't end in \.\w{2,3}.
> 
> "Oops."

In your eagerness to score a point, you perhaps didn't actually read what I was
saying, which was that "localhost" was a perfectly valid value for an SMTP
server for the module to send mail to. I was not talking about email addresses.

To turn off Mail::Bulkmail's possibly-misguided attempts to save you from
yourself, and allow any sort of email address you like, set:
$bulk->Trusting("1");

Gerv

Comment 92

[Bradley Baetz (:bbaetz)]

My point is that Mail::Bulkmail is overengineered _for what we need_.

Has anyone looked at my comment #81?  Mail::Mailer has the advantage of
implementing IO::Handle, so we can use TT to print to it directly., and that
would let us take advantage of that.

Personally, I prefer gerv's setup code for headers, jaypee's template stuff, and
my module code. Not sure where that leaves us... :)

Comment 93

[Gervase Markham [:gerv]]

That's a great solution :-) bbaetz: if you make a combined patch based on what
you and Dave think are the best bits of the two solutions, I agree to not
complain about it :-) (I've always said I'd defer to Dave's decision on this
matter, but he hasn't made one yet...)

Gerv

Comment 94

[Bradley Baetz (:bbaetz)]

Unfortunatley, I'm booked until Nov 8th (when my thesis is due :)

Comment 95

[Matthew Tuck [:CodeMachine]]

> Personally, I prefer gerv's setup code for headers, jaypee's template stuff,
> and my module code. Not sure where that leaves us... :)

Can you elaborate on what is so good about these things and how they are better
than the alternatives?

Comment 96

[Bradley Baetz (:bbaetz)]

Attachment: allow header output from a template

OK, this is (sort of) what I meant.

This patch allows mail headers to be outputted from the templates, which is
sort of a requirement for the other patches.

Notes:

- There are two modules.
  - Bugzilla::Mailer currently uses Mail::Send, but thats _really_ not fixed -
that file is basically empty
  - Bugzilla::Template::Plugin::Mailer is the TT side of things.

- The TT side of things is, um, interesting. We can't pass in an open file
handle to the template, because we can't get one until after the headers have
been done. Hence the use of a FILTER. That either means two templates
("foo_headers" and "foo"), or this scheme. Note that [% FILTER $mailer %] foo
[% END %] [% FILTER $mailer %] bar [% END %] will probably send two mails, if
it works at all. Don't Do That. I doubt that that restriction will be a problem
in practice - if you think it will be, please give me an example now.

TODO:
- use prefs to set the mail type, allowing sendmail vs SMTP vs debug vs ... (do
this by overriding Bugzilla::Mailer->new)
- Convert the rest of the templated mail stuff to this syntax.
- Move the non-templated mail stuff to templates, so that this mechanism can
work.

The patch:
- only updates one template to work with this new scheme.
- the checksetup/tests change is so that we don't run the template, only
compile it. This is useful, since we don't want checksetup to be sending a
mail.... This may be better done by creating a Template::Context, rather than a
Template (and then getting its context) - need to think about that.
- process_mail_template is there so that we don't ouptut the blank lines to
STDOUT. As the comment says, it will become $template->process_mail at some
point, to go with $template->process

THIS PATCH IS NOT FINISHED YET!!!! Its fairly close to it, though, at least in
design/concept.

Comment 97

[Gervase Markham [:gerv]]

bbaetz' patch looks pretty good to me :-)

Gerv

Comment 98

[J. Paul Reed [:preed]]

There are now officially too many cooks in this kitchen.

I'll need to look at Mail::Send before I can really evaluate bbaetz's patch.
Just so I'm clear, we're now looking at Mail::Bulkmail, Mail::Mailer,
Mail::Send, and Net::SMTP.

This is getting absurd.

Comment 99

[Jouni Heikniemi]

To cut it short: Jaypee abstracted out my thoughts pretty well in comment 82, I
don't have a lot to add. 

Regarding the discussion about whether or not local SMTP and sendmail options
cover 100% of the mailing possibilities, I'll just withdraw my opinion. I don't
feel comfortable with the concept of closing out everything except sending email
out of this; I believe we should at least design-wise note the possibility of
other notification systems and thus create a generic notification module.
However, I haven't read through all of the discussion in various fora, and it
might well be that the current Bugzilla codebase can handle this sort of
expansions decently in the future. I have faith in your understanding :-)

I'd love to look through the patches and evaluate the merits of bbaetz's
addition (which looks cool allright), but I just can't squeeze the time right
now (caught between studies and a ridiculous budgeting season at work). If you
have any Win32 specific issues to solve or testing to be done, I will try to
assist you as promptly as I can.

Comment 100

[Bradley Baetz (:bbaetz)]

jaypee: Mail::Send is a front end to Mail::Mailer (in the same package)

Mail::Mailer wants you to do:

$mailer->open({To => '[e-mail]', From=> '[e-mail]', X-Bugzilla-Version=>
123});

Mail::Send uses:

$mailer->to('[e-mail]');
$mailer->add("From", "[e-mail]");
$mailer->add("X-Bugzilla-Version", 123);
$mailer->open

ie its just a nicer front end which is easier to use through the templating
system - internally Mail::Send stores all these headers in a class variable
which is just passed to Mail::Mailer when Mail::Send->open is called.

ie you can drop Mail::Mailer from your list....

Also, note that my solution was for a _different_ problem, and that its very
easy to make it use almost any module you want. All you'd have to do is change
the filer sub in Bugzilla::Plugin::Mailer::Template, and change what class
Bugzilla::Mailer inherits from. This patch is for teh 'where do you put the
headers' question discussed earlier in teh bug.

jth: This system is for email. If we wanted, say, to send notification via AIM,
then what we would do is have a Bugzilla::Notify module, and use different
templates for each output format, where the templates would use different
plugins. (You almost certainly want different templates, because the type of
information is different) Its not hard to do, if we ever want to.

Comment 101

[Gervase Markham [:gerv]]

Just in case it isn't sufficiently clear from my previous comments:

I give in. I concede. I surrender. I submit. Enough. Finis.

If JP wants to flesh out his patch for this bug, I would be happy for it to be
included into the tree. Bugzilla admins deserve more than our arguing. My only
request is that it meets the usability and code criteria I set out at the end of
comment 89.

Let's get this show on the road.

Gerv

Comment 102

[J. Paul Reed [:preed]]

I had guessed as much. ;-)

I was also talking with bbaetz about his meta-patch and how to incorporate it.

*Unfortunately*, I'm back at school now, so my dev-time is limited. This bug,
however, is *the* TOP of my list right now, so every free bz-developing moment
will be spent on this.

I'd like to try and get it into 2.17.1, but that's an entirely separate discussion.

Comment 103

[Bradley Baetz (:bbaetz)]

Note that I'm no longer as happy with Mail::Send as I was - it doens't do the -i
thing for sendmail, and I don't know if I'm happy with it forking to run
sendmail. (I'm not sure I'm unhappy with it, either, and we currently fork for
processmail, anyway)

Anyway, I'll attach two patches - a newer version of my previous patch, plus a
Mail::Mailer patch I'm yet to send off to the author, fixing the -i thing, and
allowing debug output to a file instead of stdout.

Whether we do use Mail::Send, or something else, is up to you.

Comment 104

[Bradley Baetz (:bbaetz)]

Attachment: mail template patch, v2

Comment 105

[Bradley Baetz (:bbaetz)]

Attachment: MailTools patch

Comment 106

[Gervase Markham [:gerv]]

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

Comment 107

[J. Paul Reed [:preed]]

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

Comment 108

[J. Paul Reed [:preed]]

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

Comment 109

[J. Paul Reed [:preed]]

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

Comment 110

[Dave Miller [:justdave]]

OK, where do we stand on this?  I see four patches that aren't marked obsolete
yet.  Although it's obvious that that last one is a patch for the MailTools perl
module, the other three aren't real obvious if they're needed in sequence or if
they're alternatives or what.  This bug is the one thing that makes 2.16 worse
than 2.14 for Windows users (if this one were fixed, there wouldn't be any worse
hacking required than what they already had to do for 2.14), and with 2.14
hitting end-of-line, Windows users are going to be stuck for an upgrade path the
next time we have a security bug (although we can all hope Bugzilla really is
secure now :)

Comment 111

[J. Paul Reed [:preed]]

We're waiting for bug 124174 to go in. This and 124174 are intimately connected,
so one has to go in first, and we're further along in getting 124174 ready.

Honestly, I wanted to work on this over the break, but no one was here to r=
124174, so...

You are correct: the patches on this bug are a mess; I need to sit down with
this bug after 124174 goes in and look at them all again. The *good* news on all
of this is I have a light quarter this winter, so I'll be able to work on this
as soon as 124174 goes in.

Comment 112

[Jean-Sebastien Guay]

Hypothetical situation : 

Let's say I just reinstalled Bugzilla from scratch from CVS because I fudged up 
something... (hypothetically :-)

And that for now, it seems to work fine (win32 machine) except for mailing of 
course.

And that since mailing is not critical to my user(s), I can help test this and 
bug 124174.

What is the sequence for applying the patches? JP, you said above that 124174 
is more close to being ready than this. So should I start with that and try it? 
I think it's been noted that this one is a jumble of patches, so after 124174, 
I wouldn't really know what to do except what I used to do, which is replace 
every call to sendmail by an equivalent call to NTSendmail (copy-paste, always 
a joy).

But I really want to help getting these two done, because they're the major 
ones for Win32 Bugzilla. Just point me in your preferred direction. :-)

Comment 113

[Dave Miller [:justdave]]

ooh, a volunteer :)

Yes, hit bug 124174 first, then we'll get this one.

Comment 114

[J. Paul Reed [:preed]]

As Justdave noted, you want bug 124174 first; you'll still have to find/replace
calls to sendmail, but since 84876 will take care of that at some point, I'd
recommend installing the patch for 124174 (the new one I will post this week,
not v7) and then replacing the one sendmail call in the new BugMail.pm file,
which is the only one that should matter.

Be sure to cc yourself to 124714 if you're not already.

Comment 115

[Jean-Sebastien Guay]

So... Now that 124174 is squashed, what's happening here?

Comment 116

[J. Paul Reed [:preed]]

This is still on the radar; right now, a lot of us are pre-occupied with
releasing 2.17.4, and a related set of security bugs, both of which take priority.

Comment 117

[Alan S. Jones]

Have you all just looked to see if the NMS utilities and "nms sendmail" the
SendMail replacment script could just be ported over or used directly with Bugzilla?

The SendMail replacement is intended as a drop in replacement for scripts that
want SendMail, but the system does not have it.

http://nms-cgi.sourceforge.net/
http://nms-cgi.sourceforge.net/scripts.shtml

It would be really nice if the docs just said if you Don't have send mail go get
this utility.  Or maybe the NMS people could relicence the utility to
MPL/GPL/LGPL then you could move the code on in to Bugzilla.

Comment 118

[J. Paul Reed [:preed]]

re: comment 117:

The goal of this bug is to make Bugzilla speak RFC 2821 SMTP, and stop trying to
support each platform's independent local MTA (or multitude thereof).

Although, your solution is a great idea for people using 2.16.x.

Comment 119

[Alan S. Jones]

I knew the bug was so the bug would speak SMTP.  NMS even has a formail script
that that does not require sendmail and talks to your SMTP server.  I figured
some code from the new FORMMail and SENDMAIL replacment could be pull into
bugzilla and save some coding time if the NMS people agreed.

Comment 120

[J. Paul Reed [:preed]]

Attachment: Series II patch, v1

It's finally here! The second series MTA config patch versions.

This version still has some issues that need to be resolved and it'll probably
get a lot of comments in review and testing. There will be an email to
developers@ shortly regarding this patch.

This patch is live at http://landfill.bugzilla.org/preed/bugzilla/

Comment 121

[Jean-Sebastien Guay]

JayPee,

Good to see some progress on this bug! Go go go! :-)

I probably won't be able to test it on our install though, because the gnuwin32 
version of patch doesn't seem to like your diff for some reason... ("Assertion 
failed" error on the first hunk). I'm waiting to get a response from the 
gnuwin32 people to see if it's a bug. You'll understand that with the size of 
that patch, I don't really want to apply it by hand. I'll see what happens.

From reading the code, it looks like a good step forward. Keep up the good work!

Comment 122

[J. Paul Reed [:preed]]

re: comment 121

Yeah, I can't blame you for not wanting to apply it by hand. :-)

My first thought was that I used -N for the diff, which diffs new files against
/dev/null; this doesn't exist on Win32, so maybe patch is choking on that? But
then you note it's happening on the first hunk, which isn't a new file, so I
don't know what's going on.

If you *really* want to test it, contact me privately, and maybe I can send you
a .tar.gz of all the patched/new files. This bug is supposed to help the Win32
situation, so we definitely need testing on that platform. :-)

Comment 123

[Jouni Heikniemi]

Comment on attachment 127613 [details] [diff] [review]
Series II patch, v1

I have now tested this on Win32. It's still unstable, but JayPee admitted he
already knows this has bugs ;-) Still, here are some results:

Filing new bugs seems to work fine.

Changing existing bugs seems to be fine as well, but I got one odd error about
not being able to send mail - it could've been a temporary net connection
problem too. Can't reproduce. :-(

Changing bug flags is totally borked. A similar error also occurs with
attachment flags; sometimes adding the flag seems to work, but then setting the
status to + or - makes all hell break loose.

--
Sat Jul 26 17:05:26 2003] process_bug.cgi: Use of uninitialized value in
concatenation (.) or string at Bugzilla/Flag.pm line 596. [Sat Jul 26 17:05:26
2003] process_bug.cgi: Use of uninitialized value in string eq at
Bugzilla/ProcessMail.pm line 85. [Sat Jul 26 17:05:26 2003] process_bug.cgi:
Use of uninitialized value in print at CGI.pl line 276.

Bugzilla has suffered an internal error. Please save this page and send it to
THE MAINTAINER HAS NOT YET BEEN SET with details of what you were doing at the
time this message appeared.

URL: http://localhost:81/process_bug.cgi

Template->process() failed twice.
First error: undef error - DBD::mysql::st execute failed: Table 'namedqueries'
was not locked with LOCK TABLES [for statement `` SELECT name, query,
linkinfooter FROM namedqueries WHERE userid=? ORDER BY UPPER(name)'']) at
Bugzilla/User.pm line 155
Bugzilla::User::queries('Bugzilla::User=HASH(0x2090db8)') called at
data\template/en/default/global/site-navigation.html.tmpl line 114 eval {...}
called at data\template/en/default/global/site-navigation.html.tmpl line 114
eval {...} called at data\template/en/default/global/site-navigation.html.tmpl
line 16 
--


Trying to get a forgotten password says:

--
Software error:

Undefined subroutine &Token::MailMessage called at Token.pm line 132.

For help, please send mail to this site's webmaster, giving this error message
and the time and date of the error.
[Sat Jul 26 17:13:56 2003] token.cgi: Undefined subroutine &Token::MailMessage
called at Token.pm line 132.
--

A similar message occurs when trying to change the email address, although from
Token.pm line 88.


All this and lots of more good bugs (clip sales pitch) on AS Perl 5.8.0/806,
Win2k, IIS 5.0. Looking forward to the next patch. :-E

Comment 124

[Jean-Sebastien Guay]

I was finally able to apply the patch (after changing line endings to DOS 
format, and applying about 6 hunks manually because they were rejected).

After applying, I went to editparams.cgi and set the mailmodule and mailmodargs 
parameters. I'm using netsmtp, and I set my mail server to the same as I use 
for my mails (which the server onto which Bugzilla is installed can talk to, no 
problem). Note that I'm using Apache and ActivePerl 5.6, whereas Jouni is using 
IIS and ActivePerl 5.8 .

The first error I got was when I added a comment to a test bug to see if 
everything was OK.

[Mon Jul 28 10:50:44 2003] [error] [client xxx.xxx.xxx.xxx] [Mon Jul 28 
10:50:44 2003] doeditparams.cgi: Use of uninitialized value in string eq at 
Bugzilla/ProcessMail.pm line 246., referer: http://blah/bugzilla/editparams.cgi

>    return "Failed to get response from server '$newArg'" if
>     ($rv eq undef);

I changed ($rv eq undef) to (!$rv), not sure if that would cause other 
problems, but now it seems to get further.

Second error (still when adding a comment to a bug):

[Mon Jul 28 10:55:33 2003] [error] [client xxx.xxx.xxx.xxx] [Mon Jul 28 
10:55:33 2003] process_bug.cgi: Argument "" isn't numeric in numeric gt (>) at 
data\template/en/default/global/hidden-fields.html.tmpl line 43., referer: 
http://blah/bugzilla/show_bug.cgi?id=170

Looking forward to feedback!

Comment 125

[Jean-Sebastien Guay]

Oops, sent that out a bit too fast.

On that last error, I checked the hidden-fields.html.tmpl file, and the 
only '>' there is at line 32. I don't really know what to do to fix it.

I'm looking forward to your feedback!

Comment 126

[J. Paul Reed [:preed]]

re: comment 123:

I'll look at the flags stuff soon (today? tomorrow?)

The table locked error you got, though, may be bug 211435, which has a fix
checked in (don't know why it's not resolved...)

If you're on the tip, cvs up and see if you still get the same error; if not,
let me know what error you *do* get. :-)

It's good to have a couple of Win32ers looking at this; any *nix people have any
luck w/ this patch?

Comment 127

[Jacob Steenhagen]

> if ($rv eq undef);

That should really be:

if (! defined $rv);

Which would get rid of the undefined value used in string eq warning.

Comment 128

[Jouni Heikniemi]

I also tested this on a Linux box. When I configured the mail prefs to be 
sendmail, /usr/sbin/sendmail -t -i and changed the server from address, I got 
the following:

--
Saving new parameters  
    
Changed mailmodule.
No recipient addresses found in header Changed mailmodargs.
No recipient addresses found in header Changed bzmailfrom.
--

Don't understand what those "no recipient addresses found" whines are. 

I still get the table lock error message, and I'm on the tip.

Hmm.. Going to do further testing in the near future, hopefully with the next 
patch :-E

Comment 129

[John Keiser (jkeiser)]

One problem with sendmail configuration.

    return "Use full path to sendmail (start with '/', no '..')"
     if ($newArg =~ /\.\.?/ || $newArg !~ /^\//);

Does not work for sendmail.  I want to have an argument with a . in it (-f
[e-mail]).  \.\.? matches anything with a single dot in it, anywhere in
the string.  I say remove the ? or make sure the \.\.? does not occur before the
first space.

Comment 130

[John Keiser (jkeiser)]

OK, you're missing "use Bugzilla::ProcessMail" from a number of places.  The two
I found were CGI.pl (account create) and Token.pm (email change among other
things).  You should search for "MailMessage" everywhere and verify that
ProcessMail is being used.  

Also, at one point I ended up with the error "Bugzilla::ProcessMail::trim() not
found, even though you are doing "use Bugzilla::Util" there.  I wonder what is
up with that.  I fixed it by fully qualifying trim().

Email change does not seem to send any mails even after I fix the above errors.
  Password change likewise.  New user and bug activity do send mails properly,
so it's probably just Token.pm that has a problem of some sort.

Comment 131

[Gervase Markham [:gerv]]

paul: any chance of a status update? :-)

Gerv

Comment 132

[J. Paul Reed [:preed]]

Funny you should ask; justdave and I were talking about it just yesterday. I
promised him a status report and/or version II of the Series II patch, and am
leaving town for Labor Day, so I said I'd get it to him by September 2nd, 6 am PDT.

It's currently a blocker for 2.17.5 (and I'd like to keep it that way, so it can
have the maximal amount of testing before 2.18), but that's justdave call, and I
could see how he might want to waive it off. Then again, he wanted 2.17.5 by
September 1st, and given the holiday weekend, I don't think that's going to
happen. ;-)

Comment 133

[Gervase Markham [:gerv]]

> It's currently a blocker for 2.17.5 (and I'd like to keep it that way

Absolutely; hence the request. Thanks for the update.

Gerv

Comment 134

[Dave Miller [:justdave]]

Comment on attachment 127613 [details] [diff] [review]
Series II patch, v1

marking review- on behalf of jkeiser (comment 129 and 130)

Comment 135

[Jeroen Ruigrok van der Werven]

Since the email functionality is a very important aspect of Bugzilla, the 
account creation uses it to send out initial passwords, you also need something 
like:

    {
        name => 'Net::SMTP',
        version => '2.19'
    },

instead of the have_vers("Net::SMTP", "2.19");.

Thereby making it a prerequisite instead of a optional component.

Comment 136

[J. Paul Reed [:preed]]

Re: comment 135: this patch still supports using sendmail for backwards
compatibility reasons which is why Net::SMTP is optional, not required. Maybe
we'll change that for 2.20, but for now, we still need to support the deprecated
methods of sending mail without *requiring* the new modules.

BTW, this is still a 2.17.5 blocker and I'm still shooting for that; I'd like
this to get a lot of testing before the 2.18rcX's.

Comment 137

[Jeroen Ruigrok van der Werven]

Attachment: Update of J. Paul Reed's email changes patch to latest CVS

This now applies cleanly to the CVS tree of 2003-09-05.

Comment 138

[Ralf Hauser]

for cygwin users, also ssmtp would be helpful!

Comment 139

[Guenter Huerkamp]

This bug is just removed from bug 212878, so this will not in 2.17.5, right?
Is 2.17.5 on the way?

Comment 140

[J. Paul Reed [:preed]]

The Bugzilla Team is working on 2.17.5, yes.

This bug was removed from 2.17.5 because it's a relatively large change, and no
one has stepped up to the plate to review the patches.

Justdave and I discussed this and agreed that it would be better to ship 2.17.5,
and then get 84876 in right after 2.17.5 ships.

That's the current plan.

Comment 141

[Gervase Markham [:gerv]]

> no one has stepped up to the plate to review the patches.

The last patch you attached (in July) has two "review-"es on it; I for one had
assumed that therefore the ball was in your court to come up with an new
version. Was that a misunderstanding?

Gerv

Comment 142

[J. Paul Reed [:preed]]

Gerv:

I've been correcting some of the issues with the patch that has the various
review-'s on it; the last issue involves a (overly?) complex template that Myk
wrote, and I've been in the process of tracking him down for the last... 2-3
weeks or so to help me with it. I guess he was on vacation or something.

Justdave and I discussed it about 2 weeks ago now that because this is such a
big patch, it would be easier to just check this in after 2.17.5; this decision
was made, of course, back when we thought we were releasing 2.17.5 later that
week. Oops.

But that's what Justdave and I decided, so that's why this bug was removed as a
dependency.

Comment 143

[Gervase Markham [:gerv]]

Right, I get that. I was just checking that you weren't annoyed because you were
waiting for one of us to come and review something in this bug. Glad that wasn't
an issue.

Gerv

Comment 144

[Jouni Heikniemi]

[e-mail]: What is "ssmtp" essentially?

Comment 145

[Ralf Hauser]

From the man page:
<<SSMTP(8)                                                              SSMTP(8)

NAME
       ssmtp, sendmail - send a message using smtp

SYNOPSIS
       ssmtp [ flags ] [ address ... ]
       /usr/lib/sendmail [ flags ] [ address ... ]

DESCRIPTION
       ssmtp is a send-only sendmail emulator for machines which normally pick
       their mail up from a centralized mailhub (via pop, imap, nfs mounts  or
       other  means).   It  provides the functionality required for humans and
       programs to send mail via the standard or /usr/bin/mail user agents.

       It accepts a mail stream on standard input with recipients specified on
       the  command  line  and  synchronously forwards the message to the mail
       transfer agent of a mailhub for the mailhub MTA to process. Failed mes-
       sages are placed in dead.letter in the sender's home directory.

       Config  files  allow  one  to  specify the address to receive mail from
       root, daemon, etc.; a default mailhub; a default domain to be  used  in
       From: lines; and per-user From: addresses and mailhub names.

       It does not attempt to provide all the functionality of sendmail: it is
       intended for use where other programs are the primary means of at  last
       mail  delivery.   It  is  usefull with pop/imap, or to simulate the Sun
       shared mail spool option for non-Sun machines, for machines whose send-
       mails  are  too  difficult (or various) to configure, for machines with
       known disfeatures in their sendmails or for ones where there are ``mys-
       terious problems''.

       It does not do aliasing, which must be done either in the user agent or
       on the mailhub. Nor does it honor .forwards, which have to be  done  on
       the recieving host.  It especially does not deliver to pipelines.

OPTIONS
       Most sendmail options are irrelevent to sSMTP. ...>>

in essence, it does the same, but you do not need days to configure it right as
with sendmail, but minutes are enough.
Also, on Linux, there are rpm's for it - e.g. (ssmtp-2.38-1.i386.rpm)

Comment 146

[Jouni Heikniemi]

If I understand the man page correctly, ssmtp has command-line compatibility
with sendmail, so ssmtp should work right out-of-the-box. Series II patches have
configurable executable path for sendmail, so if you set your ssmtp path there,
everything just might work. Of course, you can ensure this yourself by testing
the next patch when it's done :-)

Comment 147

[Ralf Hauser]

unfortunately, ssmtp doesn't work out of the box with bugzilla. I tried it.  It
seems that bugzilla used a sendmail option ssmtps doesn't have. So, I resorted
to patch with a SMTP which is ugly.

Comment 148

[Jouni Heikniemi]

Ok then. Well, if you have the time, try finding out which option is the one
that causes problems, so we can consider if this is avoidable through some sort
of configuration switch.

Comment 149

[Dave Miller [:justdave]]

Did you try it with sendmailnow turned on in params?  If sendmailnow is off, it
will use sendmail-specific queuing options.  With sendmailnow turned on, the
only options it uses are -t and -i, which should be accepted by pretty much
every sendmail clone out there.

Comment 150

[Ralf Hauser]

Perhaps, I posted a false alert.
In order to reproduce, I now switched back to the ssmtp instead of CPAN-SMTP and
so far, I am not seeing a problem (perhaps, this was a side-effect of the taint
problem 177828 or I just didn't have ssmtp configuration fully finished yet -
i.e. it worked from the command-line mail, but perhaps didn't have a reasonable
from-address yet, ...). Apologies. If any problem arises, I'll report.

Comment 151

[Ulrich Windl]

Mail-sending is still not done in a consistent way. Just consider "grep sendmail
*.pl *.cgi":
CGI.pl#541 does not use any additional options
globals.pl#1399 hardcodes "-ODeliveryMode"
importxml.pl#117 uses a different setting ("background" vs. "deferred")
move.pl#153 also uses hard-wrired "DeliveryMode"
whineatnews.pl#67 does not add a "From:" field, so the mail is sent as the CGI
user (daemon, nobody, wwwrun, whatever). Shouldn't it be "bugzilla-admin" or
something like that?

Is there anything wrong with using one subroutine to send out mail, using some
consistent and configurable parameters?

Comment 152

[Dave Miller [:justdave]]

Ulrich: you didn't read the bug before you commented, did you...  That's exactly
what this bug is supposed to fix.

Comment 153

[J. Paul Reed [:preed]]

Attachment: Series II patch, v2

Ready to have the crap tested out of it (*been* long enough, huh?)...

http://landfill.bugzilla.org/preed/bugzilla/ is running this patch, if anyone
wants to play with it there.

Comment 154

[Gervase Markham [:gerv]]

Hey, J.P.,

Here are a few thoughts, outlined in hopefully a polite and non-confrontational
way :-)

Gerv

>Index: Bugzilla.pm
>===================================================================
>+##
>+## We need a separate TT instance because process() isn't reentrant; it turns
>+## out that for bugactivity pages, since we send mail from a template now
>+## (see bug 124174) using the same TT instance causes the process() handling
>+## the generation of email to step on the previously called process() handling
>+## the generation of the webpage; this fixes that.
>+##

The usual Bugzilla standard is a single hash.
re-entrant has a hyphen.
"bug activity" is two words.

>@@ -255,6 +270,10 @@
> =item C<template>
> 
> The current C<Template> object, to be used for output
>+
>+=item C<mail_template>
>+
>+The current C<Template> object, to be used for email output ONLY!

Or "A Template instance to be used exclusively for email output".

>Index: CGI.pl
>===================================================================
>+    ## Explicitly set the From: to be the BZ server
>+    $messageInfo->{'envelopeFrom'} = Param('bzmailfrom');
>+    $messageInfo->{'envelopeTo'} = $login . Param('emailsuffix');

Is the technical distinction between "From" and "Envelope-From" relevant to any
mails that Bugzilla sends?

>Index: Token.pm
>===================================================================
>+    $messageInfo->{'oldemailaddress'} = $old_email . Param('emailsuffix');
>+    $messageInfo->{'newemailaddress'} = $new_email . Param('emailsuffix');
>+    $messageInfo->{'max_token_age'} = $maxtokenage;
>+    $messageInfo->{'token_ts'} = $token_ts;

We need to separate all (or none, but all is more consistent with existing
practice) words in vars with underscores or hyphens.

>Index: checksetup.pl
>===================================================================
>+# Net::SMTP 2.19 is part of libnet 1.0703; 2.19 was picked as a minimal 
>+# version because libnet 1.0703 shipped with RedHat 7.2, the API is the same,
>+# and there are no known bugs in that version.
>+my $netsmtp     = have_vers("Net::SMTP", "2.19");
>+
> print "\n" unless $silent;
> if ((!$gd || !$chartbase) && !$silent) {
>     print "If you you want to see graphical bug charts (plotting historical ";
>@@ -307,6 +312,17 @@
>            "-e'install \"GD::Text::Align\"'\n" if !$gdtextalign;
>     print "\n";
> }
>+
>+if (!$netsmtp && !$silent) {
>+    print "If you you want Bugzilla to send mail directly using the SMTP ";
>+    print "capabilities of any MTA, you should install Net::SMTP; it comes ";
>+    print "with most default perl installations these days. As of 2.18, this ";
>+    print "method is the default (and suggested) way of sending mail. ";
>+    print "If you don't have it, run:\n";
>+    print "Net::SMTP:       perl -MCPAN -e'install \"Net::SMTP\"'\n" if 
>+     !$netsmtp;

Perl the program has a capital P. Also, a lot of people might not understand the
acronym "MTA".

Instead of this rather complex statement, why not just make Net::SMTP
compulsory? If it doesn't have any particular difficulties about its
installation, this seems much more sensible. I expect that the only people who
don't use it will be doing something different for very exceptional reasons.

>Index: defparams.pl
>===================================================================
>@@ -1093,6 +944,47 @@
>    type    => 'b',
>    default => 1,
>   },
>+
>+  {
>+   name    => 'emaildelivery',
>+   desc    => 'Is bug and other email from Bugzilla currently \'on\'?' .
>+              '(site-wide toggle)',
>+   type    => 'b',
>+   default => 1,
>+  },
>+
>+  {
>+   name    => 'mailmodule',
>+   desc    => 'The method this installation uses to send (bug, password ' .
>+              'change, etc.) email',
>+   type    => 's',
>+   choices => [ Bugzilla::ProcessMail::GetMailModuleList() ],
>+   default => 'netsmtp',
>+  },

Would it make sense for "Disabled" to be just an option on this list, meaning we
don't need the "emaildelivery" param?

>Index: importxml.pl
>===================================================================
>+  my $messageInfo = {};
>+  my $messageInfo->{'envelopeFrom'} = Param('moved-from-address');
>+  my $messageInfo->{'envelopeTo'} = $to; ## XXX - This probably won't work

Does it? :-)

>+  my $messageInfo->{'subject'} = $subject;

This looks like a localisation problem. In fact, perhaps the MailMessage
function should refuse to accept a "subject" param, to avoid people doing this
in new code by mistake.

>Index: Bugzilla/Flag.pm
>===================================================================
>+    if ($flag->{'status'} eq '?') {
>+        ## XXX - We're ignoring a pref here; need to ask what to do if the 
>+        ## pref isn't set...

What's the issue here?

>Index: Bugzilla/ProcessMail.pm
>===================================================================

Could this module be called simply Mail.pm?

>+## Do NOT |use Bugzilla;| or |use Bugzilla::Config;|! Doing so causes 
>+## a circular depedancy in defparams and causes stuff to blow up. This is why
>+## you'll see Bugzilla::Config::Param() all over the place here instead of
>+## the usual Param();
>+##
>+use Bugzilla::Util;
>+
>+use base qw(Exporter);
>+@EXPORT = qw(MailMessage GetMailModuleList %MailModuleParamCheckers);
>+
>+use strict;
>+
>+## Available modules and their param checker functions
>+my %MailModules = (
>+   'sendmail' => \&DoSendmailMailing,
>+   'netsmtp' => \&DoNetSMTPMailing,
>+   'debug' => \&DoDebugMailing,
>+);

We tend to be using lower case field and function names in objects, I think.

>+my %ModuleParamCheckers = (
>+   'sendmail' => \&CheckSendmailParams,
>+   'netsmtp' => \&CheckNetSMTPParams,
>+   'debug' => \&CheckDebugParams,
>+);

Would it make sense to combine these two hashes into a multi-level hash?

>+    #print "<b>MailMessage called!</b>";

You can probably take this out now ;-)

>+    my $emailHeaderTemplate = 'email/' . $tmplName . '-header.txt.tmpl';
>+    my $emailMessageTemplate = 'email/' . $tmplName . '.txt.tmpl';
>+
>+    ## Process the message header
>+    $template->process($emailHeaderTemplate, $vars, \$header)
>+     || ::ThrowTemplateError($template->error());
>+
>+    ## Process the message body; actual message gets created here
>+    $template->process($emailMessageTemplate, $vars, \$body)
>+     || ::ThrowTemplateError($template->error());

Couldn't we just [% INCLUDE %] the header template in the body template?

>Index: template/en/default/email/bugactivity-header.txt.tmpl
>===================================================================
>+  # INTERFACE:
>+  # emailmsg: hash; hash containing information about this message 

Is it not better to call this "message"? After all, as you pointed out in our
discussions about this earlier, we may send the same data by any number of
transports (IM, for example.)

>+  #
>+  #%]
>+[% PROCESS "email/global-header.txt.tmpl" 
>+     headerfrom=emailmsg.headerfrom 
>+     headerto=emailmsg.headerto 

This seems a somewhat roundabout way of doing the headers.

Why not do:

$vars->{'from'} = something;

Then have global-header.txt.tmpl just use [% from %].
You don't need to put it into the hash and then have a translation step in each
template. You just 
need a set of well-known and obvious names (like "from", "to", "cc" etc.)

There's another advantage to this; it also gives the opportunity to have some
mails as just:

- include global header
- do the body

in a single template.

To avoid unnecessary tiny template proliferation, I think we should only have a
header template when there's something special to go in the header. After all,
we can always split it out later if necessary. A lot of the current header
templates just perform the above unnecessary translation step.

>Index: template/en/default/email/bugactivity.txt.tmpl
>===================================================================

bugactivity should have a hyphen IMO. It's two words.

Also, we tend to refer to this stuff as "bug changed" mail, don't we? 

>Index: template/en/default/email/confirm.html.tmpl
>===================================================================

I thought the email directory was just for email? IMO the HTML templates
relating to email change should stay in the "account" directory with other
account-specific operations, where they are now.

>Index: template/en/default/email/emailchange-new-header.txt.tmpl
>===================================================================

Again, template name convention (as articulated by Myk) is to hyphen-separate
words (for this and some other template names in this patch.)

>Index: template/en/default/email/emailchange-new.txt.tmpl
>===================================================================
>+[% expiration_ts = emailmsg.token_ts + (emailmsg.max_token_age * 86400) %]

Comment to explain the magic number?

>Index: template/en/default/email/flag-email.txt.tmpl
>+[% emailmsg.user.identity %] has [% statuses.${emailmsg.flag.status} %] [%+
emailmsg.to_identity %] for [% emailmsg.flag.type.name %]:

Line too long ;-) Take advantage of PRE_CHOMP to break it up.

>Index: template/en/default/email/movebugs.txt.tmpl
>===================================================================
>+<?xml version="1.0" standalone="yes"?>
>+<!DOCTYPE bugzilla SYSTEM "[% Param('urlbase') %]bugzilla.dtd">

The XML bug generation definitely should not be in a .txt file in the email
directory. move-bugs.txt.tmpl should delegate to the XML template for
show_bug.cgi, wherever that is.

>+To get a list of all NEW/REOPENED  bugs, you can use this URL (bookmark it if 

Nit: double space.

>+  [% ELSIF error == "invalid_output_type" %]
>+    [% title = "Invalid Output Type" %]
>+    Invalid output type [% type FILTER html %].

This error is not used.

>+  [% ELSIF error == "mail_send_failed" %]
>+    Mail sending operation failed; From: was '[% from FILTER html %], To: 

Missing closing single quote.

Gerv

Comment 155

[J. Paul Reed [:preed]]

> re-entrant has a hyphen.

Not according to my dictionary.

> "bug activity" is two words.

It had no space because I was referring directly to the bugactivity template
(which was the best name I could come up with for that particular template).

> Or "A Template instance to be used exclusively for email output".

Changed.

> Is the technical distinction between "From" and "Envelope-From" relevant to 
> any mails that Bugzilla sends?

Not yet (as I remember), but it could be in the future (I built this in as a
feature).

> We need to separate all (or none, but all is more consistent with existing
> practice) words in vars with underscores or hyphens.

I'll be happy to change the names of the variables that I'm creating
(oldemailaddress, in the hash, for instance), but I don't want to change names
that aren't relevant to the patch or that I didnt' create. Besides, it's not
entirely clear which style is accepted; is this in the developers' guide?
$maxtokenage, for instance, was written by you.

> Perl the program has a capital P. Also, a lot of people might not understand 
> the acronym "MTA".

Fixed.

> Instead of this rather complex statement, why not just make Net::SMTP
> compulsory? If it doesn't have any particular difficulties about its
> installation, this seems much more sensible. I expect that the only people who
> don't use it will be doing something different for very exceptional reasons.

Because if we required it, people with large, existing installations who might
have sendmail tuned in certain ways will get pissed off that they can't upgrade
because we're *forcing* them to install Net::SMTP. bmo would be an example of this.

I can totally see requiring it for 2.20+ though, since the sendmail method is
now deprecated as of this patch.

> Would it make sense for "Disabled" to be just an option on this list, meaning
> we don't need the "emaildelivery" param?

I could go either way on that... I did it this way because I wanted there to be
an external toggle to turn email off that was separated both in the UI and in
the code from the mail module stuff.

But I'm not married to that way of doing it... if other people have opinions on
this, I'd be curious to hear them.


>Index: importxml.pl
>+  my $messageInfo->{'envelopeTo'} = $to; ## XXX - This probably won't work

> Does it? :-)

You tell me! ;-) Actually, I *think* it won't... and the reason it won't is
because $to isn't declared (in fact, I'll bet importxml.pl doesn't even compile.

The real reason I put that note in there is because $to (or @recipients above)
is an array of addresses, and the framework isn't (yet) designed to handle
sending multiple mails in the envelopeTo. I will have to fix that, but I don't
know if I want to do that as part of this patch... 

> This looks like a localisation problem. In fact, perhaps the MailMessage
> function should refuse to accept a "subject" param, to avoid people doing this
> in new code by mistake.

Actually, it's worse than that... you'll notice that the text of the message is
set along with the body. I agree that it's a localization problem, and I agree
that it has to be fixed, but again... this patch is large enough as it is. I'd
like to see it get in, and then have that filed as another bug. importxml.pl is
important, but it's kind of ancillary to daily operations of most Bugzilla
installations.

>Index: Bugzilla/Flag.pm
>===================================================================
>+    if ($flag->{'status'} eq '?') {
>+        ## XXX - We're ignoring a pref here; need to ask what to do if the 
>+        ## pref isn't set...

> What's the issue here?

I honestly can't remember. The Flag code needs a lot of eyes, because in looking
at that section, I just noticed a number of "issues" with it (including the
localization problem you noted in importxml.pl). I think the pref we're ignoring
either has to do with no sending flag emails (can users set that as a pref?) or
possibly with the envelopeFrom. In fact, I think that's it and the comment just
got shuffled around. You're supposed to be able to make the email appear from
the user who made the change, and I didn't do that here because I wasn't sure
how easy it was to grab that information, and then it kinda got ignored because
the flag stuff was the last thing I worked on.

> Could this module be called simply Mail.pm?

I wanted to differentiate it from Bugzilla::BugMail.

The module executes a function on mail, as opposed to figuring out how, when,
and what to put in an email (as BugMail does), so I wanted to include a sense of
action in the module, i.e. ProcessMail.

> We tend to be using lower case field and function names in objects, I think.

I'll be happy to change it to the standard, if one exists. But I need to know
for sure.

> Would it make sense to combine these two hashes into a multi-level hash?

Again, I could go either way on that; I think I did it this way so it would be
easier/clearer for others who may not have as much perl experience to add email
modules.

>+    #print "<b>MailMessage called!</b>";
> You can probably take this out now ;-)

Indeed. ;-)

> Couldn't we just [% INCLUDE %] the header template in the body template?

Could we? I'm not a TT expert. I think I did it this way because if we did it
that way, the email body template would have to figure out the name of the
template based on its own name, whereas here, it can figure it out based upon
which generalized template was passed into the function (i.e. in this version,
we're just tacking on "-header.txt.tmpl" as opposed to hacking apart the name of
the file and trying to consistently place "-header.txt.tmpl" into the name.)

> Is it not better to call this "message"? After all, as you pointed out in our
> discussions about this earlier, we may send the same data by any number of
> transports (IM, for example.)

I'd rather not change the name this late in the game, but you make an extremely
good point.

> This seems a somewhat roundabout way of doing the headers.
> 
> Why not do:
> 
> $vars->{'from'} = something;
>
> Then have global-header.txt.tmpl just use [% from %].
> You don't need to put it into the hash and then have a translation step in 
> each template. You just need a set of well-known and obvious names (like 
> "from", "to", "cc" etc.)

I agree that putting that information in the hash seems like a better way to do
it; I'll investigate that.

> To avoid unnecessary tiny template proliferation, I think we should only have 
> a header template when there's something special to go in the header. After
> all, we can always split it out later if necessary. A lot of the current
> header templates just perform the above unnecessary translation step.

The reason I split each message into a header template and a message template is
because there's some restrictions on what you can put into message headers, so I
wanted the split the two in very distinct parts. I gave each message it's own
header template because it made the code on the calling end (i.e. the two
process() calls) very simple and straightforward.

There are some merits to doing it with one "global" header, though... let me
think about this and discuss it on IRC with others.

> bugactivity should have a hyphen IMO. It's two words.
>
> Also, we tend to refer to this stuff as "bug changed" mail, don't we? 

I don't know; I don't think it's specified anywhere, so that's what I used.

> I thought the email directory was just for email? IMO the HTML templates
> relating to email change should stay in the "account" directory with other
> account-specific operations, where they are now.

I agree. I think I may have accidentally moved it.

> Again, template name convention (as articulated by Myk) is to hyphen-separate
> words (for this and some other template names in this patch.)

Ok.

> Comment to explain the magic number?

Ok... but I didn't put the magic number in there... I just ported it. ;-) If
you'r ethinking about it, I think it's pretty easy to deduce what it is, but...

> Line too long ;-) Take advantage of PRE_CHOMP to break it up.

WHAT_CHOMP? (Remember... not a TT expert).

>Index: template/en/default/email/movebugs.txt.tmpl
>===================================================================
>+<?xml version="1.0" standalone="yes"?>
>+<!DOCTYPE bugzilla SYSTEM "[% Param('urlbase') %]bugzilla.dtd">

> The XML bug generation definitely should not be in a .txt file in the email
> directory. move-bugs.txt.tmpl should delegate to the XML template for
> show_bug.cgi, wherever that is.

I've since forgotten how this all worked exactly, but I believe this is here
because move.pl sends email to move bugs. If it sends mail, it's in this
directory. Now, the template is emitting XML, yes, but it's ultimately .txt
because it's from the POV of the mail server... which sees XML as text.

Comment 156

[Gervase Markham [:gerv]]

>> re-entrant has a hyphen.
> 
> Not according to my dictionary.

OK. It can be either, and it looks much better with a hyphen.

>> "bug activity" is two words.
> 
> It had no space because I was referring directly to the bugactivity template
> (which was the best name I could come up with for that particular template).

The template name should have words separated (with hyphens) too ;-)

>> Is the technical distinction between "From" and "Envelope-From" relevant to 
>> any mails that Bugzilla sends?
> 
> Not yet (as I remember), but it could be in the future (I built this in as a
> feature).

The reason I ask is that it makes things much more complicated. It also gives
people the expectation that there's some need for a difference - I certainly
spend a confused period looking for it. 

Why might we ever need it? Can we add it in as and when we need it later?

>> We need to separate all (or none, but all is more consistent with existing
>> practice) words in vars with underscores or hyphens.
> 
> I'll be happy to change the names of the variables that I'm creating
> (oldemailaddress, in the hash, for instance), but I don't want to change names
> that aren't relevant to the patch or that I didnt' create. Besides, it's not
> entirely clear which style is accepted; is this in the developers' guide?
> $maxtokenage, for instance, was written by you.

I wouldn't claim that I always get it right ;-) My comment on accepted style was
after a quick grep of the templates. And I think underscore-separated is
clearer, as well.

Of course, I'm not asking you to change names you didn't create. :-)

>> Instead of this rather complex statement, why not just make Net::SMTP
>> compulsory? If it doesn't have any particular difficulties about its
>> installation, this seems much more sensible. I expect that the only people who
>> don't use it will be doing something different for very exceptional reasons.
> 
> Because if we required it, people with large, existing installations who might
> have sendmail tuned in certain ways will get pissed off that they can't upgrade
> because we're *forcing* them to install Net::SMTP. bmo would be an example of
this.

You misunderstand me. I'm saying it should be compulsory to _install_ it, not to
use it. That makes the checksetup.pl stuff much easier for the vast majority of
people who will just use it (because it's in the list of required modules, no
fuss) with the downside of making b.m.o. install it unnecessarily. 

I also want to prevent as many people as possible shooting themselves in the
foot by trying to use sendmail - as you can see from the newsgroup, it causes
problem after problem.

>> Would it make sense for "Disabled" to be just an option on this list, meaning
>> we don't need the "emaildelivery" param?
> 
> I could go either way on that... I did it this way because I wanted there to be
> an external toggle to turn email off that was separated both in the UI and in
> the code from the mail module stuff.

But the two are only consulted once, and within three lines of each other. "If
mail is enabled, get the method". 

>>Index: importxml.pl
>>+  my $messageInfo->{'envelopeTo'} = $to; ## XXX - This probably won't work
> 
>> Does it? :-)
> 
> You tell me! ;-) Actually, I *think* it won't... and the reason it won't is
> because $to isn't declared (in fact, I'll bet importxml.pl doesn't even compile.

Er... not wanting to appear rude, but why are patches coming for review without
even being compiled, let alone tested?

> I honestly can't remember. The Flag code needs a lot of eyes, because in looking
> at that section, I just noticed a number of "issues" with it (including the
> localization problem you noted in importxml.pl). I think the pref we're ignoring
> either has to do with no sending flag emails (can users set that as a pref?) or
> possibly with the envelopeFrom.

Yes, they can set that as a pref.

>> Could this module be called simply Mail.pm?
> 
> I wanted to differentiate it from Bugzilla::BugMail.
> 
> The module executes a function on mail, as opposed to figuring out how, when,
> and what to put in an email (as BugMail does), so I wanted to include a sense of
> action in the module, i.e. ProcessMail.

SendMail?

>> We tend to be using lower case field and function names in objects, I think.
> 
> I'll be happy to change it to the standard, if one exists. But I need to know
> for sure.

Just look at what the other objects do. If there's a clear majority, use it. I'm
a StudlyCaps man myself, but I've been doing lower-case because the rest are
like that.

>> Couldn't we just [% INCLUDE %] the header template in the body template?
> 
> Could we? I'm not a TT expert. I think I did it this way because if we did it
> that way, the email body template would have to figure out the name of the
> template based on its own name,

No figuring required. You just put

[% INCLUDE foo-header.txt.tmpl %] 

at the top of the email body template.

>> Is it not better to call this "message"? After all, as you pointed out in our
>> discussions about this earlier, we may send the same data by any number of
>> transports (IM, for example.)
> 
> I'd rather not change the name this late in the game, but you make an extremely
> good point.

Well I would hope so, it's your point :-) Come on, changing it's not hard - it's
a search and replace.

>> bugactivity should have a hyphen IMO. It's two words.
>>
>> Also, we tend to refer to this stuff as "bug changed" mail, don't we? 
> 
> I don't know; I don't think it's specified anywhere, so that's what I used.

I meant in conversation. But it's not a big issue.

>> Line too long ;-) Take advantage of PRE_CHOMP to break it up.
> 
> WHAT_CHOMP? (Remember... not a TT expert).

Sorry.

Break the line immediately before a [% DIRECTIVE %] of some sort, and TT will
put them back together for you.

>> The XML bug generation definitely should not be in a .txt file in the email
>> directory. move-bugs.txt.tmpl should delegate to the XML template for
>> show_bug.cgi, wherever that is.
> 
> I've since forgotten how this all worked exactly, but I believe this is here
> because move.pl sends email to move bugs. If it sends mail, it's in this
> directory. Now, the template is emitting XML, yes, but it's ultimately .txt
> because it's from the POV of the mail server... which sees XML as text.

What I meant was that this template should be removed, and the call to it
replaced with a call to template/en/default/bug/show.xml.tmpl, which is where
our canonical XML output template is.

Gerv

Comment 157

[Dave Miller [:justdave]]

>>> Is the technical distinction between "From" and "Envelope-From" relevant to 
>>> any mails that Bugzilla sends?
>> 
>> Not yet (as I remember), but it could be in the future (I built this in as a
>> feature).
>
>The reason I ask is that it makes things much more complicated. It also gives
>people the expectation that there's some need for a difference - I certainly
>spend a confused period looking for it. 
>
>Why might we ever need it? Can we add it in as and when we need it later?

If you want the bounces to go somewhere different from user replies.

Bounces will go to the Envelope-From.  Replies will go to the From:.

This will matter if we get the inbound email stuff integrated.  This is also how
it's done NOW, with the existing code.  The envelope-from is
[e-mail] while the From is [e-mail].

Comment 158

[Jouni Heikniemi]

Comment on attachment 139296 [details] [diff] [review]
Series II patch, v2

In <http://landfill.bugzilla.org/preed/bugzilla/show_bug.cgi?id=14>, requesting
approval from myself says:

--
Content-type: text/html
Software error:

Can't use string ("") as an ARRAY ref while "strict refs" in use at
Bugzilla/ProcessMail.pm line 99.

For help, please send mail to the webmaster ([e-mail]), giving
this error message and the time and date of the error. 
--

Secondly,

Failed Test	Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
t/001compile.t	   3   768    79    3	3.80%  29 32 59
t/004template.t   17  4352   373   17	4.56%  282-284 287 289 291 293 295-300
					       302-305
t/008filter.t	   1   256   161    1	0.62%  53
t/009bugwords.t    4  1024   161    4	2.48%  70 87 91 93


Some miscellaneous comments then. Sorry, there are a lot of nits, too. I'll try
to do a better review when the patch has its critical problems (the ones above)
in a bit better shape. I'll skip the filter/bugwords comments in templates as
they'll get handled when you make the testing suite run cleanly.


>Index: CGI.pl
>===================================================================

>+    my $messageInfo = {};

Nit: How about "$message"? (Info, Data et al. usually mean nothing)
Repeat this comment a kazillion times later on. :-)

>+    ## Explicitly set the From: to be the BZ server
>+    $messageInfo->{'envelopeFrom'} = Param('bzmailfrom');
>+    $messageInfo->{'envelopeTo'} = $login . Param('emailsuffix');
>+    $messageInfo->{'login'} = $login;
>+    $messageInfo->{'password'} = $password;

While I agree with Gerv's comments on making header passing as easy as
possible, I'd also appreciate an easy way to separate params that are used for
message content and those used for the message transmission (headers). At first
I thought the prefix "envelope" was going to do it, but I'm less certain on it
now. What do you think of this?

>Index: Token.pm
>===================================================================
>+    ## From: doesn't change

Btw, is there a specific reason for using double # in comments?

>+    my $messageInfo = {};
>+    $messageInfo->{'envelopeFrom'} = Param('bzmailfrom');
>+    $messageInfo->{'envelopeTo'} = $loginname . Param('emailsuffix');

This three-line structure gets repeated so often that I'm starting to wonder if
a shortcut method would be useful? I mean something like the following:

my $message = CreateMessage(-to => $loginname);

(where "from" would default to bzmailfrom and to would have emailsuffix
appended automatically)


>Index: checksetup.pl
>===================================================================
>+if (!$netsmtp && !$silent) {
>+    print "If you you want Bugzilla to send mail directly using the SMTP ";

I tend to agree with Gerv that requiring Net::SMTP would probably be ok. The
couple of big installations which definitely want to use sendmail now can
either hack code or do something else, but checksetup.pl's printout for the
optional modules is absolutely horrid, and something this important shouldn't
be buried down there.

>Index: defparams.pl
>===================================================================

>+  {
>+   name    => 'emaildelivery',
>+   desc    => 'Is bug and other email from Bugzilla currently \'on\'?' .
>+              '(site-wide toggle)',
>+   type    => 'b',
>+   default => 1,
>+  },

I'd go JayPee's way here. While Gerv is right that the real need for this is
minimal code-wise, it's sometimes useful to be able to disable and enable mail
in a no-fuss way; touching the mailmodule setting is harder if you do this with
an automated script, since you have to remember your mail settings on
reactivation. This is a pretty theoretical point in this case, though...
Anything goes for me.

>+              '<br><b>Net::SMTP</b> - domain name of your SMTP-speaking mail '. 
>+              'server<br><b>Sendmail</b> - location of the sendmail binary' .
>+              ' and any arguments to pass during execution (be sure to add' .
>+              ' <tt>-t</tt> and <tt>-i</tt> if actually using sendmail)<br>' .
>+              ' <b>Debug</b> - location of the debug file', 

This is sort of funny. At first, you pull the values for mailmodule param from
the module, but yet these explanations get hardcoded into this file. Although
I'm certainly not insisting on it, would it be considerably harder to make
these use the same data source (return the help text from ProcessMail as well)?
Design-wise, I'd find that quite a lot better.

>+   desc    => 'Email address Bugzilla server email appears to come from ' .
>+              '(full address)',
>+   type    => 't',
>+   default => 'bugzilla-daemon',

I was confused by the desc; how about 'The "from" address for email sent by
Bugzilla'? Also, the meaning of "full address" is somewhat messed up by the
incomplete default address. I propose having "[e-mail]" as a
default value.


>Index: Bugzilla/ProcessMail.pm
>===================================================================

>+## a circular depedancy in defparams and causes stuff to blow up. This is why

Nit: dependency

>+## The passed in message hash contains template/email message-specific 
>+## content; the template will ensure it gets what it needs. BUT, all msgHashs
>+## MUST have:
>+##   -- 'envelopeTo' (the recipient) 
>+## 
>+## *IF* bugmailfrom is 'fromuser', then we also MUST HAVE:
>+##   -- 'envelopeFrom' (the sender) 
>+##
>+
>+sub MailMessage {

How about moving that comment inside the sub? That's not really a comment
describing the sub in its whole, but rather a data structure handled inside it.

>+    ## the note in Bugzilla.pm) and the global vars hash (using it should
>+    ## be ok)
>+    my $vars = $::vars;

... reading it should be ok, but can modifying it (later in the code) cause
unwanted side effects in the original template processing? There is at least a
theoretical chance for a namespace conflict...

>+    $msgHash->{'envelopeFrom'} = Bugzilla::Config::Param('bzmailfrom') if 
>+     (!exists($msgHash->{'envelopeFrom'}));

Can't we just do 

$msgHash->{'envelopeFrom'} ||= Bugzilla::Config::Param('bzmailfrom');

?

>+        if (!exists($msgHash->{"$field"}) || 
>+            Bugzilla::Util::trim($msgHash->{"$field"}) eq '') {

This |"$field"| is so confusing. How about just |$field|?

>+    ## To: is a bit more complicated: it has to combine any possible Cc: 
>+    ## headers with the envelopeTo 
>+    $msgHash->{'headerto'} = $msgHash->{'envelopeTo'};
>+    my $to = [];
>+    push(@{$to}, $msgHash->{'envelopeTo'});
>+    if (exists($msgHash->{'envelopeCc'})) {
>+        $msgHash->{'headercc'} = $msgHash->{'envelopeCc'};
>+        foreach my $addr (@{$msgHash->{'envelopeCc'}}) {
>+            push(@{$to}, $addr);
>+        }
>+    }

Hmm... I think a better solution would be to push all the addresses into a hash
and then have the hash keys as the "to" list (I think we already do this in
bugmail.pm somewhere). This would weed out possible dupes in cc lists and to
lists as well, while also making it easier to later support a third group of
recipients (possibly bcc). 


>+    ## with the the wrong module. 

Nit: double "the".

>+    return "Invalid mail module '$module' selected " if 

Nit: Extra space at the end.

>+## Finally, add a key->value pair that is the name of the "module" and a CODE 
>+## reference to a function that does the mailing to %MailModules up top and 
>+## the name of the "module" and a CODE reference to the module's param 
>+## validator (for Bugzilla::Config/defparams to use) to %ModuleParamCheckers 
>+## up top. Presto... instant new mail handling module.

If you don't implement my suggestion in defparams (on pulling the mailmodargs
desc from here), remind module customizers to also change that in defparams.

>+sub CheckSendmailParams {
>+    my $newArg = shift;

Here, throw in a Windows check for good measure (the pipe open syntax would
fail anyway).

if ($^O =~ /MSWin/)
  return "Sendmail interface does not work on Windows";


>+    (-x $executable) 
>+     || return "Specified Sendmail binary ($executable) doesn't exist or " . 
>+     "isn't executable";

Aw come on, don't spare the keyboard, tap in the |if|. This is horrible. O:-) 

The checks should all be written in a uniform manner; either

if (!x)
  return y;

OR

return y if (!x);

OR

x || return y;


I massively prefer the first one.

>+    open(MAILDBUG, ">>$mailDebugFile") or 
>+     ::ThrowCodeError('debug_mailing_failed', { openrv => $! } );

Why do you call this thing "MAILDBUG" instead of MAILDEBUG?


>Index: template/en/default/email/confirm.html.tmpl
>===================================================================

So... Why are we touching this template? What are the semantics of the "email"
template directory? All email-related stuff or just the email messages?

>Index: template/en/default/email/global-header.txt.tmpl
>===================================================================

Is there anything we can do here to ensure that our mail gets interpreted as
plain text? We do write out many unfiltered potentially harmful strings, but of
course doing HTML filtering would be odd, since the mail is essentially
plaintext. I'm just wondering if any HTML contained inside can make mail
clients recognize it as HTML in the absence of a content type specification.

Comment 159

[J. Paul Reed [:preed]]

> You misunderstand me. I'm saying it should be compulsory to _install_ it, not to
> use it. That makes the checksetup.pl stuff much easier for the vast majority of
> people who will just use it (because it's in the list of required modules, no
> fuss) with the downside of making b.m.o. install it unnecessarily. 

I disagree.

I think it shouldn't be compulsory until one version after the feature is
introduced.

Dave can make the call.

> Er... not wanting to appear rude, but why are patches coming for review without
> even being compiled, let alone tested?

You're talking about the bug moving code, which just happens to use email to get
its work done.

This is an unusually large patch, and it's getting more painful to deal with
every week it sits here because people are picking nits about variable names. I
think you can agree that this code is going to mostly be used for *bugmail* and
*flags*, so that's where the majority of the effort has gone into testing.

If you're volunteering to test the moving code, then by all means...
 
> SendMail?

I fixed the double hashes. I fixed the $messageInfo thing Jouni mentioned.

I'm not fixing any other variable names. If you have a problem with them, submit
a patch to fix it.

I'm tired of arguing with people over what modules and variable names should be
called when the ones I picked are just fine.

> No figuring required. You just put
> 
> [% INCLUDE foo-header.txt.tmpl %] 
> 
> at the top of the email body template.

When I finished this change, I finally remembered why I had it that way: it's
because headers and the email message itself are intrinsically different things
and we can't presume how mail modules are going to want to handle them. So,
they're passed to each module as two separate variables; thus, they have to be
processed as two separate process() calls to keep the strings separate, and thus
they need to have two different templates.

I'm looking at simplifying those headers, but honestly, I think what I have in
there now is fine, and there's a very good reason why I'm doing it.

Contrary to popular belief, I actually have a small amount of clue here... I
just keep spending time looking at stupid things like whether the hash name
should be called 'emailmsg' or just 'email' that I've long since forgotten why I
did things the way I did.

You too, Gerv, have lost the big picture here.

Comment 160

[J. Paul Reed [:preed]]

(In reply to comment #158)

> Can't use string ("") as an ARRAY ref while "strict refs" in use at
> Bugzilla/ProcessMail.pm line 99.

*Should* be fixed... let me know if you reproduce it.

> Secondly,
> 
> Failed Test	Stat Wstat Total Fail  Failed  List of Failed
> -------------------------------------------------------------------------------
> t/001compile.t	   3   768    79    3	3.80%  29 32 59
> t/004template.t   17  4352   373   17	4.56%  282-284 287 289 291 293 295-300
> 					       302-305
> t/008filter.t	   1   256   161    1	0.62%  53
> t/009bugwords.t    4  1024   161    4	2.48%  70 87 91 93
> 

I'm still failing some of these, but they look to me to be inconsequential. Let
me know if I'm reading it wrong.

> Nit: How about "$message"? (Info, Data et al. usually mean nothing)
> Repeat this comment a kazillion times later on. :-)

Changed.
 
> While I agree with Gerv's comments on making header passing as easy as
> possible, I'd also appreciate an easy way to separate params that are used for
> message content and those used for the message transmission (headers). At first
> I thought the prefix "envelope" was going to do it, but I'm less certain on it
> now. What do you think of this?

I guess I'm confused on what your question/suggestion is. Are you saying that I
reference this data as "headerFrom" elsewhere and it's "envelopeFrom" here and I
should just pick one? I'll agree with that... but is that what you're pointing out?

> Btw, is there a specific reason for using double # in comments?

Should be removed.

> This three-line structure gets repeated so often that I'm starting to wonder if
> a shortcut method would be useful? I mean something like the following:
> 
> my $message = CreateMessage(-to => $loginname);
> 
> (where "from" would default to bzmailfrom and to would have emailsuffix
> appended automatically)

I don't think that's a bad idea, but I'd like to concentrate on getting what's
there fixed and getting what's there in. There are obviously going to be issues
that I'll want to polish along with bug fixes.

This falls under there, I think.
 
> I tend to agree with Gerv that requiring Net::SMTP would probably be ok. The
> couple of big installations which definitely want to use sendmail now can
> either hack code or do something else, but checksetup.pl's printout for the
> optional modules is absolutely horrid, and something this important shouldn't
> be buried down there.

I defer to Dave on this; he'll have a better idea of what installations "want."


> This is sort of funny. At first, you pull the values for mailmodule param from
> the module, but yet these explanations get hardcoded into this file. Although
> I'm certainly not insisting on it, would it be considerably harder to make
> these use the same data source (return the help text from ProcessMail as well)?
> Design-wise, I'd find that quite a lot better.

Me too. I think the main problem with it is there's no real good place to
manipulate the data in defparams; ideally, GetMailModuleList() should return a
hash of name/info/argument "help strings," but it doesn't do that because we'd
have to functions in defparams to format the strings correctly. I don't really
have a problem with doing that, but... again, that's polish for later.

It also might change if we finish templatizing admin pages.

> I was confused by the desc; how about 'The "from" address for email sent by
> Bugzilla'? Also, the meaning of "full address" is somewhat messed up by the
> incomplete default address. I propose having "[e-mail]" as a
> default value.

Good point; fixed.



> How about moving that comment inside the sub?

Done.

> 
> >+    ## the note in Bugzilla.pm) and the global vars hash (using it should
> >+    ## be ok)
> >+    my $vars = $::vars;
> 
> ... reading it should be ok, but can modifying it (later in the code) cause
> unwanted side effects in the original template processing? There is at least a
> theoretical chance for a namespace conflict...

Yeah, there could be; I try to make the stuff pretty email-specific i.e.
'envelopeHeader', 'header*', etc. I totally agree in principle, but... I kinda
want to just leave it for now.

> Can't we just do 
> 
> $msgHash->{'envelopeFrom'} ||= Bugzilla::Config::Param('bzmailfrom');

Yes.

> This |"$field"| is so confusing. How about just |$field|?

Yeah... I wrote this code when I was using another style that I often (am
forced) to use.

> >+    ## To: is a bit more complicated: it has to combine any possible Cc: 
j> >+    ## headers with the envelopeTo 
> >+    $msgHash->{'headerto'} = $msgHash->{'envelopeTo'};
> >+    my $to = [];
> >+    push(@{$to}, $msgHash->{'envelopeTo'});
> >+    if (exists($msgHash->{'envelopeCc'})) {
> >+        $msgHash->{'headercc'} = $msgHash->{'envelopeCc'};
> >+        foreach my $addr (@{$msgHash->{'envelopeCc'}}) {
> >+            push(@{$to}, $addr);
> >+        }
> >+    }
> 
> Hmm... I think a better solution would be to push all the addresses into a hash
> and then have the hash keys as the "to" list (I think we already do this in
> bugmail.pm somewhere). This would weed out possible dupes in cc lists and to
> lists as well, while also making it easier to later support a third group of
> recipients (possibly bcc). 


> If you don't implement my suggestion in defparams (on pulling the mailmodargs
> desc from here), remind module customizers to also change that in defparams.

I will, for now.

> >+sub CheckSendmailParams {
> >+    my $newArg = shift;
> 
> Here, throw in a Windows check for good measure (the pipe open syntax would
> fail anyway).

Done.

> Aw come on, don't spare the keyboard, tap in the |if|. This is horrible. O:-) 

/me is a l33t p3R1 h@Xx0r.

> The checks should all be written in a uniform manner; either
> 
> if (!x)
>   return y;
> 
> OR
> 
> return y if (!x);
> 
> OR
> 
> x || return y;
> 
> 
> I massively prefer the first one.

I only did the second one because perl, IIRC, *USED* to barf on that C-like
syntax; does 5.8 not now?

> Why do you call this thing "MAILDBUG" instead of MAILDEBUG?

To save the 'e'. They're endangered, you know. Fixed.

> 
> 
> >Index: template/en/default/email/confirm.html.tmpl
> >===================================================================
> 
> So... Why are we touching this template? What are the semantics of the "email"
> template directory? All email-related stuff or just the email messages?

I screwed up. At least, I'm pretty sure I did. It shouldn't be in this diff.

> Is there anything we can do here to ensure that our mail gets interpreted as
> plain text? We do write out many unfiltered potentially harmful strings, but of
> course doing HTML filtering would be odd, since the mail is essentially
> plaintext. I'm just wondering if any HTML contained inside can make mail
> clients recognize it as HTML in the absence of a content type specification.

Depends on the client; braindead, insecure ones might try to do that. Adding it
would be a good idea... but again, it's polish I want to do later (and you can
remind me of this then! ;-)

Comment 161

[Gervase Markham [:gerv]]

> If you're volunteering to test the moving code, then by all means...

All I was suggesting was "perl -c[T]" on files you changed.

> I think it shouldn't be compulsory until one version after the feature is
> introduced.

Then perhaps, during the interim period, you'd like to join us in the
n.p.m.webtools newsgroup supporting the large numbers of people who have trouble
configuring Bugzilla's email capabilities. This problem outnumbers all other
installation issues put together.

> You too, Gerv, have lost the big picture here.

On the contrary. I have my eye on the big picture of all of the things that this
bug blocks, and the amount of time between revisions of it. Most particularly,
the templatisation of mail, which blocks full user UI templatisation, which was
a 2.16 goal.

You'll note that I consider this bug so important that it took me less than a
day to review the last patch you posted, despite it being very big.

Gerv

Comment 162

[J. Paul Reed [:preed]]

Attachment: Series II patch, v3

This takes care of most of the review comments, save:

-- some of the TT comments
-- some stuff that I'd like to defer to later

Anyway, it should be closer...

*crosses fingers*

Comment 163

[Jouni Heikniemi]

Comment on attachment 141651 [details] [diff] [review]
Series II patch, v3

First, replies from the last round:

(testing suite results)
> I'm still failing some of these, but they look to me to be inconsequential. 
> Let me know if I'm reading it wrong.

Any testing suite failures are critical, as they cloud other failures. 

The stuff in 005 is probably not related to this patch.

Failed Test	Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
t/001compile.t	   2   512    79    2	2.53%  29 59
t/005no_tabs.t	   1   256   239    1	0.42%  60
t/009bugwords.t    1   256   160    1	0.62%  70


> I guess I'm confused on what your question/suggestion is. Are you saying that 
> I reference this data as "headerFrom" elsewhere and it's "envelopeFrom" here 
> and I should just pick one? I'll agree with that... but is that what you're 
> pointing out?

That's a good point too and I agree, but it's not what I originally meant. What
I propose next is sort of a supercharged version of that, though. I was
confused by the fact that you do 

$message->{'whateverheader'} = foo;
$message->{'somevariable'} = bar;

Where "somevariable" is something for the body, and "whateverheader" is the
header content. I think it would be good to have the message envelope
information in a different hash and the template-used vars in another, so that
we'd have:

$messageHeaders->{'to'} = foo;
$message->{'login'} = bar;

Of course, big part of this will be eliminated if a shortcut notation for
creating new message objects is devised.

> I defer to Dave on this; he'll have a better idea of what installations 
> "want."

Very well. Dave?

> > >+    ## the note in Bugzilla.pm) and the global vars hash (using it should
> > >+    ## be ok)
> > >+    my $vars = $::vars;
> > ... reading it should be ok, but can modifying it (later in the code) cause
> > unwanted side effects in the original template processing? There is at least 
> > a theoretical chance for a namespace conflict...
> Yeah, there could be; I try to make the stuff pretty email-specific i.e.
> 'envelopeHeader', 'header*', etc. I totally agree in principle, but... I kinda
> want to just leave it for now.

I can understand why you want to leave it, but shouldn't we try to create a
solution that either a) removes the risk or b) documents it clearly enough?
Now, if we think about token mail for example, we have fields like vars->login
there. Those names could very well be used both in the capsulating html
template AND the mail template inside, thus causing a conflict. If you rely on
prefixing, let's make it a formal standard. Changing this afterwards is
possible, but it's probably even harder later on.

(different syntaxes for if-returns)
> I only did the second one because perl, IIRC, *USED* to barf on that C-like
> syntax; does 5.8 not now?

You may actually need braces, but I don't think that's a problem. The second
one is fine for me.


New stuff, functional issues:

Password change mail has two equal links (that's not the intention; one is
supposed to be proceed and the other one "cancel").

Request mails are broken. I get stuff like the following:
--
Jouni Heikniemi <[e-mail]> has   for tarkistus:
 13: Test
http://xxx/bugzilla/tip/show_bug.cgi?id=13 
--
Also, the request mail subjects have an extra "Subject:" prepended to them, so
they start with "Subject: Subject:"


The mail sent when trying to change the users mail address has similar
problems:
--
 has received a request to change the email address
for your account to [e-mail].
--

Also, the one sent to the new address has again two equal links. When I click
one of them, I get an internal error:
--
Required email message info missing; missing hash entry was: 'envelopeTo'
--

The patch doesn't apply quite cleanly; please check the files once more? Some
of the issues mentioned here may be consequences of skipping the errors.
--
patching file template/en/default/account/cancel-token.txt.tmpl
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file
template/en/default/account/cancel-token.txt.tmpl.rej
patching file template/en/default/account/email/change-new.txt.tmpl
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file
template/en/default/account/email/change-new.txt.tmpl.rej
patching file template/en/default/account/email/change-old.txt.tmpl
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file
template/en/default/account/email/change-old.txt.tmpl.rej
patching file template/en/default/account/password/forgotten-password.txt.tmpl
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file
template/en/default/account/password/forgotten-password.txt.tmpl.rej
Reversed (or previously applied) patch detected!  Assume -R? [n]
Apply anyway? [n]
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file
template/en/default/request/email.txt.tmpl.rej
--

When trying to create a new account:

--
Internal Error
...
URL:
http://.../bugzilla/tip/createaccount.cgi?login=jouni-hmm2%40heikniemi.net&real
name=
Template::Exception at /usr/lib/perl5/5.8.0/CGI/Carp.pm line 312.
--


I'm not able to do a line-by-line code review at this point; the issues with
reuse of vars hash and the still ongoing debate on how headers should be
constructed and passed makes it hard to take a really good look at the code.
Also, the functional deficiencies in this patch will probably cause lots of
lines to shuffle anyway. 

I'd really appreciate a few paragraphs of documentation on the following
questions:

- what are the most important changes?
- how are the mail messages constructed and sent (from a code perspective)?
  +- the role of the MailMessage sub?
- what are the naming conventions in the famous $message hash?
  (and yes, I really think I need this information to understand it)
- How are you intended to use the mail header templates? There are several
  different approaches here, from the reasonably trivial of 
  whinemail-header's style (where you only customize the mail subject)
  to monstrous catenations in flag-header's style.

We're going to have to update developer's guide on this stuff anyway, so the
answers will probably be good for that work, too.

Comment 164

[Dave Miller [:justdave]]

:: sigh ::

We'll do this like immediately after we branch.

Comment 165

[Dave Miller [:justdave]]

Attachment: Series II v4

Here's JayPee's last patch with a little bit of cleanup to it.	I'm sure I've
not covered everything.  If anyone wants to look at it and offer feedback, I'd
appreciate it.	I'm going to continue to hack on it and take it over from here.

Comment 166

[Dave Miller [:justdave]]

Attachment: Series II v4 (corrected)

Sorry, that last upload was corrupted, try this one.

Comment 167

[Dave Miller [:justdave]]

OK, my first comments after trying this out in a real-life Bugzilla is that
there's no error checking in the Net::SMTP code.

Mar 24 22:12:58 sinclair sendmail[2467]: i2P6CwRt002467: ruleset=check_mail,
arg1=<[e-mail]>, relay=localhost [127.0.0.1],
reject=553 5.1.8 <[e-mail]>... Domain of sender
address [e-mail] does not exist

Bugzilla told me it sent these mails successfully, but I'm sure you can see what
happened. :)

Comment 168

[Dave Miller [:justdave]]

Attachment: Series II v4 (corrected really)

OK, here we go again. :)  This one actually includes all of the new files that
are supposed to be added.

Comment 169

[Jouni Heikniemi]

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

Comment 170

[Dave Miller [:justdave]]

Those of you watching this for Win32 compatibility reasons, that moniker just
got moved to bug 49893, which addresses the issues hurting Windows, but doesn't
completely solve this bug, thus the swapped dependencies.

Comment 171

[Michael Nacey]

Any of these fixes mean that ln -s /usr/sbin/sendmail /usr/lib/sendmail is no
longer necessary for bugzilla to send mail? Not sure where that original bug
went to.

Comment 172

[Nick Barnes]

This bug seems to be taking forever to get anywhere.  So I've made bug 270611
just to address the sendmail path issue, which could be fixed with a tiny patch
in the near future, long before this monster patch hits the tree.

Comment 173

[Dave Miller [:justdave]]

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

Comment 174

[Max Kanat-Alexander]

This bug has many comments, and a Really Cool Patch.

However, this Really Cool Patch is obviously (note that it's been here since
March) too large to review or actually check in. So, all the code is here, let's
just split it up into some separate bugs.

I know that we can write one patch that does everything. It's fun, it's easy,
and it will never land. :-)

I haven't read this whole bug, so I don't know how to split it. Any ideas? It
looks like the most important part is creating ProcessMail.pm, right? That
should be a separate bug.

Comment 175

[Jouni Heikniemi]

I think about everybody agrees this bug has become something that isn't really
fixable anymore. Even further, some of the functionality addressed herein is no
longer even desired. Bug 49893 is an attempt to fix the most critical part of
this. There we've been planning to totally drop sendmail; that would make the
mailing mechanism very much simpler. That's the other critical half of this bug.

The other half is the better templatization of mail stuff. That could probably
be tackled as another rather big bug (at least the key parts). Further mail
enhancements (optimizing the usage of a single SMTP session, for example) made
necessary by those two could then be handled as separate, considerably smaller bugs.

Comment 176

[Dave Miller [:justdave]]

Note that this bug was originally a metabug, and everything got consolidated
here after someone came up with a patch that supposedly fixed all of them in one
shot.  Then someone else came up with a different patch, and nobody could agree
on which one to do.

Nobody has time to deal with the whole shot anymore, we should turn this back
into a metabug and re-swap the dependency list.

Comment 177

[Gervase Markham [:gerv]]

Given the number of comments and attachments here already, IMO it would be
better to resolve this one and start again from scratch.

Gerv

Comment 178

[Max Kanat-Alexander]

OK, I've re-worked this bug to be a meta-bug once again, and corrected
dependencies. I will also break out some things from here into new bugs. If I
miss any, please file a bug and make it block this one.

Comment 179

[Frédéric Buclin]

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

Comment 180

[Marc Schumann [:Wurblzap]]

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

Comment 181

[Max Kanat-Alexander]

As most of the blockers to this have been fixed, I think we can consider this as pretty much fixed. At the least, as far as the original goals of improving MTA supoprt go, we've made excellent progress in the last few years.