Skip Menu |
 

This queue is for tickets about the HTTP-Message CPAN distribution.

Report information
The Basics
Id: 79239
Status: resolved
Priority: 0/
Queue: HTTP-Message

People
Owner: Nobody in particular
Requestors: MARKOV [...] cpan.org
Cc: c.deignan [...] rsd.com
ether [...] cpan.org
AdminCc:

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



CC: c.deignan [...] rsd.com
Subject: leaving end boundary without \n
Hi Gisle, In HTTP::Message, when you parse multiparts, You use this: (around line 1000) $str =~ s/\r?\n--\Q$b\E--\r?\n.*//s; if ($str =~ s/(^|.*?\r?\n)--\Q$b\E\r?\n//s) { $self->{_parts} = [map HTTP::Message->parse($_). split(/\r?\n--\Q$b\E\r?\n/, $str)] } According to RFC2046 section 5.1.1 page 19: --gc0pJq0M:08jU534c0p-- NOTE TO IMPLEMENTORS: Boundary string comparisons must compare the boundary value with the beginning of each candidate line. An exact match of the entire candidate line is not required; it is sufficient that the boundary appear in its entirety following the CRLF. It is a bit hard to read, but I (and some implementation) read this as: there doesn't need to be a \n after the -- of the closing boundary. The regex is therefore to strict, requiring an \n - $str =~ s/\r?\n--\Q$b\E--\r?\n.*//s; + $str =~ s/\r?\n--\Q$b\E--.*//s; [[ side note: I think OO likes to see HTTP::Message->parse($_) to be rewritten als (ref $self)->parse($_) ]] Thanks in advance, MarkOv
Your sugged fix was applied as https://github.com/libwww-perl/http-message/commit/c1bac162b83094764ed4f8d8737c5367ec02305c

Regarding your side note, I think it should stay as HTTP::Message->parse($_).  The actual class of $self is likely to be HTTP::Request or HTTP::Response, but that's not the kind of stuff to expect in the parts.
Download (untitled) / with headers
text/plain 884b
On Sat Oct 20 18:04:34 2012, GAAS wrote: Show quoted text
> Your sugged fix was applied as > https://github.com/libwww-perl/http- > message/commit/c1bac162b83094764ed4f8d8737c5367ec02305c > > Regarding your side note, I think it should stay as HTTP::Message-
> >parse($_).
> The actual class of $self is likely to be HTTP::Request or > HTTP::Response, but > that's not the kind of stuff to expect in the parts.
Thanks for fixing. Also, I am really happy with the split-up of LWP you did last year. Thanks. On the latter: the difference shows itself when one of the extensions has additional needs for parse(). Your own extensions do not need that... however, other people (like me) may also extend your classes. You never know. There is no reason to force method lookup here. The usual way is to make parse() a combined class/instance method. In that case you can simply call $self->parse()
On Sat Oct 20 18:18:43 2012, MARKOV wrote:
Show quoted text
> On the latter: the difference shows itself when one of the extensions
> has additional needs for parse(). Your own extensions do not need
> that... however, other people (like me) may also extend your classes.
> You never know. There is no reason to force method lookup here.

That's right.  I was thinking about arguing for why I would not remove this hardcoding until somebody came up with a use case for it, but decided that it was easier to explain it by how it would have to be implemented; so I just did that https://github.com/libwww-perl/http-message/commit/5f0b18944eabcd2883e797818a84374138b40140.

The fact that ref($self)->new() wasn't the right solution here is an indication that it was a mistake to subclass HTTP::Request and HTTP::Response from HTTP::Message.  A more proper design would have been to use composition.  An HTTP::Request would then consist of a "request line" and a message and would then just use delegation to make it as easy to use.  After all I did not make HTTP::Message a subclass of HTTP::Headers and it's basically the same relationship there.  Oh, well.  Next time I'll design this better.  :-)

When this design came about HTTP::Message was abstract.  It was there purely for sharing the implementation between requests and responses.  The multipart support came later and suddenly it made sense to also instantiate pure messages.
CC: c.deignan [...] rsd.com
Subject: Re: [rt.cpan.org #79239] leaving end boundary without \n
Date: Sun, 21 Oct 2012 16:20:02 +0200
To: Gisle_Aas via RT <bug-HTTP-Message [...] rt.cpan.org>
From: Mark Overmeer <solutions [...] overmeer.net>
Download (untitled) / with headers
text/plain 1011b
* Gisle_Aas via RT (bug-HTTP-Message@rt.cpan.org) [121021 07:18]: Show quoted text
> The fact that ref($self)->new() wasn't the right solution here is an indication > that it was a mistake to subclass HTTP::Request and HTTP::Response from > HTTP::Message.
IMO, that is a valid design. To parties talk in messages, which are just slightly different... the HTTP::Message is the important class, not these tiny extensions. I do not see any benefit in _part_class. But... there no single truth in OO. For Mail::Box, I had to implement the same thing (different kinds of mail-folders, in this case). The multipart-part are in Mail::Message::Part which extends Mail::Message. That works very well. -- Regards, MarkOv ------------------------------------------------------------------------ Mark Overmeer MSc MARKOV Solutions Mark@Overmeer.net solutions@overmeer.net http://Mark.Overmeer.net http://solutions.overmeer.net
RT-Send-CC: c.deignan [...] rsd.com, solutions [...] overmeer.net
The changes that were already in git have been released in 6.07. Please verify!


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.