While I concur with hangon's endorsement of Mime::Lite, I primarily wanted to offer you a couple of notes:
First, you go through the trouble of ripping through your argument list, but in some cases, you're just passing the same argument list to another function and returning. I'd suggest you change the start of your sub to something like this:
sub send_mail { $CONFIG{mail_format} = 2; if($smtp_path =~ /\w+/){ $CONFIG{mailprogram} = $smtp_path; $CONFIG{mailuser} = $smtp_user; $CONFIG{mailpass} = $smtp_pass; return &send_mail_NT(@_); }
Some may argue against this suggestion: If you change the argument list to your sub, then you'll have to make changes to call the send_mail_NT function anyway. But since you already have another send_mail function, it's advisable to use compatible argument lists. (Note: I also removed the creation of the $VAR{res} slot which you use only to hold a return value.)
Second, you're constructing a hash from your arguments, but you don't need any hash features in your sub, so it's just a lot of extra typing. I'd suggest you just split your argument list into named variables and use them throughout your sub:
my ($sndrEml, $sndrNme, $rcvrEml, $rcvrNme, $subject, $message, $c +crcEml, $ccrcNme, $attPths) = @_;
Third, you perform identical operations on several values. You might want to either (a) create a sub to handle the task, or (b) do them all in a single statement. For example, your statements:
all perform the same cleanup tasks. If you need to change your cleanup code, you have three places to change it in. You can create a sub like this:$sndrNme =~ s/\</\</ig; $rcvrNme =~ s/\</\</ig; $subject =~ s/\</\</ig; $ccrcNme =~ s/\</\</ig; $sndrNme =~ s/\<.*?>//ig; $rcvrNme =~ s/\<.*?>//ig; $subject =~ s/\<.*?>//ig; $ccrcNme =~ s/\<.*?>//ig;
sub _clean_value { local $_ = shift; s/\</\</ig; s/\<.*?>//ig; $_; }
allowing you to clean up your values like this:
$sndrNme = _clean_value($sndrNme); $rcvrNme = _clean_value($sndrNme); $subject = _clean_value($subject); $ccrcNme = _clean_value($ccrcNme);
Alternately, if you don't think it's worth a subroutine, you could perform all the operations at one go by taking advantage map, like so:
($sndrNme, $rcvrNme, $subject, $ccrcNme) = map { s/\</\</ig; s/\<.*?>//ig; $_; } ($sndrNme, $rcvrNme, $subject, $ccrcNme);
Finally, your sub serves two purposes, and it's better to break down your program such that each sub serves a single, well-defined purpose. The first purpose your sub has is to decide between methods used to send an EMail. The second is to actually send the EMail. So I'd suggest you chop up your sub into something like this1:
sub send_email { # Set up values that *all* the EMail senders will need to know $CONFIG{mail_format} = 2; # Which method should we use to send the EMail? if($smtp_path =~ /\w+/){ # specific values used by send_mail_NT $CONFIG{mailprogram} = $smtp_path; $CONFIG{mailuser} = $smtp_user; $CONFIG{mailpass} = $smtp_pass; return send_mail_NT(@_); } else { # Our default method if no better method was found $CONFIG{mailprogram} = $send_mail_path; return send_mail_Default(@_); } } sub send_email_Default { my ($sndrEml, $sndrNme, $rcvrEml, $rcvrNme, $subject, $message, $c +crcEml, $ccrcNme, $attPths) = @_; my $Host = gethostname; my $CRLF = "\n"; my $message .= qq~$CRLF$CRLF~ .= qq~IP Address: $ENV{REMOTE_ADDR}$CRLF~ . qq~Host Node: $Host$CRLF~ . qq~User Agent: $ENV{HTTP_USER_AGENT}~; $message =~ s/\r//ig; $message =~ s/\n{3,300}/$CRLF$CRLF/ig; $attPths =~ s/\s+//ig; my $HTMLmess = $message; my $TEXTmess = $message; $TEXTmess =~ s/\"/"/g; $TEXTmess =~ s/\</\</ig; my $files = ""; my $hasAtts = 1; ($sndrNme, $rcvrNme, $subject, $ccrcNme) = map { s/\</\</ig; s/\<.*?>//ig; $_; } ($sndrNme, $rcvrNme, $subject, $ccrcNme); my $bound1 = qq~----=_NextPart_P_115Dream~; # printable my $bound2 = qq~----=_NextPart_A_SubHB~; # attachments my $bound3 = qq~----=_NextPart_E_SlowTrain~; # embded my $fm = _fixNme($sndrEml, $sndrNme); my $to = _fixNme($sndrEml, $sndrNme); my $cc = _fixNme($sndrEml, $sndrNme); if(open(M, qq~|$CONFIG{mailprogram} -t ~)){ print M qq~From: $fm$CRLF~; print M qq~To: $to$CRLF~; print M qq~CC: $cc$CRLF~ if $ccrcEml; print M qq~Subject: $subject$CRLF~; print M qq~MIME-Version: 1.0$CRLF~; print M qq~$VAR{fileHeaders}~; print M qq~Content-Type: text/plain;$VAR{CRLF}~; print M qq~Content-Transfer-Encoding: 7bit$VAR{CRLF}$VAR{CRLF} +~; print M qq~$VAR{TEXTmess}$VAR{CRLF}$VAR{CRLF}$VAR{files}~; close M; return 1; } else { return; } } sub _fixNme { my ($Eml, $Nme) = @_; return $Nme =~ /\w+/ ? qq~$Eml ($Nme)~ : $Eml; }
Notes:
...roboticus
Update: Node seems to be a bit long, so I added readmore tag to shorten it a bit; also fixed a footnote reference.
In reply to Re: How to add attachments to an email using cgi/perl
by roboticus
in thread How to add attachments to an email using cgi/perl
by Anonymous Monk
| For: | Use: | ||
| & | & | ||
| < | < | ||
| > | > | ||
| [ | [ | ||
| ] | ] |