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()
Download (untitled) / with headers
text/plain 636b
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>
Download (untitled) / with headers
text/plain 784b
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
Download (untitled) / with headers
text/plain 1008b
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>
Download (untitled) / with headers
text/plain 588b
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
Download (untitled) / with headers
text/plain 723b
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>
Download (untitled) / with headers
text/plain 262b
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


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.