Closed Bug 124174 Opened 22 years ago Closed 21 years ago

processmail should be a package

Categories

(Bugzilla :: Email Notifications, defect, P1)

2.14.1
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: jouni, Assigned: preed)

References

Details

(Whiteboard: [needed for Win32bz])

Attachments

(2 files, 12 obsolete files)

It would be good if globals.pl defined a sub CallProcessMail which contained 
the system() call, so that all the separate files could just call that single 
sub. In a Win32 installation it's many times necessary to whip up several 
parameters to the system call, and it's rather cumbersome to run around the 
source files pasting the new code everywhere. Combining the calls into a single 
place would help changing the system call syntax.

I currently use the following: 

sub CallProcessMail {
 system("d:\\perl\\bin\\perl.exe", "-I\\\\t1\\\\s1\\bugzilla\\", "\\\\t1\\\\s1
\\bugzilla\\processmail.pl", @_);
}
Actually, processmail should be a package/sub, and then we wouldn't have to go
through all these problems.

I'll attach a patch which both moves it to a sub, and causes processmail to
return values rather than outputting them directly.

Callers of ./processmail have been changed. Once post_bug abnd process_bug are
templatised then the extra template code just becomes an INCLUDE, like it is for
attachments.

I've kept the interface to processmail as is for this patch.

mattyt, timeless: Here's the patch I was talking about.

Dave: do we want this for 2.16? As well as fixing the windows issues, it makes
the templatisation a bit easier, and allows us to defer calling processmail to
when we display it (and can then FLUSH, too, after each one), so that you still
see some results even if the mailserver is down.

Note that after applying the patch, you need to |mv processmail Processmail.pm|
- I've done the diff this way so that the differences can easily be seen.
Assignee: jake → bbaetz
Priority: -- → P2
Summary: processmail calls should go through a single sub → processmail should be a package
Attached patch patch (obsolete) — Splinter Review
Status: NEW → ASSIGNED
Keywords: patch, review
Target Milestone: --- → Bugzilla 2.16
I think we're too close to 2.16 for this.

Moving off
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
I just posted bug 140782 which has a strong relationship to this one. This
should be 2.16 imo, though I wholly understand why you wouldn't want it that way.

The processmail stuff is really way too complicated to be made to work on
Windows platforms. I say this, having just worked on it several hours with the
latest CVS version. ;-I
Blocks: 140782
OK, moving back to 2.16 and making a blocker.  We just completely broke email
altogether on Win32 (not sendmail, but processmail - see bug 140782), and this
is the right way to fix it.  Although we still aren't officially supporting
Win32, we shouldn't break things that previously did work on Win32.
Severity: enhancement → blocker
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
If this patch gets revived:

- The current bug/process/results.html.tmpl template should be enhanced to
contain the "enter bug" results, and bug/create/created.html.tmpl changed to use
it. (Same with attachment/updated.html.tmpl IMO.)
- let's not use StudlyCaps for CGI parameters
- Should we take the opportunity to kill off data/nomail? Email prefs are the
correct way to do this now.
- Can we think of a better name than "Processmail" for this? I've never liked
that name.

Gerv
Processmail the module could be called 'BugChangeNotifier' (or something like
that) to indicate that: 

1) it is used to send mail about bug changes, not just any mail
2) it is essentially a notifier, not a mailer: mail is the way to do it now, but
in the future other notification methods should be available and possibly
incorporated into the same module.

Attached patch v2 (obsolete) — Splinter Review
OK, lets try this. Now that all the places which want to use this are
templatised, its a bit simpler.

Before applying, do |cp processmail Bugmail.pm|

Gerv: 

> - The current bug/process/results.html.tmpl template should be enhanced to
contain the "enter bug" results, and bug/create/created.html.tmpl changed to
use
it. (Same with attachment/updated.html.tmpl IMO.)

I don't know what this means. Take a look at what I've done, and see if this is
what you wanted

> - let's not use StudlyCaps for CGI parameters

bah. _ sucks, and InterCaps is easier to type. Fixed anyway, in most places.

> - Should we take the opportunity to kill off data/nomail? Email prefs are the

correct way to do this now.

No. We are taking this opportunity to hoepfully fix this for win32. Nothing
else. The only other thing I am fixing is that the easiest way to do this is to
have it go from the template, so that if mail is slow and there are lots of
dependancies, you at least see what is happening in stages, like 2.14 does.
This doesn't actually really matter, since processbug uses lots of little
templates, but at least you'll be able to see which bug its dying on.

> - Can we think of a better name than "Processmail" for this? I've never liked

that name.

I used Bugmail.pm instaead of BugNotifications.pm, or something. because this
is still strongly related to bugmail, and would require a rewrite to be
different.

Other comments:

- in globals.pl, I have to dereference the hash (It took me ages to work out
that thats what the problem was). Alternatives include just not passing a ref
in at all (can we then get at it from within TT?) or having Bugmail::Send take
a hash reference, rather than a hash.

- data/nomail sucks. I read it in once per use, which should have a slight perf
improivment when we go through lots of dependancies, but has the downside that
if I didn't use the wrapper func in globals.pl, .we'd do that for every cgi Or
will a use be pulled in always, anyway, at compile time? I'll have to test. If
so I'll just read it in once per bug, keeping the current usage.

- 'people' isn't such a good name. Suggestions for a better one?

- Its been lightly tested - it sends mail, and appears to get most of the
-force stuff right. I haven't done a detailed test, though, esp with the cc
forcing code.

- In the new tempalte, I don't seem to be able to take stuff out of the array
into a pair of variables - I have to use .0 and .1. Is there a better way to do
this with TT, or is this a TT limitation?

- can someone on windows please test this, and let me know if it fixes those
problems?
Attachment #73428 - Attachment is obsolete: true
Ok, that patch does it. Preliminary testing shows it works fine on Windows,
although you still have to spank Bugmail.pm a bit to enable mailing on Win
platforms (as Windows boxes don't tend to have a '/usr/lib/sendmail' handy).

One code change I had to do to get it working:

+    @{$force{'CClist'}} = ($people{'cc'} && scalar($people{'cc'}) > 0) ?
map(trim($_), $people{'cc'}) : ();

to

@{$force{'CClist'}} = ($people{'cc'} && scalar($people{'cc'}) > 0) ?
map(trim($_), @{$people{'cc'}}) : ();

(note the added deref near the end of the line). Wonder if this change breaks
anything in your patch? It worked fine with me. 
Bug 140782 is the real (potential) release blocker; the fix for this bug fixes
that one but is not otherwise necessary for the release.

At this point in the development cycle only the simplest and least risky fix for
a blocker bug should be considered.  Is the fix for this bug really the simplest
and least risky approach?  For that matter, is it really necessary to block a
release because of a Win32 issue with code that had to be customized by Win32
installers of every previous release?
There is going to be a release candidate, I think. If this is true, then this
fix will necessarily be tested by everyone trying it out, since hardly anybody
will play without completely without email. And we have already a report that
it's working (even) on windows, so I don't see the problem with this. 

Also note that email handling currently is a mess which is complained about
(e.g. on the newsgroups) quite often.
Oops, this patch is indeed more complicated than just centralizing the calls to
sendmail.
Sendmail has nothing at all to do with this patch - on windows, we fail before
even getting there.

The only way arround it would be to print all of the template up to where we;d
call process mail, thencall processmail, then do the test of it. The other way
is to change all that code to use work explictly, ad mentioned in the docs, but
that would then need testing on perl5.005, too...

The diffs are reasonably trivial though, except for that type in the cc bit (I
mentioned that I hadn't tested that part...)

We need processmail to be able to be called directly, which means making it a
package. Or we could write to a temp file, I suppose, but that will have its own
issues.

One other issue - why does processmail-the-script set the umask to 0? I removed
that, and it seemeed to work - does processmail create files anymore, or is this
a relic from teh pre-bug_activity days?
Myk, re comment 10:

While it's true that hacking the source has always been necessary on Win32, this
is a different issue. If the Bugzilla-Sendmail border breaks, that's a somewhat
external issue which can be left for the Win32'ers to fix. But if, say,
process_bug-processmail border breaks, that's a Bugzilla bug (albeit caused by a
lacking feature in ActiveState Perl). 

If this fix isn't checked into 2.16, everyone running Win32 will have to
manually apply something like bbaetz's patch AND change processmail to use
another mailer. This patch removes the first step (which is a new issue), and
the latter is way more trivial (and exactly what's been needed for the past
releases as well).
> We need processmail to be able to be called directly, which means making it a
> package.

Can't we do what I did with bug_form.pl - wrap the entire thing in a subroutine,
require "processmail" at the top, and call the subroutine? That's only a few
lines of change.

Gerv
No, because that would still print to stdout - thats the reasl problem. As I
said, we could redirect to a temp file, then read in from that file afterwards.
But thats really messy and ugly, and has its own security issues.

I've basically done what you did, really. Making it a package involved changing
the file's name, and adding |Package Bugmail;| to the top (Ok, and the AUTOLOAD,
too) In fact, the 'new' bug_form.pl should have been done that way.

This is not a fundamental change in how things work - just look at the changes
visually. If you want me to walk though the changes to processmail/Bugmail.pm,
then I will, but they're really quite simple.
Comment on attachment 81491 [details] [diff] [review]
v2

>+if (exists $::FORM{'rescanallBugmail'}) {
>+    use Bugmail;
>+    Status("OK, now attempting to send unsent mail");
>+    SendSQL("SELECT bug_id FROM bugs WHERE lastdiffed < delta_ts AND delta_ts < date_sub(now(), INTERVAL 30 minute) ORDER BY bug_id");
>+    my @list;
>+    while (my @row = FetchSQLData()) {
>+        push @list, $row[0];
>+    }

Isn't
while (MoreSQLData()) {
    push (@list, FetchOneColumn());
}
the right way to do this?

>+    print "<br>" . scalar(@list) . " bugs found with possibly unsent mail.";

Do you need "scalar" here?

>+    foreach my $id (@list) {
>+        if (detaint_natural($id)) {
>+            Bugmail::Send($id);
>+        }

Why are we detainting bug ids we get from the DB?

>-    print("Run <code>processmail rescanall</code> to fix this<p>\n");
>+    print qq{<a href="sanitycheck.cgi?rescanallBugmail=1">Click here to send these mails</a><p>\n};

Nit: add a full stop outside the link.

>+    # SendBugmail- sends mail about a bug, using Bugmail.pm
>+    'SendBugmail' => sub ($;$) {

House style seems to be not to use function prototyping ($;$).

>-      [% mailresults %]
>+      [% PROCESS "bug/process/bugmail.html.tmpl" id = bugid %]

<sigh> I'm not quite sure how it got so we have four different templates for
that square box you get when you change a bug or something else. The
Right Thing is for all of these to be centralised, and bugmail.html.tmpl
to be part of that one template. However, I'm not sure if we'll manage 
this for 2.16 :-(
(The template which appears to be doing the unifying is 
bug/process/results.html.tmpl.)

>+  # Contributor(s): Bradley Baetz <bbaetz@student.usyd.edu.au>
>+  #%]
>+
>+[%# INTERFACE:
>+  # id: bug id to send mail about
>+  # people: hash for processmail
>+  #%]

That's a bit wet :-) Try:

  # people: hash. People involved in this change. Hash has up to five elements:
  #   changer: string. The login name of the user who made the change.
  #   For bug changes:
  #   owner: string. The login name of the bug assignee.
  #   reporter: string. The login name of the bug reporter.
  #   qacontact: string. The login name of the bug's QA contact. Optional.
  #   cc: list of strings. The login names of those on the CC list.

>+  # people: hash; processmail params. Optional

As above.

>+# This is run when we load the package
>+if (open(FID, "<data/nomail")) {
>+    while (<FID>) {
>+        $nomail{trim($_)} = 1;
>+    }
>+    close FID;
>+}

Can't we just remove support for this entirely (cut out the code)? 
Impact: it wouldn't work any more if people are using it.
Possible mitigation:
- Get checksetup.pl to read the file, if it exists, and turn off email
prefs for all the users in it. Then delete the file. Do we even need
to do that?

>+# This is a bit of a hack, basically keeping the old system()
>+# cmd line interface. Should clean this up at some point

Is this still true? You appear to be using a nicer interface.

>+    # Make sure to clean up _all_ package vars here. Yuck...
>+    $nametoexclude = $people{'changer'} ? $people{'changer'} : "";

$nametoexclude = $people{'changer'} || "";

>+    #die "FOO: $people{'changer'}\n";

Please remove debugging code :-)

>+
>+    return ProcessOneBug($id);

Can this sub not now be rolled into the above sub? It appears to do very
little.

Gerv
Attachment #81491 - Flags: review-
- 'people' isn't such a good name. Suggestions for a better one?

conspirators? :-)

Gerv
> Isn't
> while (MoreSQLData()) {
>    push (@list, FetchOneColumn());
>}
>the right way to do this?

Hey, I jsut moved code ;) But yes, I'll change that.

>>+    print "<br>" . scalar(@list) . " bugs found with possibly unsent mail.";

>Do you need "scalar" here?

I want to know the number of items in teh list - how else do I do so?

>>+    foreach my $id (@list) {
>>+        if (detaint_natural($id)) {
>>+            Bugmail::Send($id);
>>+        }

>Why are we detainting bug ids we get from the DB?

heh. Because processmail runs with dbi-taint mode enabled. I personally think
that that is stupid, but my attempts to remove it met with opposition, so...
However, that is no longer part of processmail, so I can and will fix that

> House style seems to be not to use function prototyping ($;$).

My intent is to change house style. Once we no longer 'require' all this stuff,
we'll be able to have perl test this for us at compile time.

> <remove data/nomail>

Theres a bug on moving this into the db. This bug will be as minimally invasive
as possible - I am not fixing this now. Lets just leave it here, and deal with
it later, either by removing it entirely, or by having a per user setting. We
can fix the 'vacation mode' bug at the same time.

>>+# This is a bit of a hack, basically keeping the old system()
>>+# cmd line interface. Should clean this up at some point

>Is this still true? You appear to be using a nicer interface.

Yes, this is still true. What should happen is that the activity log should be
read for additions and subtractions. Then we can call ProcessOneBug, passing
only the bugid in. The 'people' hash is just the command lines made nicer, since
I figured that I could at least get rid of @ARGLIST. (We may pass the changer
in, to make things simpler. We currently have an issue where if mail isn't sent,
the changer which does kick off the larger mail is considered to ahve changed
everything. We may want to fix this, although since that case should, in theory,
never happen, it may not be worth it)

>>+    return ProcessOneBug($id);

>Can this sub not now be rolled into the above sub? It appears to do very
>little.

On the contrary; this sub does all the work. Its the previous sub which has the
issues, since it has to clear all the pacakge vars. I want to keep the two
logically separate.

You didn't comment on my reference-vs-hash thing - do you know the TT rules for
that sort of thing?

Yes, people isn't a good name - see my comments. 'conspirators' is worse, though.
> >Do you need "scalar" here?
> 
> I want to know the number of items in teh list - how else do I do so?

print "<br>" . @list . " bugs found..." ?

> > House style seems to be not to use function prototyping ($;$).
> 
> My intent is to change house style. Once we no longer 'require' all this 
> stuff, we'll be able to have perl test this for us at compile time.

That's as may be. But currently house style is not to do it - and I, for one,
will oppose a change. Function prototypes in Perl gain nothing that
well-structured code and early parameter reading into sensibly named variables
doesn't get you, but just cause hassle and errors when they get out of sync with
reality

> You didn't comment on my reference-vs-hash thing - do you know the TT rules 
> for that sort of thing?

Er... I didn't quite understand the situation.

> Yes, people isn't a good name - see my comments. 'conspirators' is worse, 
> though.

I was only joking :-)

Gerv
I just wanted to note that making it a .pm will be a good thing. If for nothing
else to save me tearing my hair out trying to figure out why process_bug.cgi's
output was repeating itself. 

process_bug.cgi does not handle processmail having the wrong path to perl very
well, and goes nuts. The regexp given in the docs:

perl -pi -e 's@#!/usr/bonsaitools/bin/perl@#!/usr/bin/perl@' *cgi *pl Bug.pm
	    

doesn't quite work against processmail.
don't forget the voters in that hash...
No, voters aren't covered there. Hmm. That probably means that the pref to send
mail when you're added/removed from voting probably doesn't work. I'll have to
test that at some point, but ts a separate bug.
Actually, changing your votes _never_ sends mail, at all, so that option doesn't
make sense. Its only manually put into the hash so that thers a valid array in
there when we iterae over the types of people.
Attached patch v3 (obsolete) — Splinter Review
OK, here comes v3. Again, remember to |mv processmail Bugmail.pm| first.
Attachment #81491 - Attachment is obsolete: true
The image_map changes in dependency-graph.html.tmpl are unrelated to this bug.

> - let's not use StudlyCaps for CGI parameters
There is a rescanallBugmail param. Maybe it should be rescanall_bugmail.

> [% mail = SendBugmail(id, people) %]
In the long run, we shouldn't rely on a template calling this function (IMO).
See also the SyncAllPendingShadowChanges discussion. But for now, it's probably
fine.

I'm going to apply this now to my installation, and I will try to test it a bit.
But if this is going to be checked in for 2.16, then it should go in asap, so
that we have more time to catch any regressions it may cause. If there are any
bugs introduced with this patch, we certainly won't find them by just looking at
the patch.
Template->process() failed twice.
First error: [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5
18:19:22 2002] process_bug.cgi: undef error - [Sun May 5
18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002]
process_bug.cgi: undef error - [Sun May 5 18:19:22 2002]
process_bug.cgi: Attempted to send tainted string 'SELECT
assigned_to,bug_file_loc,bug_severity,bug_status,bug_type,cclist_accessible,component,everconfirmed,groupset,keywords,op_sys,priority,product,qa_contact,rep_platform,reporter,reporter_accessible,resolution,short_desc,status_whiteboard,target_milestone,version,votes,
lastdiffed, now() FROM bugs WHERE bug_id = 1' to the database at globals.pl line
251. 
Second error: [Sun May 5 18:19:22 2002] process_bug.cgi: undef error - [Sun May
5 18:19:22 2002] process_bug.cgi: undef error - [Sun May
5 18:19:22 2002] process_bug.cgi: undef error - [Sun May 5 18:19:22 2002]
process_bug.cgi: undef error - [Sun May 5 18:19:22 2002]
process_bug.cgi: Attempted to send tainted string 'select (bit & 64504) != 0
from groups where name = 'tweakparams'' to the database at
globals.pl line 251. 

Do I need to do some additional untainting for my "bug_type" stuff, or is this a
bug in the patch? This happened when I tried to comment on one of my test bugs
where I am reporter, assignee and qacontact at the same time.
Oops - yeah, ignore that template.

Why shouldn't we rely on the template to send mail? Thats effectivly what the
old code did, and it meant that people could see what was taking all this time.
(To do so here, I probably need to add some [% FLUSH %] calls, though)

Those errors look really unrelated to my patch. Do you get them without it?
No, the errors went away when I backed out your patch. But this doesn't
necessarily mean that it's a problem with your patch. I have an additional
column bug_type in the bugs table:
bug_type | enum('DEFECT','ENHANCEMENT','FEATURE','TASK','QUESTION','PATCH')
|      |     | DEFECT |
So if this patch adds some additional taint stuff I may be required to make some
additional changes. But maybe someone else can test the patch first? Is it
already live on landfill?

Maybe you are right with the email sending stuff. But we should document
somewhere all the places where we rely on templates to call some backend code.
Without looking at your patch, I don't know. Work out which value is tainted,
and detaint it. There shouldn't be any extra requirements from my patch in that
regard, though.

In any event, this doesn't happen without your addon patch, so...
Ok, I finally found out that $id is tainted here, in ProcessOneBug:

    SendSQL("SELECT " . join(',', @::log_columns) . ", lastdiffed, now() " .
            "FROM bugs WHERE bug_id = $id");

Note that your patch removes a call to detaint_natural($id) before calling
ProcessOneBug:

-    foreach my $id (@list) {
-        if (detaint_natural($id)) {
-            ProcessOneBug($id);
-        }

Placing a 

    detaint_natural($id); 

at the beginning of ProcessOneBug seems to fix it for me.
But maybe it belongs in the Send(...) sub above?

I wonder why this error is not showing up in your version...
But I removed this from sanity check - is that where you are having your errors?
(Actually, now that I think of it, that detainting code probably does need to stay)

The difference is that now the number we pass to the bug stuff needs to be
untainted.

If you remove the two lines in Bugmail.pm changing $db->{'taint'}, does that fix
it for you?

Otherwise there probably is somewhere which needs to be detainted. However, this
works for me, so it could be a perl 5.005 thing.

Can you get a stacktrace, so that I can see where it is failing? That second
error lookes real strange.
> I removed this from sanity check - is that where you are having your errors?
No, I'm getting the error when commenting on a bug, as I said in comment 27.

> remove the two lines in Bugmail.pm changing $db->{'taint'}
No, commenting out these two lines does not fix it.

> Otherwise there probably is somewhere which needs to be detainted. 
Well, it obviously doesn't work for me, and I don't understand the problem. If
ProcessOneBug expects $id to be untainted, and now we are passing it through the
template processing stuff which eventually calls Bugmail::Send(...), then I
think it's quite understandable that it needs to be detainted before passing it
to ProcessOneBug, isn't it? Well, I don't understand this stuff at all, but it
sounds plausible to me.

> stacktrace
I sent a page with confess output in private mail.

> That second error lookes real strange.
I think it's a followup from the first error. It goes away when the first error
is fixed. When I turned on sqllog, I was at first very confused where the last
two sql queries came from, since they did not seem to match with the code. I
think they were generated by an attempt to generate a nice error template or
something, or even for the footer.
The thing I got in the email was just a "you didn't select any bugs to change"
think. Just copy and paste the stack to the bug, here.
OK, impact on this one is a little too high to do this close to a release.

The Windows folks will have to wait for 2.17.1.  However, 2.17.1 WILL be
released within a month of 2.16, and I plan on this being one of the first
things checked in after 2.16 goes out.  Maybe we'll even put this patch by
itself back on top of 2.16 and make it a 2.16.1.

We'll RC the 2.16 branch tonight.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
I think the issue is just related to my old perl 5.005_02 installation, so it
shouldn't hold up any progress on this bug, as long as you require a newer perl
or put in the one-line workaround.

Great news about the RC1 :-)
If this works with 5.005_03, then yes, we'll just up the perl version.

OTOH, are we going to require 5.6 after 2.16, in the end?
Dave, I think it's a good idea to relnote the Windows problems and do the 2.16.1
pretty soon after 2.16 is out so that even Windows people can get on with the
upgrade. There are lots of reasons to switch from 2.14 to 2.16, and not everyone
is willing to use a developer version (2.17.1).
If 2.16.1 include this patch and the Bug 84875 this will remove 90% of the 
modifications needed to work under windows...
oups, Bug 84876 not 84875...
That's a good idea... 84876 has had a lot of work done on it, but was waived off
on 2.16 because those involved (Gerv) were busy getting 2.16 RC1 out the door,
and there were some details we were still trying to get nailed down on the patch
for 84876.

I said in 84876 (comment #72) that I'd backport the patch for 84876 if anyone
wanted it, but releasing a 2.16.1 with it in there makes even more sense.
Why not making 2.16.1 the first "windows ready" version ??
Becasue it would be 2.17.1, not 2.16.1 - ie a development release, not a stable one.
This is really a semantic issue (but then again, as one of my software
engineering profs says, life is nothing but semantics), but why can't it be
released as 2.16.1?

We released a 2.14.1 when we had security "issues"; if we can e very controlled
in what we might put into 2.16.1 (maybe fixes for this and 84876, or a subset of
84876), and relnote the fact that it's a release that makes "Bugzilla do sane
things on Windows, no need to upgrade if you're on a real OS," I shouldn't think
it would be a development release.

It's not meant to be used in development situations, or conversely: we *should*
have a release that "does the sane thing on Windows," and it *should* be a
stable version... we should give the impression, via the version number, that
the patches applied to 2.16 are stable enough to be used in a (Windows)
production environment, assuming they indeed are.
I don't think there will be "Windows support" until we have a developer who runs
Bugzilla under Windows.
Depending of what you call a "devloper" I can do the job !!

I am running 2 system with 2.14 and a test 2.16 and already submite 
some "win32" patch

OK, the taint this is 5.00503, too - I reproduced this on landfill. The test
case is really odd. It appears that having _anything_ which is tainted in the
vars hash will cause this:

use Template;

my $template = Template->new({
  INCLUDE_PATH => '.',
}) || die Template->error();


my $vars = { 'is_tainted' => sub {
    return not eval { my $foo = join('',@_), kill 0; 1; };
  }
}; 
   
$vars->{'foo'} = $ENV{'PATH'};
$vars->{'var'} = 1;

print "Perl: " . is_tainted($vars->{'var'}) . "\n";

with a template of:

[% is_tainted(var) %]

Comment out the $vars{'foo'} line, and everything is ok.

This looks really odd. The only think I can think of is a builtin function in
perl5.005 somewhere which converts the hash to an array and then back, but thats
a bit far fetched.

Anyone got some ideas? (The test is in ~bbaetz/tainttest/ on landfill, btw)
My only idea would be to look at the compiled code of the template, and then try
to find out which function is responsible for this. But I have absolutely no
experience in this area, so this may be a stupid idea...
I thikn its doing this earlier. Actually, this is probably why TT only lets you
use hash refs - stuff is put into an array. I'lll have to think about this.
Just a status report:

I pulled a fresh CVS copy on 9th May at about 10:00 GMT, patched it with
attachment 82411 [details] [diff] [review] (bbaetz's patch v3) and have been running it in production use
ever since. No problems with mailing at all. Platform W2k, IIS5, ActiveState
Perl 5.6.1. 

So the patch seems to be working quite nicely apart from the taint problems
you've been discussing.
What does it take to get it on the trunk, to get broader testing? I've been
running with this patch for some time, too, and I haven't heard of any problems,
besides this one-liner to workaround the taint problem. (But that doesn't mean
too much; most of our users wouldn't even complain if Bugzilla was down for a
day or two ;-)
Mainly me fionding time to tidy this up...
I'm starting to get convinced this might make it to 2.16.  Andreas, you're using
Solaris, right?  Hearing from a few more folks (Linux and Windows) that they've
used this successfully without problems would be convincing. :)
Yes, my installation is on a heavily patched solaris 2.6 Generic_105181-16 on a 
sun4u sparc SUNW,Ultra-2 with with currently only one processor (though there
was a time when it had two) running perl 5.005_02 and mysql Ver 9.36 Distrib
3.22.27, for sun-solaris2.6 (sparc), using sendmail. :-)
Attached patch v4 (obsolete) — Splinter Review
OK, this adds the detaint_natural in, and changes the interface so that we pass
a hash ref, not a hash, into Bugmail::Send.

Any other comments?
Attachment #82311 - Attachment is obsolete: true
Note that the wait time is actually a real pain - I almost double submitted a
review on a bug with lots of people ccd, because it was taking so long before I
got any feedback. If this patch goes in, and I add a [% FLUSH %] call both
before and after sending mail, then we get better perceived performace. 
Blocks: 154627
Blocks: 154638
Comment on attachment 84192 [details] [diff] [review]
v4

I have plans on reviewing this soon - if you're kind enough to give me a
version that applies cleanly on the trunk tip. :-)
Attachment #84192 - Flags: review-
Assigned -> me, per a conversation on IRC to this effect (doing this so it shows
up in my bug lists).
Assignee: bbaetz → preed
Status: ASSIGNED → NEW
Priority: P2 → P1
Attached patch v5 (obsolete) — Splinter Review
This version applies cleanly to the trunk as of right now (which isn't saying
much).
Attachment #84192 - Attachment is obsolete: true
jth/other Windows folks: Could you please apply this new version of the patch
and let me know how it goes?

I'm driving this patch through to checkin for bbaetz; he's busy right now.

I'd like to get this checked in before it rots too much since the trunk not as
static as, say, the 2.16 branch, so your help in this area would be appreciated.
Status: NEW → ASSIGNED
As I mentioned on irc, I've changed my mind - the template should not have the
job of calling code to process mail messages.

Also, the .pm should be in the newly-created Bugzilla/ directory
Ok. My Win32 testing platform has almost recovered from the crash in late July,
so I plan on testing this next week. Two weeks away from computers has been
good, although not for Bugzilla ;-) 
Comment on attachment 95210 [details] [diff] [review]
v5

Ok, I've quickly tested this. There are a couple of problems I encountered:

The patch doesn't apply cleanly (the first hunk of processmail fails)

Whenever Bugmail::Send is about to get called, the following error pops up:

undef error - Undefined subroutine &Bugmail::Send called at globals.pl line
1666. 


These occur on both Windows and Linux. Unfortunately I need to run now, so I
can't debug this further now. :-( If you can create another version of the
patch, I'll certainly test more.
Attachment #95210 - Flags: review-
Jouni: Thanks for testing that; I'll get the patch updated to HEAD, address the
issues bbaetz raised in comment 62 and test it on Linux.
Whiteboard: [needed for Win32bz]
This would be a massive perf gain, as we then wouldn't need to reload all the
modules every time we call processmail (Siunce they'd be in the same process)
Blocks: bz-perf
In case anyone cares, I'm gonna try to get this fixed for 2.17.1, since it's a
blocker and all.

Everyone together now: cross your fingers.

Blocks: 178153
*** Bug 125688 has been marked as a duplicate of this bug. ***
Attached patch v6 (obsolete) — Splinter Review
Ok, let's get this fun bus back on the road.

This is an updated version of v5, which I ended up patching manually because
stuff has changed so much. Some things to note:

-- I removed a call to processmail in CGI.pl, having to do with voting; I don't
know if I have all the correct variables necessary.

-- In the new BugMail.pm, calling Taint on the $::db handle caused things to
blow chunks (complain about tainting); since it was noted that that stuff's a
work in progress, I commented it out and all works, but don't really know if
that's the right thing(tm) to do.

-- The patch is basically v5, but it's been updated to head, which involved a
lot of general changes; for instance, changes to Bugzilla/Template.pm, which
didn't exist when v5 was written.

-- I modified Bugzilla::BugMail::Send to return a hashref instead of an array,
which made the code a bit easier to understand.

You can play around with a current HEAD installation + this patch at
http://landfill.mozilla.org/preed/bugzilla/

I did some simple things like file bugs and make changes, and I got the emails,
but please try to break it.

Any Win32ers on CC: the faster you can test this, the quicker we can knock out
another [needed for Win32bz] bug.
Attachment #95210 - Attachment is obsolete: true
Attachment #110202 - Flags: review?(reviewers)
Comment on attachment 110202 [details] [diff] [review]
v6

New patch coming shortly.
Attachment #110202 - Flags: review?(reviewers)
Attached patch v7 (obsolete) — Splinter Review
This patch is v6 + the fix for bug 183388, which was checked in today.

It was merged manually, since it's such a small patch.

I'll be cc'ing the patch author, asking him to verify the merge.
Attachment #110202 - Attachment is obsolete: true
Cc'ing the patch author for bug 183388; can you please verify my manual merge of
the fix for that bug on v7 of this patch?

Thanks!
Comment on attachment 110301 [details] [diff] [review]
v7

Since reviewers@ didn't listen to me, we'll do this this spammish way...
Attachment #110301 - Flags: review?(bbaetz)
Attachment #110301 - Flags: review?(gerv)
Attachment #110301 - Flags: review?(bugreport)
Attachment #110301 - Flags: review?(jake)
Initial note...

In sanitycheck.cgi.....

1) s/use Bugmail/use Bugzilla::BugMail/
2) (nit) If this is being USEd, should it be at the top?   Could it be REQUIREd
instead?



re: comment 74:

Ignore that 'use' statement behind the curtain. ;-)

It's from bbaetz's first version of the patch, and both issues are moot, since I
removed the line entirely, and it works fine.

I also had to change a line further down to be:
Bugzilla::BugMail::Send($id);

Since I'm sure there will be another version or two of this patch, look for it
in v8.
re: comment 75, where's v8? :-)
re comment 76: Why it's in your local grocer's freezer, of course! :-)

v8 is still in my local tree; the differences between v7 and v8 are two lines
that don't really affect the core patch (just the cleanup features in
sanitycheck.cgi), so I didn't see a reason to go through the process of
canceling all the r= requests, posting a new page, and then re-requesting
everything.

I did this, in part, because I'm sure there will be a v8 that includes more than
just those two-line changes after those first r='s are done.
Comment on attachment 110301 [details] [diff] [review]
v7

> Index: CGI.pl
> ===================================================================
> +        $vars->{'people'} = { 'changer' => $who };

As bbaetz commented in comment 8, people isn't such a good name.
"mailaddressees"? "addressees"? "recipients"?

> Index: post_bug.cgi
> ===================================================================
> -if (defined $::FORM{'qa_contact'}) {
> -    push (@ARGLIST, "-forceqacontact", DBID_to_name($::FORM{'qa_contact'}));
> +if (defined $::FORM{qa_contact}) {
> +    $vars->{'people'}->{'qa'} = DBID_to_name($::FORM{'qa_contact'})

Nit: please retain the quotes around the hash key.

>  push (@{$vars->{'sentmail'}}, { type => 'created',
>                                  id => $id,
> -                                mail => $mailresults
> +                                #mail => $mailresults

This should be removed rather than commented out - same for the two other
occurrences in this file.

> Index: process_bug.cgi
> ===================================================================
> -        # Save off the removedCcString so it can be fed to processmail
> -        $removedCcString = join (",", @removed);
>  
>          # If any changes were found, record it in the activity log
>          if (scalar(@removed) || scalar(@added)) {
> @@ -1396,6 +1394,7 @@
>              LogActivityEntry($id,"cc",$removed,$added,$whoid,$timestamp);
>              $bug_changed = 1;
>          }
> +        @ccRemoved = @removed;
>      }

This does do a copy rather than just create another reference to the same
array, right?

> @@ -1709,11 +1691,7 @@
>          CheckFormFieldDefined(\%::FORM,'comment');
>          SendSQL("INSERT INTO duplicates VALUES ($duplicate, $::FORM{'id'})");
>          
> -        $vars->{'mail'} = "";
> -        open(PMAIL, "-|") or exec('./processmail', $duplicate, $::COOKIE{'Bugzilla_login'});
> -        $vars->{'mail'} .= $_ while <PMAIL>;
> -        close(PMAIL);
> -        
> +        $vars->{'people'} = { 'changer' => $::COOKIE{'Bugzilla_login'} }; 

The change appears always to be current user - do we need to specify this
everywher explicitly?

> Index: sanitycheck.cgi
> ===================================================================
> -    print("Run <code>processmail rescanall</code> to fix this<p>\n");
> +    print qq{<a href="sanitycheck.cgi?rescanallBugmail=1">Click here to send these mails</a>.<p>\n};

Please let's not have "Click here". Just "Send these mails" would be a fine
link. Also, wouldn't this rerun all the sanity checks that you had just run
_again_? On a big installation, that would be rather irritating. Perhaps we
could make rescanallbugmail an exclusive action, and exit() after doing it?

> Index: Bugzilla/Template.pm
> ===================================================================
> RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v
> retrieving revision 1.1
> diff -u -r1.1 Template.pm
> --- Bugzilla/Template.pm	20 Dec 2002 07:21:30 -0000	1.1
> +++ Bugzilla/Template.pm	29 Dec 2002 11:55:59 -0000
> @@ -198,6 +198,18 @@
>              # UserInGroup - you probably want to cache this
>              'UserInGroup' => \&::UserInGroup,
>  
> +            # SendBugmail- sends mail about a bug, using Bugzilla::BugMail.pm
> +            'SendBugmail' => sub {
> +                my ($id, $people) = (@_);

I'd prefer to call this sub either SendMail (or SendBugMail or SendMailForBug
or BugMailSend), if you are using StudlyCaps. My preference would probably be
SendMailForBug...

> +                use Bugzilla::BugMail;
> +
> +                # perl5.005 will taint all template vars if any of them 
> +                # are tainted
> +                detaint_natural($id) || die "id for SendBugmail isn't a number";

Well, we don't support 5.005 any more, so do we need to do this?

> Index: Bugzilla/BugMail.pm 
> ===================================================================
> +# This code is really ugly. It was a commandline interface, then it was moved

Nit: full stop.

> +# This is run when we load the package
> +if (open(FID, "<data/nomail")) {
> +    while (<FID>) {
> +        $nomail{trim($_)} = 1;
> +    }
> +    close FID;
> +}

Should we continue to support data/nomail? The Right Way to do this these days
is create an account for the user and disable their mail in preferences.

> +sub ProcessOneBug($) {
> +    my ($id) = (@_);
> +
> +    # Set Taint mode for the SQL
> +    #$::db->{Taint} = 1;
> +    # ^^^ Taint mode is still a work in progress...

What are the implications of doing this? It looks quite scary... how does it
interact with normal taint mode?

> Index: template/en/default/bug/process/bugmail.html.tmpl
> ===================================================================
> +[%# INTERFACE:
> +  # people: hash. People involved in this change. Hash has up to five elements:
> +  #   changer: string. The login name of the user who made the change.
> +  #   For bug changes:
> +  #   owner: string. The login name of the bug assignee.
> +  #   reporter: string. The login name of the bug reporter.
> +  #   qacontact: string. The login name of the bug's QA contact. Optional.
> +  #   cc: list of strings. The login names of those on the CC list.

Where do watchers fit into this scheme?

> +<center>
> +  If you wish to tweak the kinds of mail Bugzilla sends you, you can
> +  <a href="userprefs.cgi?tab=email">change your preferences</a>.
> +</center>

Can we get this just once per batch of mails sent, rather than for every mail,
as now?

Gerv
Attachment #110301 - Flags: review?(gerv) → review-
> Should we continue to support data/nomail? The Right Way to do this these days
> is create an account for the user and disable their mail in preferences.

I saw we should still support data/nomail.
1) If somebody is using it and it suddenly is unsupported with no reasonable
   replacement, that's a bad thing.
2) There is no easy way for an admin to disable all of a users bugmail prefs, so
   that makes this not be a viable solution.

> What are the implications of doing this? It looks quite scary... how does it
> interact with normal taint mode?

Turning on $::db->{Taint} was the eventual goal when we stated taint mode. 
Basically what it does is die() if you try to send a tainted string to and cause
data coming from the database to be tained.  We accomplished the first half of
this with the is_tainted function now in Bugzilla::Util, but we still "trust"
everything the database gives us.

Of course this is commented out, so it doesn't really effect anything, but I am
curious why it's there.
the taint stuff is there because we all agreed to turn it off, since taintout as
silly - we trust our db. I have patches to use the new DBI TaintIn attribute,
which tests stuff gong in, similar to what SendSQL does.

Theres a separate bug on moving data/nomail into the db, which should be done
for all sorts of other reasons anyway.
Comment on attachment 110301 [details] [diff] [review]
v7

Canceling the r= requests.

I've got a new version of the patch that addresses most of your issues, Gerv
and fixes the stuff Joel brought up in sanitycheck.cgi.

But now bbaetz and I are hashing out whether TT should send the mail or not...
which is an issue we've both been waffling on.

Look for a new patch within the week.
Attachment #110301 - Flags: review?(jake)
Attachment #110301 - Flags: review?(bugreport)
Attachment #110301 - Flags: review?(bbaetz)
Blocks: 84876
Just an update: I haven't dropped this on the floor, but my workstation died
this weekend, so I'm currently rushing around town for a new motherboard + CPU;
I don't think I lost any data, though (hard drives are intact; mobo crapped
itself, though); I will post the new patch once I have my workstation back up
and running.
Attached patch v8 (obsolete) — Splinter Review
v8: try this one on for size.

This patch addresses the issues raised in the r=, plus has been updated to
HEAD.
Attachment #110301 - Attachment is obsolete: true
Attachment #112381 - Flags: review?(gerv)
Installation of the patch went fine, but I can't test it yet.

The reason I can't test it isn't related to this bug, so I'll ask on the 
webtools mailing list. After that is solved, I should be able to test the patch 
and I'll report my findings here.
OK, v8 gives me this when submitting a comment to a bug :

_____quote_____

Bugzilla has suffered an internal error. Please save this page and send it to 
jean_seb@hybride.com with details of what you were doing at the time this 
message appeared. 

URL: http://pcrd.hybride.com/bugzilla/process_bug.cgi

undef error - Template::Exception at D:/perl/lib/CGI/Carp.pm line 301.  

_____end_quote_____

The update went through fine though. (the comment is added to the bug)

Is there a way of backing out the patch (I have the cygwin version of patch, 
which is supposed to be the same as the unix version), so that I can check if 
the error message goes away then?
Well, yee haw.

My test installation is at http://landfill.mozilla.org/preed/bugzilla/; can you
try and repro it there?

Oh, and what version did you patch? HEAD? It should apply cleanly to earlier
versions, but may not be valid with earlier versions.

backing out the patch: cvs up -C. This assumes you have cvs 1.11 or better (on
both the client and server... don't know if cvs-mirror.m.o is 1.11 or better,
though).
patch -R
Thanks for the commands, I'll try to see what happens. JP, (can I call you 
JP? :-) I'll check your test installation. Is it win32 as well? You probably 
tested it already, which would mean that I have a problem on my installation...

More info :
I also ran a sanity check, which told me that there were about 40 bugs with 
unsent mail. When clicking the link to send it, I got a page saying this :

_____quote_____

Bugzilla Sanity Check  
    
OK, now attempting to send unsent mail 

25 bugs found with possibly unsent mail.

_____end_quote_____

without a footer, which I interpreted to mean that a perl error occured. 
Checking my apache error log, I found this :

[Fri Jan 24 16:00:59 2003] [error] [client 192.8.200.204] Undefined subroutine 
&Bugzilla::BugMail::Send called at D:/htdocs/bugzilla/sanitycheck.cgi line 
164., referer: http://pcrd.hybride.com/bugzilla/sanitycheck.cgi


BTW, the link is still called "Click here to send these mails", which I think 
Gerv objected to in comment #78...
Oh, wait, did I have to have any of the previous patches installed? Or did I 
have to |mv processmail BugMail.pm| first? Damn, I feel stupid :-(
My installation isn't Win32. But that's ok, since we need help making it work on
Win32, so the more testers with that platform, the better.

I'll change the name of the link; look for it in v9 (note to Gerv: ignore that
in v8, please).

You shouldn't have had to do any mv's or install any of the other patches...
although, make sure Bugzilla/BugMail.pm is there; if it's not, the patch may be
horked.
Yeah, BugMail.pm wasn't there, bugmail.html.tmpl either.

So now, I've created those files manually from the contents of the patch file, 
because even when I created empty files and did patch < [patch_file], nothing 
would be put into them.

After changing BugMail.pm to use Net::SMTP, it WORKS! :-)

And for sanitycheck.cgi, it still gave me the same error, so I added this :

<------- [line 161 of sanitycheck.cgi] -------->
    print "<br>" . @list . " bugs found with possibly unsent mail.<br>";

+    use Bugzilla::BugMail;

    foreach my $id (@list) {
        Bugzilla::BugMail::Send($id);
    }
<---------------------------------------------->

and that works. (I got 25 bugmails about old bugs... :-) But the page telling 
me that 25 bugmails have been sent still didn't have a footer, is that normal? 
No error in apache's log...

So I can confirm that the patch works. Now all we need is Bug #84876, and most 
(easily 90%) of the Windows installation headache will be gone!
Attached patch v9 (obsolete) — Splinter Review
v9: fixes the problems raised by  Jean-Sebastien in comments 91 and 88.

*This* one *might* actually go in! :-)
Attachment #112381 - Attachment is obsolete: true
Attachment #112381 - Flags: review?(gerv)
Comment on attachment 112564 [details] [diff] [review]
v9

Let's start by trying to get an r= from Gerv... :-)
Attachment #112564 - Flags: review?(gerv)
Jean-Sebastien:

Can you do this for me: back out v8 locally, and try applying v9, and see if the
only thing you have to change is using Net::SMTP, and let me know if *that* works.

Thanks!
Oh, one last spam (sorry): this patch should work correctly this time in terms
of creating the new files.

Be sure you use the correct -p value, even if it's -p0.
Yep, v9 works great. No errors or anything. Great work!
Comment on attachment 112564 [details] [diff] [review]
v9

This is generally pretty good (with a few nits below). But before I give it
review+, I need to see the differences between processmail and (Bug)Mail.pm.
These are a bit hard to see in diff - any chance you could quickly summarise
the changes you made to make processmail into a module?


> Index: CGI.pl
> ===================================================================
> +        $vars->{'bugmailrcpts'} = { 'changer' => $who };

Ick. Can we call it "recipients", please? :-)

> Index: attachment.cgi
> ===================================================================
> -  $mailresults .= $_ while <PMAIL>;
> -  close(PMAIL);
> - 
>    # Define the variables and functions that will be passed to the UI template.
> +  $vars->{'bugmailrcpts'} =  { 'changer' => $::COOKIE{'Bugzilla_login'} };
> @@ -791,21 +779,10 @@
>    # Define the variables and functions that will be passed to the UI template.
> +  $vars->{'bugmailrcpts'} = { 'changer' => DBID_to_name($::userid) };

Why do you use $::COOKIE{'Bugzilla_login'} in one place, and
DBID_to_name($::userid) in the other?

> Index: post_bug.cgi
> ===================================================================
> -push (@ARGLIST, "-forceowner", DBID_to_name($::FORM{assigned_to}));
> +# Tell the user all about it
> +$vars->{'bugmailrcpts'} = {
> +                     'cc' => \@cc,
> +                     'owner' => DBID_to_name($::FORM{'assigned_to'}),
> +                     'reporter' => DBID_to_name($::userid),
> +                     'changer' => $::COOKIE{'Bugzilla_login'}

Same question here. The reporter is the changer, so you should make that clear
in the code.

>  foreach my $i (@all_deps) {
> -    my $mail = "";
> -    open(PMAIL, "-|") or exec('./processmail', $i, $::COOKIE{'Bugzilla_login'});
> -    $mail .= $_ while <PMAIL>;
> -    close(PMAIL);
> +#    my $mail = "";
> +#    open(PMAIL, "-|") or exec('./processmail', $i, $::COOKIE{'Bugzilla_login'});
> +#    $mail .= $_ while <PMAIL>;
> +#    close(PMAIL);

Please remove this instead of commenting it out.

> Index: Bugzilla/BugMail.pm
> ===================================================================
> +# This is run when we load the package
> +if (open(FID, "<data/nomail")) {
> +    while (<FID>) {
> +        $nomail{trim($_)} = 1;
> +    }
> +    close FID;
> +}

While you are there, please give this filehandle a better name :-)

> +# args: bug_id, and an optional hash ref which may have keys for:
> +# changer, owner, qa, reporter, cc
> +# Which contain values of people to be forced to their respective
> +# roles.

Slightly dodgy English here :-)

> +    # Since any email recipients must be rederived if the user has not
> +    # been rederived since the most recent group change, figure out when that
> +    # is once and determine the need to rederive users using the same DB 
> +    # access that gets the user's email address each time a person is 
> +    # processed.

This comment is seriously cryptic. But if you didn't write it, I won't make you
fix it.

> +    my $resid = 
> +
> +    SendSQL("SELECT bugs_activity.bug_id, bugs.short_desc, fielddefs.name, " .
> +            "       removed, added " .

Not sure about this formatting...

> Index: Bugzilla/Template.pm
> ===================================================================
> +            # SendBugmail- sends mail about a bug, using Bugzilla::BugMail.pm
> +            'SendBugmail' => sub {
> +                my ($id, $bugmailrcpts) = (@_);
> +                require Bugzilla::BugMail;
> +                Bugzilla::BugMail::Send($id, $bugmailrcpts);

(Same renaming thing.) Also, please have a consistent capitalisation - is it
Bugmail (the function) or BugMail (the package)? 

Are you certain there's no chance of the BugMail package ever being used to
send non-bug-mail? If there is, you should call it Bugzilla::Mail instead.

> Index: template/en/default/bug/process/results.html.tmpl
> ===================================================================
> +  # 
> +  # bugmailtcpts: hash; BugMail recipient params. Optional.

Typo. But the name's changing anyway, right? ;-)

>        <h2>[% title.$type %]</h2>
> -      [% mail %]
> +      [% PROCESS "bug/process/bugmail.html.tmpl" %]

Does this template not need a bug_id passed to it?

Gerv
Attachment #112564 - Flags: review?(gerv) → review-
> Ick. Can we call it "recipients", please? :-)

Recipients is just as bad as "people," IMO. Recipients of what? Granted,
Bugzilla only send email now, but... I want to be more clear on what they're
recipients *of*.

I'd be willing to change it to bugmailrecips if you don't like rcpts... I was in
an SMTP-y mood that day...

> Are you certain there's no chance of the BugMail package ever being used to
> send non-bug-mail? If there is, you should call it Bugzilla::Mail instead.

Yes. Bug 84876 will introduce a Bugzilla::ProcessMail package, which is what
that will be.

> Does this template not need a bug_id passed to it?

The way that works is it uses the bug number in id; I'm assuming that's set
somewhere else in the template, but maybe that's a recipe for disaster. On the
other hand, I don't know where I'd pull that from (Template::GetBugID() or some
such?), so maybe 'id' is guaranteed for us?

> Not sure about this formatting...

It's to keep the SELECT clause clear from the FROM clause... and pretty much
lifted from processmail; I won't defend it, but I understand it, and see no
reason to change it.

> Why do you use $::COOKIE{'Bugzilla_login'} in one place, and
> DBID_to_name($::userid) in the other?

Old style, I suspect; I changed it, and will test.

> Same question here. The reporter is the changer, so you should make that clear
> in the code.

Same answer here... :-)

re: comments: I didn't write either of those, the first one ("Dodgy English")
would be good to ask bbaetz what he meant, assuming he wrote it; the 2nd one,
same thing, although I'm planning to rewrite a lot of processmail once things
settle down, so...

I'll post a summary of the diffs in a moment.
Summary of changes between processmail and BugMail.pm:

-- Some autoload stuff/package variable initialization; also removed some global
vars we don't need anymore because they're in the package now.

-- BugMail::Send() replaces the command line parsing stuff, and drives the whole
process (aside: I understand that "dodgy" comment now, and will try to clean it
up...) BugMail::Send() now also calculates the $::last_changed value; this used
to be done whenever processmail was run, but it's done here now since it's a
package.

-- Removed the generation of the HTML-formatted output of processmail, since
that's done by TT now.

-- Includes the fix for bug 183388

-- One *really* oddly formatted SQL statement was fixed.

-- Finally, remove all the commandline processing stuff.

These changes are in order of what diff output, so you should be able to go
through them and see what I'm referencing.
Attached patch v10 (obsolete) — Splinter Review
Changes between v9 and v10:

-- s/bugmailrcpts/bugmailrecips/g

-- $::last_changed has been moved to Bugzilla::BugMail; no reason to polute the
global namespace.

-- The disparity between $::COOKIE{Bugzilla_login} and DBID_to_name was
changed/tested

-- Changed one of the comments to (hopefully) make it clearer.
Attachment #112564 - Attachment is obsolete: true
Attachment #112865 - Flags: review?(gerv)
Attachment #112865 - Flags: review?(bugreport)
v10 tested on my side, no problem.
> Recipients is just as bad as "people," IMO. Recipients of what? Granted,
> Bugzilla only send email now, but... I want to be more clear on what they're
> recipients *of*.

A notification. Not necessarily mail, as you (or someone) pointed out in bug
84876. It could be an instant message, or whatever. This is a variable being
passed to a template, which will do some sort of notification, but the CGI
should neither know nor care what. So recipients is obvious as to its content,
anyone knows what it means now, but it could mean something additionally in the
future.

But it's also aesthetically horrible, as are most word contractions. And
"bugmailrecips" makes me think of recipes. :-) So, if the above doesn't convince
you or your other reviewer, how about "mailrecipients"?

> > Does this template not need a bug_id passed to it?
> 
> The way that works is it uses the bug number in id; I'm assuming that's set
> somewhere else in the template, but maybe that's a recipe for disaster. On the
> other hand, I don't know where I'd pull that from (Template::GetBugID() or 
> some such?), so maybe 'id' is guaranteed for us?

Even if it's set elsewhere, you should set it explicitly, to avoid the sort of
confusion which confused me. If the interface is an ID, we should make it clear
that we are passing it across the interface - otherwise someone might change
that template, for some reason, to call it "bug_id", and mail will break because
it can't find "id" any more.

(Actually, in general, calling any variable "id" in Bugzilla is bad, as we have
so many different sorts of id. But that's another thing.)

Gerv
I'm not convinced, but mailrecipients is fine; I'll s/// that.

I'll also fix that id issue you mentioned; you're absolutely right.

Joel: assume I fix these, and r= the rest of v10, if you have time, please. Same
with with you, Gerv.

I'll carry the r='s forward to v11 if you don't find anything else.
i like 'victims'
but failing that, bugmailees should suffice. oh it isn't mail.
bugnotifiess bugalertees :-)
bugcontacts <- best bet
Comment on attachment 112865 [details] [diff] [review]
v10

> Index: post_bug.cgi
> ===================================================================
> -push (@ARGLIST, "-forceowner", DBID_to_name($::FORM{assigned_to}));
> +# Tell the user all about it
> +$vars->{'bugmailrecips'} = {
> +                     'cc' => \@cc,
> +                     'owner' => DBID_to_name($::FORM{'assigned_to'}),
> +                     'reporter' => $::COOKIE{'Bugzilla_login'},
> +                     'changer' => $::COOKIE{'Bugzilla_login'}
> +                    } 

> Index: process_bug.cgi
> ===================================================================
> +    $vars->{'bugmailrecips'} = { 'cc' => \@ccRemoved,
> +                          'owner' => $origOwner,
> +                          'qa' => $origQaContact,
> +                          'changer' => $::COOKIE{'Bugzilla_login'}
> +                        };

> +            $vars->{'bugmailrcpts'} = { 'changer' => 
> +             $::COOKIE{'Bugzilla_login'} };

Nit: inconsistent formatting between instances.

> +            # SendBugMail- sends mail about a bug, using Bugzilla::BugMail.pm

Tiny nit: missing space.

> -      [% mailresults %]
> +      [% PROCESS "bug/process/bugmail.html.tmpl" id = bugid %]

Just to be completely clear (I think you are doing this anyway) - I think you
should a) always pass the ID across this interface explicitly, and b) rename
the variable inside bugmail.html.tmpl to bugid or bug_id, because calling
things "id" is bad.

> Index: template/en/default/bug/process/bugmail.html.tmpl
> ===================================================================
> +[%# INTERFACE:
> +  # bugmailrecips: hash. People involved in this change. Hash has up to five 
> +  # elements:
> +  #   changer: string. The login name of the user who made the change.
> +  #   For bug changes:
> +  #   owner: string. The login name of the bug assignee.
> +  #   reporter: string. The login name of the bug reporter.
> +  #   qacontact: string. The login name of the bug's QA contact. Optional.
> +  #   cc: list of strings. The login names of those on the CC list.
> +  #%]

Shouldn't id/bugid/bug_id be in this list?


Fix these nits, and r=gerv. Let's get this in :-)

Gerv
Attachment #112865 - Flags: review?(gerv) → review+
Attached patch v11 (obsolete) — Splinter Review
v11; fixes Gerv's nits:

-- Changes the %vars variable name to mailrecipients
-- always pass the bugid to the template via a variable called mailing_bugid
(bugid and bug_id were *really* common).
Attachment #112865 - Attachment is obsolete: true
Attachment #112865 - Flags: review?(bugreport)
Attachment #113937 - Flags: review?(jouni)
Comment on attachment 113937 [details] [diff] [review]
v11

Mostly really small nits, but there's one potentially harmful case issue in
BugMail.pm which I'd like to see fixed before checkin.

------------------
>+# Tell the user all about it

This comment sucks. If it means nothing, remove it. If it means something,
change it :-) (I know, it was there before, but the context of the comment has
changed so much that it's a good idea to do something about this, too)

>-    push (@ARGLIST, "-forceqacontact", DBID_to_name($::FORM{'qa_contact'}));
>+    $vars->{'mailrecipients'}->{'qa'} = DBID_to_name($::FORM{'qa_contact'})

Just for the sake of consistency (and for the future), add a semicolon there
:-)

>-    
>-    my $removedCcString = "";
>+   
>+    my @ccRemoved = (); 

If you're changing those empty lines, kill the spaces totally. Now you've
removed one out of four.

>+            $vars->{'mailrecipients'} = { 'changer' => 
>+             $::COOKIE{'Bugzilla_login'} };

This wrapping doesn't look good.

>+if (exists $::FORM{'rescanallBugmail'}) {

Über-nit: If you're using "BugMail" (with the capital M) everywhere else, you
should probably use it in this form field as well.

>+    print "<br>" . @list . " bugs found with possibly unsent mail.<br>";

Make this

Status("@list bugs found with possibly unsent mail.");

or something similar. Two issues: 1) avoid the .-based catenation if you're
using "" anyway (or use ''), 2) use Status instead of prints with <br>s to
preserve formatting.

>+    @{$force{'CClist'}} = (exists $recipients->{'cc'} && 
>+     scalar($recipients->{'cc'}) > 0) ? map(trim($_), 
>+     @{$recipients->{'cc'}}) : ();
>+    @{$force{'Owner'}} = $recipients->{'owner'} ? 
>+     (trim($recipients->{'owner'})) : ();
>+    @{$force{'QAcontact'}} = $recipients->{'qacontact'} ? 
>+     (trim($recipients->{'qacontact'})) : ();
>+    @{$force{'Reporter'}} = $recipients->{'reporter'} ? 
>+     (trim($recipients->{'reporter'})) : ();

This is _so_ ugly. Any chance of a cuter wrapping? The scope of the ? :
operator is not very clear now.

But, an important issue (this is the one I'm marking review- for): In
BugMail.pm, you have the $force{'QAContact'} key written as both 'QAContact'
and 'QAcontact'. Please review the role names once more for case issues. How
about making keys of the force hash all lowercase, just to avoid conflicts in
the future?

>+      [% PROCESS "bug/process/bugmail.html.tmpl" mailing_bugid= bugid %]

Nit: a space before the assignment operator.

>+  # mailrecipients: hash. People involved in this change. Hash has up to five 
>+  # elements:
>+  #   changer: string. The login name of the user who made the change.
>+  #   For bug changes:
>+  #   owner: string. The login name of the bug assignee.
>+  #   reporter: string. The login name of the bug reporter.
>+  #   qacontact: string. The login name of the bug's QA contact. Optional.
>+  #   cc: list of strings. The login names of those on the CC list.

First, indent the mailrecipients comment like

# mailrecipients: hash. Yadda yadda yadda
#		  This is the second line
  ---------------
Empty space here :-)

And then, "For bug changes:" isn't too clear. How about a description like:
"The following keys have values if the respective fields have changed."?



Also, a diff of tip-processmail and the BugMail.pm in your patch reveals the
following:

>-    SendSQL("SELECT userid, (refreshed_when > " . SqlQuote($::last_changed) . ") " .
>-            "FROM profiles WHERE login_name = " . SqlQuote($person));
>+    SendSQL("SELECT userid, (refreshed_when > " . SqlQuote($last_changed) .
>+            ") " .  "FROM profiles WHERE login_name = " . SqlQuote($person));

Which is fine with me, but the second line now contains an unnecessary
catenation; you could just write it as ") FROM profiles...".
Attachment #113937 - Flags: review?(jouni) → review-
Attached patch v12 (obsolete) — Splinter Review
This should hopefully make *everyone* happy. :-)

Jouni: I fixed that one instance of "QAContact," since it looks like it was
missed in the fix for bug 183388, and I can't find an instance of "QAContact"
anywhere else (they were all changed to "QAcontact"); admittedly, this isn't
very clear at all, but fixing the ambiguity is *another* bug.

I also left some of the ternary operators as is... yes, it's ugly, but... that
*too* is another bug. ;-)
Attachment #113937 - Attachment is obsolete: true
Comment on attachment 113944 [details] [diff] [review]
v12

>I also left some of the ternary operators as is... yes, it's ugly, but... that
>*too* is another bug. ;-)

I disagree, but don't really care at this point.

>+# Email everyone regarding the change 

This comment still sucks, because it's not a "change" we're talking about here.
This is post_bug.cgi, so why not make it "Email everyone about the new bug"?

r=jouni, but please fix that comment before checkin. 


Good job, by the way :-)
Attachment #113944 - Flags: review+
> so why not make it "Email everyone about the new bug"?

Done.

Also, the reason it's good is because bbaetz wrote it. ;-)

Justdave?
Flags: approval?
hooray!!
Flags: approval? → approval+
Attached patch v12 - finalizedSplinter Review
Same thing as v12, just updated to HEAD.

r='s carried forward from gerv and jth.
Attachment #113944 - Attachment is obsolete: true
Checked in:

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.199; previous revision: 1.198
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.37; previous revision: 1.36
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.78; previous revision: 1.77
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.175; previous revision: 1.174
done
Removing processmail;
/cvsroot/mozilla/webtools/bugzilla/processmail,v  <--  processmail
new revision: delete; previous revision: 1.94
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.63; previous revision: 1.62
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
initial revision: 1.1
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/attachment/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/created.html.tmpl,v
 <--  created.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/attachment/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/updated.html.tmpl,v
 <--  updated.html.tmpl
new revision: 1.8; previous revision: 1.7
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/bugmail.html.tmpl,v
done
Checking in template/en/default/bug/process/bugmail.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/bugmail.html.tmpl,v
 <--  bugmail.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/bug/process/results.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/process/results.html.tmpl,v
 <--  results.html.tmpl
new revision: 1.4; previous revision: 1.3
done

Thanks for everyone's help on this!
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Blocks: 136156
Blocks: 31314
i have installed the patch, i thought that all open Sendmail commands will
replaced to one in bugmail.pm.

Just search for open sendmail and found serveral open sendmail commands.
cgi.pl
globals.pl
importxml.pl
move.pl
whieneatnews.pl
token.pm 
and so on.

Am i wrong with my meaning? Sorry for my english, it's not my default.
thanks
Re comment 114: You're thinking of bug 84876.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.