Skip to content

Commit

Permalink
Moved the rest of session related attributes and methods to Core::App
Browse files Browse the repository at this point in the history 
'session' attribute was removed from Core::Context

Moved from Core::Context to Core::App:
* destroyed_session (attribute)
* _build_session (replaced the temp. writer)
* has_session
* destroy_session

Added the clearer 'clear_destroyed_session' to 'destroyed_session'
attribute and now it runs in app_cleanup.
(this is to make sure we don't keep a destroyed session for the
next request -- that was a fun debugging session)

Dispatcher now takes curr_session instead of curr_context as optional
parameter (in forward).

Some docs references to Context where changed to App.

This is a major step forward in the effort to remove Core::Context
(almost there)
  • Loading branch information
@mickeyn
mickeyn committed May 31, 2014
1 parent dce899b commit 9c3b21b
Show file tree
Hide file tree
Showing 13 changed files with 129 additions and 142 deletions.
2 changes: 1 addition & 1 deletion lib/Dancer2/Cookbook.pod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ for you.

When you're done with your session, you can destroy it:

context->destroy_session
app->destroy_session

This comment has been minimized.

Sign in to view

Copy link
@veryrusty

veryrusty Jun 2, 2014

Member

Wondering if app->destroy_session should just become destroy_session, a (non-global) DSL keyword?

=head2 Sessions and logging in

Expand Down
105 changes: 98 additions & 7 deletions lib/Dancer2/Core/App.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,8 +275,10 @@ has session => (
is => 'ro',
isa => InstanceOf['Dancer2::Core::Session'],
lazy => 1,
builder => '_build_session',
writer => 'set_session',
predicate => 'has_session',
clearer => 'clear_session',
predicate => '_has_session',
);

sub _build_response {
Expand All @@ -288,6 +290,94 @@ sub _build_response {
);
}

sub _build_session {
my ($self) = @_;
my $session;

# Find the session engine
my $engine = $self->engine('session');

# find the session cookie if any
if ( !$self->destroyed_session ) {
my $session_id;
my $session_cookie = $self->cookie( $engine->cookie_name );
if ( defined $session_cookie ) {
$session_id = $session_cookie->value;
}

# if we have a session cookie, try to retrieve the session
if ( defined $session_id ) {
eval { $session = $engine->retrieve( id => $session_id ) };
croak "Fail to retrieve session: $@"
if $@ && $@ !~ /Unable to retrieve session/;
}
}

# create the session if none retrieved
return $session ||= $engine->create();
}

=method has_session
Returns true if session engine has been defined and if either a session object
has been instantiated in the context or if a session cookie was found and not
subsequently invalidated.
=cut

sub has_session {
my ($self) = @_;

my $engine = $self->engine('session');

return $self->_has_session
|| ( $self->cookie( $engine->cookie_name )
&& !$self->has_destroyed_session );
}

=attr destroyed_session
We cache a destroyed session here; once this is set we must not attempt to
retrieve the session from the cookie in the request. If no new session is
created, this is set (with expiration) as a cookie to force the browser to
expire the cookie.
=cut

has destroyed_session => (
is => 'rw',
isa => InstanceOf ['Dancer2::Core::Session'],
predicate => 1,
clearer => 'clear_destroyed_session',
);

=method destroy_session
Destroys the current session and ensures any subsequent session is created
from scratch and not from the request session cookie
=cut

sub destroy_session {
my ($self) = @_;

# Find the session engine
my $engine = $self->engine('session');

# Expire session, set the expired cookie and destroy the session
# Setting the cookie ensures client gets an expired cookie unless
# a new session is created and supercedes it
my $session = $self->session;
$session->expires(-86400); # yesterday
$engine->destroy( id => $session->id );

# Clear session and invalidate session cookie in request
$self->destroyed_session($session);
$self->clear_session;

return;
}

sub setup_session {
my $self = shift;

Expand Down Expand Up @@ -421,7 +511,7 @@ sub _init_hooks {
Dancer2::Core::Hook->new(
name => 'core.app.after_request',
code => sub {
my $response = shift;
my $response = $self->response;

# make sure an engine is defined, if not, nothing to do
my $engine = $self->session_engine;
Expand All @@ -435,17 +525,17 @@ sub _init_hooks {
# update the session ID if needed, then set the session cookie
# in the response

if ( $self->context->has_session ) {
my $session = $self->context->session;
if ( $self->has_session ) {
my $session = $self->session;
$engine->flush( session => $session )
if $session->is_dirty;
$engine->set_cookie_header(
response => $response,
session => $session
);
}
elsif ( $self->context->has_destroyed_session ) {
my $session = $self->context->destroyed_session;
elsif ( $self->has_destroyed_session ) {
my $session = $self->destroyed_session;
$engine->set_cookie_header(
response => $response,
session => $session,
Expand Down Expand Up @@ -487,6 +577,7 @@ sub cleanup {
my $self = shift;
$self->clear_request;
$self->clear_response;
$self->clear_destroyed_session;
}

sub engine {
Expand Down Expand Up @@ -810,7 +901,7 @@ sub forward {
my $new_response = Dancer2->runner->dispatcher->dispatch(
$new_request->env,
$new_request,
$self->context,
($self->session)x!! $self->has_session,
);

# halt the response, so no further processing is done on this request.
Expand Down
102 changes: 0 additions & 102 deletions lib/Dancer2/Core/Context.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,107 +23,5 @@ has app => (
);


=attr session
Handle for the current session object, if any
=cut

has session => (
is => 'rw',
isa => Session,
lazy => 1,
builder => '_build_session',
predicate => '_has_session',
clearer => 1,
);

sub _build_session {
my ($self) = @_;
my $session;

# Find the session engine
my $engine = $self->app->engine('session');

# find the session cookie if any
if ( !$self->destroyed_session ) {
my $session_id;
my $session_cookie = $self->app->cookie( $engine->cookie_name );
if ( defined $session_cookie ) {
$session_id = $session_cookie->value;
}

# if we have a session cookie, try to retrieve the session
if ( defined $session_id ) {
eval { $session = $engine->retrieve( id => $session_id ) };
croak "Fail to retrieve session: $@"
if $@ && $@ !~ /Unable to retrieve session/;
}
}

# create the session if none retrieved
return $session ||= $engine->create();
}

=method has_session
Returns true if session engine has been defined and if either a session object
has been instantiated in the context or if a session cookie was found and not
subsequently invalidated.
=cut

sub has_session {
my ($self) = @_;

my $engine = $self->app->engine('session');

return $self->_has_session
|| ( $self->app->cookie( $engine->cookie_name )
&& !$self->has_destroyed_session );
}

=attr destroyed_session
We cache a destroyed session here; once this is set we must not attempt to
retrieve the session from the cookie in the request. If no new session is
created, this is set (with expiration) as a cookie to force the browser to
expire the cookie.
=cut

has destroyed_session => (
is => 'rw',
isa => InstanceOf ['Dancer2::Core::Session'],
predicate => 1,
);

=method destroy_session
Destroys the current session and ensures any subsequent session is created
from scratch and not from the request session cookie
=cut

sub destroy_session {
my ($self) = @_;

# Find the session engine
my $engine = $self->app->engine('session');

# Expire session, set the expired cookie and destroy the session
# Setting the cookie ensures client gets an expired cookie unless
# a new session is created and supercedes it
my $session = $self->session;
$session->expires(-86400); # yesterday
$engine->destroy( id => $session->id );

# Clear session in context and invalidate session cookie in request
$self->destroyed_session($session);
$self->clear_session;

return;
}


1;
13 changes: 6 additions & 7 deletions lib/Dancer2/Core/DSL.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,21 +136,20 @@ sub session {
# shortcut reads if no session exists, so we don't
# instantiate sessions for no reason
if ( @_ == 2 ) {
return unless $self->context->has_session;
return unless $self->app->has_session;
}

my $session = $self->context->session;
croak "No session available, a session engine needs to be set"
if !defined $session;
my $session = $self->app->session
|| croak "No session available, a session engine needs to be set";

$self->app->set_session($session);
$self->app->setup_session;

# return the session object if no key
return $session if @_ == 1;
@_ == 1 and return $session;

# read if a key is provided
return $session->read($key) if @_ == 2;
@_ == 2 and return $session->read($key);


# write to the session or delete if value is undef
if ( defined $value ) {
Expand Down
9 changes: 4 additions & 5 deletions lib/Dancer2/Core/Dispatcher.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ has default_content_type => (

# take the list of applications and an $env hash, return a Response object.
sub dispatch {
my ( $self, $env, $request, $curr_context ) = @_;
my ( $self, $env, $request, $curr_session ) = @_;

# warn "dispatching ".$env->{PATH_INFO}
# . " with ".join(", ", map { $_->name } @{$self->apps });
Expand All @@ -35,9 +35,6 @@ sub dispatch {
# we're going to parse the request body multiple times
my $context = Dancer2::Core::Context->new();

$curr_context and $curr_context->has_session
and $context->session( $curr_context->session );

foreach my $app ( @{ $self->apps } ) {
# warn "walking through routes of ".$app->name;

Expand All @@ -63,13 +60,15 @@ sub dispatch {
my $match = $route->match($request)
or next ROUTE;

$curr_session and $app->set_session($curr_session);

$request->_set_route_params($match);
$app->set_request($request);

my $response = with_return {
my ($return) = @_;

# stash the multilevel return coderef in the context
# stash the multilevel return coderef in the app
$app->has_with_return
or $app->with_return($return);

Expand Down
2 changes: 1 addition & 1 deletion lib/Dancer2/Core/Role/DSL.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ sub _compile_keyword {
my $code = $compiled_code;
$compiled_code = sub {
croak "Function '$keyword' must be called from a route handler"
unless defined $self->app->context;
unless defined $self->app;

This comment has been minimized.

Sign in to view

Copy link
@dams

dams Jun 2, 2014

Contributor

Hmm, from what I've seen, a DSL always has an app, so $self->app->context is always going to be true. If the code is correct, and the removal of context makes this check useless, then maybe it's worth removing the 'is_global' option from keywords ?

This comment has been minimized.

Sign in to view

Copy link
@xsawyerx

xsawyerx Jun 2, 2014

Member

Context was provided to get request and response objects. Both are now available within the app. That does mean that this check is probably unnecessary, as you point out.

I think it was used to make sure that the Context is available at the time that users call the keyword.

This comment has been minimized.

Sign in to view

Copy link
@dams

dams Jun 2, 2014

Contributor

Unfold the line 71 you'll see that the !$is_global test :) That's what I was referring to. This is_global option is used only here. So the difference between a global keyword and a normal one was that a normal one needed the context. Now that context has disappeared, the test is meaningless, and so the distinction between global / normal doesn't exist anymore technically. My question was: what's the "non technical" difference between global / normal keyword then ? If there is none anymore, then we should probably remove is_global. Note that I'm catching up back on the code, so I may well be saying crap here :)

This comment has been minimized.

Sign in to view

Copy link
@veryrusty

veryrusty Jun 2, 2014

Member

The is_global flag is needed, many of the DSL keywords only make sense in the context or a request or response. The response is lazy and has a builder (though you may not want a response created if no request exists??). The check in line 75 above probably needs to be unless $self->app->has_request.

This comment has been minimized.

Sign in to view

Copy link
@mickeyn

mickeyn Jul 5, 2014

Author Contributor

Fixed in c0169a5
$code->(@_);
};
}
Expand Down
8 changes: 4 additions & 4 deletions lib/Dancer2/Manual.pod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ define their own.
=head2 Request workflow

C<before> hooks are evaluated before each request within the context of the
request and receives as argument the context (a L<Dancer2::Core::Context>
request and receives as argument the app (a L<Dancer2::Core::App>
object).

It's possible to define variables which will be accessible in the action blocks
Expand Down Expand Up @@ -403,10 +403,10 @@ this hook.>

C<on_route_exception> is called when an exception has been caught, at the
route level, just before rethrowing it higher. This hook receives a
L<Dancer2::Core::Context> and the error as arguments.
L<Dancer2::Core::App> and the error as arguments.

hook on_route_exception => sub {
my ($context, $error) = @_;
my ($app, $error) = @_;
};

=head2 File rendering
Expand Down Expand Up @@ -1434,7 +1434,7 @@ You may also need to clear a session:
# destroy session
get '/logout' => sub {
...
context->destroy_session;
app->destroy_session;
...
};

Expand Down
Loading

0 comments on commit 9c3b21b

Please sign in to comment.