Bug #66411 for Catalyst-Plugin-PageCache: [PATCH] change finalize for after 'dispatch'

Sun Mar 06 00:25:03 2011 yanick [...] babyl.dyndns.org - Ticket created

CC:	Yanick Champoux <yanick [...] babyl.dyndns.org>
Subject:	[PATCH] change finalize for after 'dispatch'
Date:	Sun, 6 Mar 2011 00:24:22 -0500
To:	bug-Catalyst-Plugin-PageCache [...] rt.cpan.org
From:	Yanick Champoux <yanick [...] babyl.dyndns.org>

C::P::PC doesn't seem to play well with Catalyst::Plugin::SubRequest. From what I can see, it's because only the main request, and not the subrequests, hit 'finalize' and get cached. To get around this problem, I've swapper the 'finalize' method for a Moosish 'after dispatch => sub {...'. The tests are still passing, and it seems to do the trick, but I'd recommend to carefully review my patch, as I don't know enough of C::P::PC's guts to know if what I did won't bite us back in some special scenario. --- lib/Catalyst/Plugin/PageCache.pm | 15 ++++++++------- 1 files changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/Catalyst/Plugin/PageCache.pm b/lib/Catalyst/Plugin/PageCache.pm index 0444b44..65a1357 100644 --- a/lib/Catalyst/Plugin/PageCache.pm +++ b/lib/Catalyst/Plugin/PageCache.pm @@ -5,6 +5,8 @@ use base qw/Class::Accessor::Fast/; use MRO::Compat; use Digest::SHA1 (); +use Moose; + our $VERSION = '0.31'; # Do we need to cache the current page? @@ -261,27 +263,27 @@ sub _set_page_cache_headers { unless $c->res->status && $c->res->status == 304; } -sub finalize { +after dispatch => sub { my $c = shift; # never cache POST requests - return $c->next::method(@_) if ( $c->req->method eq "POST" ); + return if ( $c->req->method eq "POST" ); my $pc_config = $c->config->{'Plugin::PageCache'}; my $hook_name = $pc_config->{cache_dispatch_hook} || $pc_config->{cache_hook}; my $hook = $hook_name ? $c->can($hook_name) : undef; - return $c->next::method(@_) if ( $hook && !$c->$hook() ); + return if ( $hook && !$c->$hook() ); return $c->next::method(@_) if ( $pc_config->{auto_check_user} && $c->can('user_exists') && $c->user_exists); - return $c->next::method(@_) if ( scalar @{ $c->error } ); + return if ( scalar @{ $c->error } ); # if we already served the current request from cache, we can skip the # rest of this method - return $c->next::method(@_) if ( $c->_page_cache_used ); + return if ( $c->_page_cache_used ); if (!$c->_cache_page && scalar @{ $pc_config->{auto_cache} }) @@ -311,8 +313,7 @@ sub finalize { $c->_page_cache_not_modified( $data ) if $data; } - return $c->next::method(@_); -} +}; sub _store_page_in_cache { -- 1.7.3.4

Sun Mar 06 00:40:32 2011 yanick [...] babyl.dyndns.org - Correspondence added

Subject:	Re: [rt.cpan.org #66411] AutoReply: [PATCH] change finalize for after 'dispatch'
Date:	Sun, 06 Mar 2011 00:39:18 -0500
To:	bug-Catalyst-Plugin-PageCache [...] rt.cpan.org
From:	Yanick Champoux <yanick [...] babyl.dyndns.org>

Just to say that I've also written a test case to test the behavior when SubRequest is issued. I'll clean it up, and attach it to the bug tomorrow. Cheers, `/anick

Sun Mar 06 11:07:01 2011 yanick+cpan [...] babyl.dyndns.org - Correspondence added

As promised, here is the test, attached.

Subject:

test.patch

diff --git a/t/16subreq.t b/t/16subreq.t new file mode 100644 index 0000000..1aa243d --- /dev/null +++ b/t/16subreq.t @@ -0,0 +1,22 @@ +#!perl + +use strict; +use warnings; + +use FindBin; +use lib "$FindBin::Bin/lib/SubReq"; +use Test::More tests => 4; +use File::Path; + +plan skip_all => "test requires Catalyst::Plugin::SubRequest" + unless eval "use Catalyst::Plugin::SubRequest; 1"; + +use Catalyst::Test 'SubReq'; + +# cache a page +ok( my $res = request('http://localhost/foo'), 'request ok' ); +is $res->content => 'foo1', 'first request'; + +ok( $res = request('http://localhost/foo'), 'request ok' ); +is $res->content => 'foo1', 'request cached'; + diff --git a/t/lib/SubReq/SubReq.pm b/t/lib/SubReq/SubReq.pm new file mode 100644 index 0000000..537a6c4 --- /dev/null +++ b/t/lib/SubReq/SubReq.pm @@ -0,0 +1,35 @@ +package SubReq; +use Moose; +use namespace::autoclean; + +use Catalyst::Runtime 5.80; + +use Catalyst qw/ + PageCache + Cache + SubRequest +/; + +extends 'Catalyst'; + +our $VERSION = '0.01'; +$VERSION = eval $VERSION; + +__PACKAGE__->config( + name => 'Foo', + # Disable deprecated behavior needed by old applications + disable_component_resolution_regex_fallback => 1, + 'Plugin::Cache' => { + backend => { + class => 'Cache::FileCache', + } }, + 'Plugin::PageCache' => { + set_http_headers => 1, + debug => 0, + disable_index => 0, + }, +); + +__PACKAGE__->setup(); + +1; diff --git a/t/lib/SubReq/SubReq/Controller/Root.pm b/t/lib/SubReq/SubReq/Controller/Root.pm new file mode 100644 index 0000000..0bb639f --- /dev/null +++ b/t/lib/SubReq/SubReq/Controller/Root.pm @@ -0,0 +1,32 @@ +package SubReq::Controller::Root; +use Moose; +use namespace::autoclean; + +BEGIN { extends 'Catalyst::Controller' } + +__PACKAGE__->config(namespace => ''); + +sub foo :Local { + my ( $self, $c ) = @_; + $c->response->body( "foo".$c->subreq('/bar') ); +} + +my $counter = 1; + +sub bar :Local { + my ( $self, $c ) = @_; + $c->cache_page(3600); + $c->response->body( $counter++ ); +} + +sub baz :Local { + my ( $self, $c ) = @_; + $c->cache_page(3600); + $c->response->body( "baz" ); +} + +sub end : ActionClass('RenderView') {} + +__PACKAGE__->meta->make_immutable; + +1;

Sun Mar 06 11:07:02 2011 The RT System itself - Status changed from 'new' to 'open'