Skip Menu |
 

This queue is for tickets about the Mail-Message-Attachment-Stripper CPAN distribution.

Report information
The Basics
Id: 19100
Status: open
Priority: 0/
Queue: Mail-Message-Attachment-Stripper

People
Owner: Nobody in particular
Requestors: Mark [...] Overmeer.net
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



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>
Download (untitled) / with headers
text/plain 4.2k
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
Subject: Re: [rt.cpan.org #19100] Superfluous module
Date: Fri, 5 May 2006 17:12:27 +0100
To: Mark Overmeer via RT <bug-Mail-Message-Attachment-Stripper [...] rt.cpan.org>
From: Tony Bowden <tony [...] kasei.com>
Download (untitled) / with headers
text/plain 305b
On Fri, May 05, 2006 at 10:47:42AM -0400, Mark Overmeer via RT wrote: Show quoted text
> Can you explain to me why your module exists?
Because we needed this functionality and the functionality in Mail::Box either didn't exist at that time, was broken, or Simon (who wrote most of the module) didn't know about it. Tony
Subject: Re: [rt.cpan.org #19100] Superfluous module
Date: Fri, 5 May 2006 18:20:09 +0200
To: Tony Bowden via RT <bug-Mail-Message-Attachment-Stripper [...] rt.cpan.org>
From: Mark Overmeer <mark [...] overmeer.net>
Download (untitled) / with headers
text/plain 957b
* Tony Bowden via RT (bug-Mail-Message-Attachment-Stripper@rt.cpan.org) [060505 16:13]: Show quoted text
> On Fri, May 05, 2006 at 10:47:42AM -0400, Mark Overmeer via RT wrote:
> > Can you explain to me why your module exists?
> > Because we needed this functionality and the functionality in Mail::Box > either didn't exist at that time, was broken, or Simon (who wrote most > of the module) didn't know about it.
Probably the latter, reading his papers... Before more people start bothering me with problems related to ::Stripper, could we plan to get rit of it? Or maybe to document how people can re-work their code to use plain Mail::Box again? -- MarkOv ------------------------------------------------------------------------ drs Mark A.C.J. Overmeer MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
Subject: Re: [rt.cpan.org #19100] Superfluous module
Date: Mon, 08 May 2006 12:40:47 +0100
To: bug-Mail-Message-Attachment-Stripper [...] rt.cpan.org
From: Tony Bowden <tony [...] tmtm.com>
Download (untitled) / with headers
text/plain 328b
Mark Overmeer via RT wrote: Show quoted text
> Before more people start bothering me with problems related to ::Stripper, > could we plan to get rit of it? Or maybe to document how people can re-work > their code to use plain Mail::Box again?
It's not really a priority for me, but I'll certainly listen to any suggestions you have... Tony
Subject: Re: [rt.cpan.org #19100] Superfluous module
Date: Mon, 8 May 2006 14:28:58 +0200
To: "tony [...] tmtm.com via RT" <bug-Mail-Message-Attachment-Stripper [...] rt.cpan.org>
From: Mark Overmeer <solutions [...] overmeer.net>
Download (untitled) / with headers
text/plain 946b
* tony@tmtm.com via RT (bug-Mail-Message-Attachment-Stripper@rt.cpan.org) [060508 11:41]: Show quoted text
> > <URL: http://rt.cpan.org/Ticket/Display.html?id=19100 > > > Mark Overmeer via RT wrote:
> > Before more people start bothering me with problems related to ::Stripper, > > could we plan to get rit of it? Or maybe to document how people can re-work > > their code to use plain Mail::Box again?
> > It's not really a priority for me, but I'll certainly listen to any > suggestions you have...
Maybe you can deprecate it, and include the simple example of how to do the same in plain Mail::Box in the manual-page. Would be nice. -- MarkOv ------------------------------------------------------------------------ drs Mark A.C.J. Overmeer MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.