[Catalyst] p-static-simple line-ending patch

[A. Pagaltzis]

* Carl Franks <fireartist at gmail.com> [2006-02-24 18:10]:
>I tried setting "binmode STDOUT" - this also fixed the problem.

Ah, there you go. That is the correct fix. Either both the input
and the output need to be cooked, or both need to be raw.

Now I’m trying to figure out how to best incorporate this into
the Catalyst architecture. I turns out to be very tricky.
Everything would be much simpler if it was possible to find out
whether a filehandle is `binmode`d, but I can’t find any way to
check that. So how do you ensure that input and output
filehandles are always in the same state?

Do you `binmode STDOUT` in `C::P::Static::Simple::_serve_static`?
That seems unclean and may fail with an exotic engine.

Do you do it in `C::Engine::finalize_body`? That seems better,
but then you’ve set binmode on the input fh in one place and on
the output fh in another, and people may forget to do that for
the input fh they pass. And it still has the same problem with
failure on exotic engines.

I guess you move the binmode call for the input fh to
`C::Engine::finalize_body`. And if you put the binmode call for
the output fh in `C::Engine::prepare_write`, then exotic engines
have a better chance to do the right thing. But then
prepare_write needs to check whether `$c->res->body` is a
filehandle. That feels like a bad distribution of
responsibilities.

So at first I was going to put a new property in `C::Engine`
which gets set appropriately in `finalize_body` and can then be
checked from `prepare_write`. But then I got the fear over what
might happen when `prepare_write` had already been called,
because I don’t yet understand how the headers get output. So I
opted instead to make this a new `binmode` method in `C::Engine`.

So now `C::Engine` calls `binmode` on `$c->res->body` when it
finds that it is a filehandle, and on itself. Engines can
override the `binmode` method to do whatever works best for them.

And anyway, when `$c->res->body` is a filehandle, it seems like a
good idea to always send the Content-Length header; the
information is available by `stat`ing the filehandle anyway, so
why have the code to set these headers duplicated in all client
code? So I promoted `C::Response::body` from dumb accessor to one
which does a few extra things: it stats the filehandle for size
and mtime, and sets its headers appropriately.

Patch is attached. Can someone else give it a spin? Can one of
the devs comment on the architectural decisions?

Regards,
-- 
Aristotle Pagaltzis // <http://plasmasturm.org/>
-------------- next part --------------
diff -adrPu Catalyst-5.65.orig/lib/Catalyst/Engine.pm Catalyst-5.65/lib/Catalyst/Engine.pm
--- Catalyst-5.65.orig/lib/Catalyst/Engine.pm	2006-01-30 10:39:10.000000000 +0100
+++ Catalyst-5.65/lib/Catalyst/Engine.pm	2006-02-24 19:58:21.000000000 +0100
@@ -43,6 +43,8 @@
 sub finalize_body {
     my ( $self, $c ) = @_;
     if ( ref $c->response->body && $c->response->body->can('read') ) {
+        $self->binmode();
+        $c->response->body->binmode();
         while ( !$c->response->body->eof() ) {
             $c->response->body->read( my $buffer, $CHUNKSIZE );
             last unless $self->write( $c, $buffer );
@@ -470,6 +472,15 @@
 
 sub prepare_write { }
 
+=head2 $self->binmode($c, [$layer])
+
+=cut
+
+sub binmode {
+    my ( $self, $c, $layer ) = @_;
+    *STDOUT->binmode( defined $layer ? $layer : () );
+}
+
 =head2 $self->read($c, [$maxlength])
 
 =cut
diff -adrPu Catalyst-5.65.orig/lib/Catalyst/Plugin/Static/Simple.pm Catalyst-5.65/lib/Catalyst/Plugin/Static/Simple.pm
--- Catalyst-5.65.orig/lib/Catalyst/Plugin/Static/Simple.pm	2006-01-10 15:02:25.000000000 +0100
+++ Catalyst-5.65/lib/Catalyst/Plugin/Static/Simple.pm	2006-02-24 20:03:43.000000000 +0100
@@ -166,18 +166,16 @@
 sub _serve_static {
     my $c = shift;
     
-    my $path = $c->req->path;    
+    my $full_path = $c->_static_file;
     my $type = $c->_ext_to_type;
     
-    my $full_path = $c->_static_file;
-    my $stat = stat $full_path;
-
     $c->res->headers->content_type( $type );
-    $c->res->headers->content_length( $stat->size );
-    $c->res->headers->last_modified( $stat->mtime );
 
     if ( Catalyst->VERSION le '5.33' ) {
         # old File::Slurp method
+        my $stat = stat $full_path;
+        $c->res->headers->last_modified( $stat->mtime );
+        $c->res->headers->content_length( $stat->size );
         my $content = File::Slurp::read_file( $full_path );
         $c->res->body( $content );
     }
@@ -185,7 +183,6 @@
         # new method, pass an IO::File object to body
         my $fh = IO::File->new( $full_path, 'r' );
         if ( defined $fh ) {
-            binmode $fh;
             $c->res->body( $fh );
         }
         else {
diff -adrPu Catalyst-5.65.orig/lib/Catalyst/Response.pm Catalyst-5.65/lib/Catalyst/Response.pm
--- Catalyst-5.65.orig/lib/Catalyst/Response.pm	2006-01-10 15:02:25.000000000 +0100
+++ Catalyst-5.65/lib/Catalyst/Response.pm	2006-02-24 20:03:51.000000000 +0100
@@ -3,7 +3,7 @@
 use strict;
 use base 'Class::Accessor::Fast';
 
-__PACKAGE__->mk_accessors(qw/cookies body headers location status/);
+__PACKAGE__->mk_accessors(qw/cookies headers location status/);
 
 *output = \&body;
 
@@ -44,6 +44,20 @@
 
 Sets or returns the output (text or binary data).
 
+=cut
+
+sub {
+    return $_[0]->{ body } unless @_ > 1;
+    my ( $self, $body ) = @_;
+    if ( ref $body && $body->can('read') ) {
+        my ( $size, $mtime ) = ( $body->stat() )[ 7, 9 ];
+        $stat->headers->content_length( $size );
+        $stat->headers->last_modified( $mtime );
+    }
+    $self->{ body } = $body;
+};
+
+
 =head2 $res->content_encoding
 
 Shortcut for $res->headers->content_encoding.