fix noisy logs with header set aftger finalize

diff --git a/Changes b/Changes
index d812bc6..6ef60df 100644 (file)
--- a/Changes
+++ b/Changes
@@ -9,6 +9,9 @@
     now when you forward to an action and that action throws an exception, $c->state
     is set to 0, instead of the value of the exeption (this is to be as indicated by
     the documentation). (cventers++ for reported bug and test case).
+  - Changed the code that detects if you try to set HTTP headers after headers are
+    finalized to not warn if you are just requested the response header state.  Tweaked
+    this error message a bit to help people understand it.
 
 5.90104 - 2016-04-04
   - Merged pull request #131, fix for noisy debug logs when used type constraints

diff --git a/lib/Catalyst/Response.pm b/lib/Catalyst/Response.pm
index e87ba61..94ee3f3 100644 (file)
--- a/lib/Catalyst/Response.pm
+++ b/lib/Catalyst/Response.pm
@@ -103,12 +103,24 @@ has _context => (
   clearer => '_clear_context',
 );
 
-before [qw(status headers content_encoding content_length content_type header)] => sub {
+before [qw(status headers content_encoding content_length content_type )] => sub {
   my $self = shift;
 
-  $self->_context->log->warn( 
+  $self->_context->log->warn(
     "Useless setting a header value after finalize_headers and the response callback has been called." .
-    " Not what you want." )
+    " Since we don't support tail headers this will not work as you might expect." )
+      if ( $self->_context && $self->finalized_headers && !$self->_has_response_cb && @_ );
+};
+
+# This has to be different since the first param to ->header is the header name and presumably
+# you should be able to request the header even after finalization, just not try to change it.
+before 'header' => sub {
+  my $self = shift;
+  my $header = shift;
+
+  $self->_context->log->warn(
+    "Useless setting a header value after finalize_headers and the response callback has been called." .
+    " Since we don't support tail headers this will not work as you might expect." )
       if ( $self->_context && $self->finalized_headers && !$self->_has_response_cb && @_ );
 };
 

diff --git a/t/useless_set_headers.t b/t/useless_set_headers.t
new file mode 100644 (file)
index 0000000..0db6fda
--- /dev/null
+++ b/t/useless_set_headers.t
@@ -0,0 +1,67 @@
+use warnings;
+use strict;
+use Test::More;
+use HTTP::Request::Common;
+
+{
+  package TestAppStats::Log;
+  $INC{'TestAppStats/Log.pm'} = __FILE__;
+
+  use base qw/Catalyst::Log/;
+
+  my @warn;
+
+  sub my_warnings { $warn[0] };
+  sub warn { shift; push(@warn, @_) }
+ 
+  package MyApp::Controller::Root;
+  $INC{'MyApp/Controller/Root.pm'} = __FILE__;
+
+  use base 'Catalyst::Controller';
+
+  sub get_header_ok :Local {
+    my ($self, $c) = @_;
+    $c->res->body('get_header_ok');
+  }
+
+  sub set_header_nok :Local {
+    my ($self, $c) = @_;
+    $c->res->body('set_header_nok');
+  }
+
+  package MyApp;
+  $INC{'MyApp.pm'} = __FILE__;
+
+  use Catalyst;
+  use Moose;
+
+  sub debug { 1 }
+
+  __PACKAGE__->log(TestAppStats::Log->new);
+
+  after 'finalize' => sub {
+    my ($c) = @_;
+    if($c->res->body eq 'set_header_nok') {
+      Test::More::ok 1, 'got this far'; # got this far
+      $c->res->header('REQUEST_METHOD', 'bad idea');
+    } elsif($c->res->body eq 'get_header_ok') {
+      Test::More::ok $c->res->header('x-catalyst'), 'Can query a header without causing trouble';
+    }
+  };
+
+  MyApp->setup;
+}
+
+use Catalyst::Test 'MyApp';
+
+ok request(GET '/root/get_header_ok'), 'got good request for get_header_ok';
+ok !TestAppStats::Log::my_warnings, 'no warnings';
+ok request(GET '/root/set_header_nok'), 'got good request for set_header_nok';
+ok TestAppStats::Log::my_warnings, 'has a warning';
+like TestAppStats::Log::my_warnings, qr'Useless setting a header value after finalize_headers', 'got expected warnings';
+
+# We need to specify the number in order to be sure we are testing
+# it all correctly.  If you change the number of tests please keep
+# this up to date.  DO NOT REMOVE THIS!
+
+done_testing(7);