From: Tomas Doran Date: Fri, 9 Jan 2009 01:55:55 +0000 (+0000) Subject: Merge flash in session and finalize before sending response patches X-Git-Tag: v0.19_01~2 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=2842d93853af927a543214ccbe17d659f5ecd181;hp=d423b91c24216b68620c52d0aea176366971760d;p=catagits%2FCatalyst-Plugin-Session.git Merge flash in session and finalize before sending response patches --- diff --git a/Changes b/Changes index 9b62b98..d23ead0 100644 --- a/Changes +++ b/Changes @@ -1,7 +1,14 @@ Revision history for Perl extension Catalyst::Plugin::Session -0.20 XXX +0.19_01 2009-01-09 + - Use shipit to package the dist - Switch to Module::install + - Flash data is now stored inside the session (key "__flash") to avoid + duplicate entry errors caused by simultaneous select/insert/delete of + flash rows when using DBI as a Store. (Sergio Salvi) + - Fix session finalization order that caused HTTP responses to be sent + before the session is actually finalized and stored in its Store. + (Sergio Salvi) 0.19 2007-10-08 diff --git a/Makefile.PL b/Makefile.PL index a8dfe6f..073ce16 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -16,6 +16,7 @@ requires 'Digest'; requires 'File::Spec'; requires 'File::Temp'; requires 'Object::Signature'; +requires 'MRO::Compat'; # an indirect dep. needs a certain version. requires 'Tie::RefHash' => '1.34'; diff --git a/lib/Catalyst/Plugin/Session.pm b/lib/Catalyst/Plugin/Session.pm index 1e9dd99..e9c2a13 100644 --- a/lib/Catalyst/Plugin/Session.pm +++ b/lib/Catalyst/Plugin/Session.pm @@ -13,7 +13,7 @@ use overload (); use Object::Signature (); use Carp; -our $VERSION = '0.20'; +our $VERSION = '0.19_01'; my @session_data_accessors; # used in delete_session BEGIN { @@ -98,13 +98,15 @@ sub finalize_headers { return $c->NEXT::finalize_headers(@_); } -sub finalize { +sub finalize_body { my $c = shift; - my $ret = $c->NEXT::finalize(@_); - # then finish the rest + # We have to finalize our session *before* $c->engine->finalize_xxx is called, + # because we do not want to send the HTTP response before the session is stored/committed to + # the session database (or whatever Session::Store you use). $c->finalize_session; - return $ret; + + return $c->NEXT::finalize_body(@_); } sub finalize_session { @@ -168,12 +170,15 @@ sub _save_flash { my $sid = $c->sessionid; + my $session_data = $c->_session; if (%$flash_data) { - $c->store_session_data( "flash:$sid", $flash_data ); + $session_data->{__flash} = $flash_data; } else { - $c->delete_session_data("flash:$sid"); + delete $session_data->{__flash}; } + $c->_session($session_data); + $c->_save_session; } } @@ -238,8 +243,11 @@ sub _load_flash { $c->_tried_loading_flash_data(1); if ( my $sid = $c->sessionid ) { - if ( my $flash_data = $c->_flash - || $c->_flash( $c->get_session_data("flash:$sid") ) ) + + my $session_data = $c->session; + $c->_flash($session_data->{__flash}); + + if ( my $flash_data = $c->_flash ) { $c->_flash_key_hashes({ map { $_ => Object::Signature::signature( \$flash_data->{$_} ) } keys %$flash_data }); @@ -687,6 +695,8 @@ changed, call C and pass in the keys as arguments. This method is used to invalidate a session. It takes an optional parameter which will be saved in C if provided. +NOTE: This method will B delete your flash data. + =item session_delete_reason This accessor contains a string with the reason a session was deleted. Possible @@ -756,10 +766,10 @@ prepare time. This method is extended and will extend the expiry time before sending the response. -=item finalize +=item finalize_body -This method is extended and will call finalize_session after the other -finalizes run. Here we persist the session data if a session exists. +This method is extended and will call finalize_session before the other +finalize_body methods run. Here we persist the session data if a session exists. =item initialize_session_data @@ -1002,10 +1012,14 @@ Andy Grundman Christian Hansen -Yuval Kogman, C (current maintainer) +Yuval Kogman, C Sebastian Riedel +Tomas Doran (t0m) C (current maintainer) + +Sergio Salvi + And countless other contributers from #catalyst. Thanks guys! =head1 COPYRIGHT & LICENSE diff --git a/t/03_flash.t b/t/03_flash.t index 0ed2242..f58222c 100644 --- a/t/03_flash.t +++ b/t/03_flash.t @@ -3,9 +3,10 @@ use strict; use warnings; -use Test::More tests => 8; +use Test::More tests => 12; use Test::MockObject::Extends; use Test::Exception; +use Test::Deep; my $m; BEGIN { use_ok( $m = "Catalyst::Plugin::Session" ) } @@ -19,47 +20,56 @@ $c->mock( return $key =~ /expire/ ? time() + 1000 : $flash; }, ); +$c->mock("debug" => sub { 0 }); $c->mock("store_session_data" => sub { $flash = $_[2] }); $c->mock("delete_session_data" => sub { $flash = {} }); $c->set_always( _sessionid => "deadbeef" ); $c->set_always( config => { session => { expires => 1000 } } ); $c->set_always( stash => {} ); +is_deeply( $c->session, {}, "nothing in session" ); + is_deeply( $c->flash, {}, "nothing in flash" ); $c->flash->{foo} = "moose"; -$c->finalize; +$c->finalize_body; is_deeply( $c->flash, { foo => "moose" }, "one key in flash" ); +cmp_deeply( $c->session, { __updated => re('^\d+$'), __flash => $c->flash }, "session has __flash with flash data" ); + $c->flash(bar => "gorch"); is_deeply( $c->flash, { foo => "moose", bar => "gorch" }, "two keys in flash" ); -$c->finalize; +cmp_deeply( $c->session, { __updated => re('^\d+$'), __flash => $c->flash }, "session still has __flash with flash data" ); + +$c->finalize_body; is_deeply( $c->flash, { bar => "gorch" }, "one key in flash" ); -$c->finalize; +$c->finalize_body; $c->flash->{test} = 'clear_flash'; -$c->finalize; +$c->finalize_body; $c->clear_flash(); is_deeply( $c->flash, {}, "nothing in flash after clear_flash" ); -$c->finalize; +$c->finalize_body; is_deeply( $c->flash, {}, "nothing in flash after finalize after clear_flash" ); +cmp_deeply( $c->session, { __updated => re('^\d+$'), }, "session has empty __flash after clear_flash + finalize" ); + $c->flash->{bar} = "gorch"; $c->config->{session}{flash_to_stash} = 1; -$c->finalize; +$c->finalize_body; $c->prepare_action; is_deeply( $c->stash, { bar => "gorch" }, "flash copied to stash" ); diff --git a/t/06_finalize.t b/t/06_finalize.t deleted file mode 100644 index b1aac67..0000000 --- a/t/06_finalize.t +++ /dev/null @@ -1,41 +0,0 @@ -#!/usr/bin/perl - -use strict; -use warnings; - -use Test::More; - -BEGIN { - if ( eval { require Catalyst::Plugin::Session::State::Cookie } ) { - plan tests => 3; - } else { - plan skip_all => "Catalyst::Plugin::Session::State::Cookie required"; - } -} - -my $finalized = 0; - -{ - package TestPlugin; - BEGIN { $INC{"TestPlugin.pm"} = 1 } # nasty hack for 5.8.6 - - sub finalize_session { $finalized = 1 } - - sub finalize { die "already finalized_session()" if $finalized } - - # Structure inheritance so TestPlugin->finalize() is called *after* - # Catalyst::Plugin::Session->finalize() - package TestApp; - - use Catalyst qw/ - Session Session::Store::Dummy Session::State::Cookie +TestPlugin - /; - __PACKAGE__->setup; -} - -BEGIN { use_ok('Catalyst::Plugin::Session') } - -my $c = TestApp->new; -eval { $c->finalize }; -ok(!$@, "finalize_session() called after all other finalize() methods"); -ok($finalized, "finalize_session() called");