On Tue, 20 Jan 2009 09:42:51 +0000 wrote:
>
> On 20 Jan 2009, at 00:26, BUCHMULLER Norbert wrote:
> > Please review my patch (and apply if you accept it).
>
> Looks semi-reasonable, but there are no tests for the functionality
> change, can you add some to show that expiry now works etc?
Yes, of course. It was late and I was too sleepy and I forgot
it. :-( Also I'll try to write test cases for the
delete_expired_sessions() functionality of ::DBI (and maybe others if
they don't have such a test).
> I'm also slightly worried about the change to get_session_data:
>
> + return $cache_obj
> + ? $cache_obj->get_expires_at
> + : undef;
>
> That means (to my reading), return the expiry time, if there is a
> cache object, or undef if there isn't - and I think you mean check
> the expiry time has not passed - if it has return undef, if it
> hasn't, return the object..
I think that the above code is right, if we accept a few assumptions
(those are the same that are assumed by ::DBI), namely:
* "flash:id" and "session:id" share their expiry time
* "flash:id" is never set before "session:id" is set
* "expires:id" is always set after "session:id", or "expires:id" contains
the same value as $c->session_expires
* "expires:id" is never fetched before setting "session:id"
Cache::Cache::get() returns the data stored in the cache object or undef
if not found (used when the key is not "expires:id").
Cache::Cache::get_object() returns a Cache::Object instance, which can
tell us the expiry time of the object (and that's the value associted
with the "expires:id" key), or undef if not found.
> If you have chance, maybe you could stop by #catalyst-dev and get a
> commit-bit? Then you can work on this change (and give people a
> chance to test it) in a branch until its ready for trunk..
I'll do so.
BTW, I don't want to hurt anyone, but sometimes I feel that the
Catalyst::Plugin::Session::Store interface is not one of the nicest
and most effective interfaces I've seen.. I feel it's too general, still
too restrictive in practical use, and rather confusing:
* the expiry time is carried independently of the entry it refers to
(ie. two independent calls on the interface API, OTOH the underlying
storage wants both of them in one API call)
* the order in which the corresponding keys (ie. "session:id" and
"expires:id") are set/get is not defined
* so a lot of the implementations use some assumptions (that are not
documented anywhere), just to be able to give optimal solutions (and they
will break if something is changed in the implementation of ::Storage)
* the prefix and key has to be split and mangled in the storage
implementations (a separate namespace and id pair would be more
straightforward, but that's just aesthetics)
I'd like to discuss it a bit, so that I can come up with either POD
patches (so to make those assumptions part of the official API) or maybe
some plan on extending the API (for the long term, with transition path).
Or you just point me to the docs I missed and/or prove me that I
misunderstood something. :-)
norbi
_______________________________________________
Catalyst-dev mailing list
Catalyst-dev@lists.scsys.co.uk
http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst-dev
