| Subject: | Superfluous module |
| Date: | Fri, 5 May 2006 16:47:28 +0200 |
| To: | bug-Mail-Message-Attachment-Stripper [...] rt.cpan.org |
| From: | Mark Overmeer <mark [...] overmeer.net> |
Hi Tony,
I wasn't aware (anymore) of your module Mail::Message::Attachment::Stripper,
until I saw someone actually using it. IMO, the module does not add
anything to Mail::Box basic features, even breaking a few things.
I would like to comment on some of the parts of your code:
sub _detach_all {
$self->_handle_part($mm);
$mm->body(Mail::Message::Body->new(data => $self->{_body}));
This is against all principles of Mail::Box: bodies of existing modules
should not change ever, to avoid accidental modification. It should be:
my $cloned = $mm->body->clone;
But there is no use in cloning... at all
sub _handle_part {
# According to Mail::Message docs, this ternary is not required. However,
# $Mail_Message->parts calls $Mail_Message->deleted which is
# unimplemented
This comment really distrubes me: when patched in the main release, you should
remove any work-around in your own code.
foreach my $part ($mm->isMultipart ? $mm->parts : $mm) {
if ($self->_is_inline_text($part)) {
$self->_gather_body($part);
} elsif ($self->_should_recurse($part)) {
$self->_handle_part($part);
} else {
$self->_gather_att($part);
}
}
This whole function should read
foreach my $part ($mm->parts('RECURSE'))
{ $self->_gather_att($part);
}
sub _gather_body { # Gen 25:8
my ($self, $part) = @_;
push @{ $self->{_body} }, $part->decoded->lines;
Why not simply use the body object? You can even say @$body to get the lines...
There is no need for copying (again)...
sub _gather_att {
my ($self, $part) = @_;
# stringification is required for safety
push @{ $self->{_atts} },
{
content_type => $part->body->mimeType . "",
Wrong: the mimeType is an object on purpose, because content-type should
be compared case-insenstive AND ignoring x- indicators.
payload => $part->decoded . "",
See gather_body. You can also use $part->decoded->string
filename => $self->_filename_for($part),
};
sub _should_recurse {
my ($self, $part) = @_;
return 0 if lc($part->body->mimeType) eq "message/rfc822";
mimeType delivers an object which compares case-insens:
return 0 if $part->body->mimeType eq "message/rfc822";
There is also a return 0 if $part->isNested;
return 1 if $part->isMultipart;
return 0;
So, the whole function does:
sub _should_recurse { $_[1]->isMultipart }
sub _is_inline_text {
my ($self, $part) = @_;
if ($part->body->mimeType eq "text/plain") {
my $disp = $part->head->get("Content-Disposition");
return 1 if $disp && $disp =~ /inline/;
return 0 if $self->_filename_for($part);
return 1;
}
inline text has only some relation to user applications displaying the
content of the message. Nothing else. I cannot imagin other serious
applications, and certainly not what I read from this code. What you
may want to express is
my $body = $part->decoded;
if($body->isText)
{ my $filename = $body->dispositionFilename;
Simply using the filename is very dangerous: it could contain a path!!!!
sub _filename_for {
my ($self, $part) = @_;
my $disp = $part->head->get("Content-Disposition");
my $type = $part->head->get("Content-Type");
Much better is
my $disp = $part->body->disposition;
my $type = $part->body->mimeType;
return ($disp && $disp->attribute("filename"))
|| ($type && $type->attribute("name"))
|| "";
To conclude.... your module provides this interface:
my $stripper = Mail::Message::Attachment::Stripper->new($mail);
my Mail::Message $msg = $stripper->message;
foreach my $att ($stripper->attachments)
{ print $att->{filename};
print $att->{content_type};
print for @{$att->{payload}};
}
which can be much faster and safer be written in plain Mail::Box as
foreach my $part ($msg->parts('RECURSE'))
{ my $body = $part->decoded;
print $body->dispositionFilename || 'none';
print $body->mimeType;
print $body;
}
Can you explain to me why your module exists?
--
Regards,
MarkOv
------------------------------------------------------------------------
Mark Overmeer MSc MARKOV Solutions
Mark@Overmeer.net solutions@overmeer.net
http://Mark.Overmeer.net http://solutions.overmeer.net