[Catalyst-dev] Catalyst::Plugin::Session::Store::File patch to use the Cache::FileCache expiry mechanism

[Tomas Doran]

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?

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 may be _totally_ wrong here, as I'm not familiar with (and haven't  
refreshed myself on) the Cache::FileCache interface - but either way,  
a comment to note the intention would probably go well here..

Many thanks for the patch however, I'll look forward to a re-submit  
with my concerns addressed.

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..

Cheers
t0m