Skip Menu |

This queue is for tickets about the CGI-Session CPAN distribution.

Report information
The Basics
Id: 21808
Status: resolved
Priority: 0/
Queue: CGI-Session

People
Owner: Nobody in particular
Requestors: fenlisesi [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: Wishlist
Broken in: 4.14
Fixed in: (no value)



Subject: notabug: some work on the constructor load()
In the attached patch, I suggest some refactoring and cleanup to load(), the (sub)constructor for calls to new() as a class method. I also put in some questions/discussion points. Those are marked with the META: tag. The patch is against 4-14. It passes all the non-skipped tests that come with the 4-14 distribution on my machine, but please examine it closely anyway, as nobody reviewed it. Cheers, $CGI::Session::VERSION = '4.14'; This is perl, v5.8.8 built for i686-linux-thread-multi Linux version 2.6.11.4-21.14-default (geeko@buildhost) (gcc version 3.3.5 20050117 (prerelease) (SUSE Linux)) #1 Thu Aug 24 09:51:41 UTC 2006
Subject: CGI-Session-load-4.14.patch

Message body is not shown because it is too large.

Subject: Re: [rt.cpan.org #21808] notabug: some work on the constructor load()
Date: Tue, 10 Oct 2006 23:15:23 -0500
To: bug-CGI-Session [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
In general I like this patch and will accept it. However, it does not apply cleanly to the current version, which has several changes since the last release. Would you mind checking a copy through subversion, as documented in the POD, and making sure the changes work with the latest version? I did incorporate answers to your META questions manually. Show quoted text
> # META: Is '0' not an acceptable _CLAIMED_ID? Should we test for > # definedness rather than truth?
Yes. Changes. Show quoted text
> # META: "Too many arguments" could be a better msg
Agreed. Changed. Show quoted text
> # META: Remove this line, or drop a line here on why it remains
Removed. I also liked your refactor to create _load_pluggables, but I didn't manually merge that in though. Mark -- http://mark.stosberg.com/
From: fenlisesi [...] gmail.com
Mark, I tried to use the Web interface to svn, but i got: Error running this command: svnlook youngest '/var/svn/CGI-Session' __aLi__ On Tue Oct 10 23:15:36 2006, mark@summersault.com wrote: Show quoted text
> > In general I like this patch and will accept it. However, it does not > apply cleanly to the current version, which has several changes since > the last release. Would you mind checking a copy through subversion, as > documented in the POD, and making sure the changes work with the latest > version? > > I did incorporate answers to your META questions manually. >
> > # META: Is '0' not an acceptable _CLAIMED_ID? Should we test for > > # definedness rather than truth?
> > Yes. Changes. >
> > # META: "Too many arguments" could be a better msg
> > Agreed. Changed. >
> > # META: Remove this line, or drop a line here on why it remains
> > Removed. > > I also liked your refactor to create _load_pluggables, but I didn't > manually merge that in though. > > Mark >
CC: perl [...] cromedome.net
Subject: Re: [rt.cpan.org #21808] web gui for svn not working
Date: Sun, 22 Oct 2006 16:34:45 -0500
To: bug-CGI-Session [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Ali ISIK via RT wrote: Show quoted text
> Queue: CGI-Session > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=21808 > > > Mark, > > I tried to use the Web interface to svn, but i got: > > Error running this command: svnlook youngest '/var/svn/CGI-Session'
Ali, I'm sorry about that. I'm cc'ing the person to who manages that for us. Perhaps he can help. However, you could still be able to do an svn checkout of it: svn co svn://svn.cromedome.net/ ( I hope I got the address right! ) Thanks for your continued interest in this project. Mark -- http://mark.stosberg.com/
From: fenlisesi [...] gmail.com
The attached patch is against the current svn. Cheers. On Sun Oct 22 16:37:02 2006, mark@summersault.com wrote: Show quoted text
> Ali ISIK via RT wrote:
> > Queue: CGI-Session > > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=21808 > > > > > Mark, > > > > I tried to use the Web interface to svn, but i got: > > > > Error running this command: svnlook youngest '/var/svn/CGI-Session'
> > Ali, > > I'm sorry about that. I'm cc'ing the person to who manages that for us. > Perhaps he can help. > > However, you could still be able to do an svn checkout of it: > > svn co svn://svn.cromedome.net/ > > ( I hope I got the address right! ) > > Thanks for your continued interest in this project. > > Mark >
--- Session-original.pm 2006-11-04 20:45:10.000000000 +0200 +++ Session-edited.pm 2006-11-05 00:50:08.000000000 +0200 @@ -672,23 +672,7 @@ sub load { } - # setting defaults, since above arguments might be 'undef' - $self->{_DSN}->{driver} ||= "file"; - $self->{_DSN}->{serializer} ||= "default"; - $self->{_DSN}->{id} ||= "md5"; - - # Checking and loading driver, serializer and id-generators - # Is this untainting reasonable here? - for ( - "CGI::Session::Driver::" . ($self->{_DSN}->{driver} =~ /(.*)/)[0], - "CGI::Session::Serialize::" . ($self->{_DSN}->{serializer} =~ /(.*)/)[0], - "CGI::Session::ID::" . ($self->{_DSN}->{id} =~ /(.*)/)[0], - ) { - eval "require $_"; - if ($@ ) { - return $self->set_error("couldn't load $_: " . $@); - } - } + $self->_load_pluggables(); if (not defined $self->{_CLAIMED_ID}) { my $query = $self->query(); @@ -773,6 +757,48 @@ sub _set_query_or_sid { } +sub _load_pluggables { + my ($self) = @_; + + my %DEFAULT_FOR = ( + driver => "file", + serializer => "default", + id => "md5", + ); + my %SUBDIR_FOR = ( + driver => "Driver", + serializer => "Serialize", + id => "ID", + ); + my $dsn = $self->{_DSN}; + foreach my $plug qw(driver serializer id) { + my $mod_name = $dsn->{ $plug }; + if (not defined $mod_name) { + $mod_name = $DEFAULT_FOR{ $plug }; + } + if ($mod_name =~ /^(\w+)$/) { + + # Looks good. Put it into the dsn hash + $dsn->{ $plug } = $mod_name = $1; + + # Put together the actual module name to load + my $prefix = join '::', (__PACKAGE__, $SUBDIR_FOR{ $plug }, q{}); + $mod_name = $prefix . $mod_name; + + ## See if we can load load it + eval "require $mod_name"; + if ($@) { + my $msg = $@; + return $self->set_error("couldn't load $mod_name: " . $msg); + } + } + else { + # do something here about bad name for a pluggable + } + } + return; +} + =pod =head2 id()
Subject: Re: [rt.cpan.org #21808] notabug: some work on the constructor load()
Date: Mon, 06 Nov 2006 13:13:02 -0500
To: bug-CGI-Session [...] rt.cpan.org
From: Mark Stosberg <mark [...] summersault.com>
Ali ISIK via RT wrote: Show quoted text
> Queue: CGI-Session > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=21808 > > > The attached patch is against the current svn. Cheers.
Applied cleanly and passed tests (with load.t update). Committed. Thanks. Mark