There are times where you need to interact with Koha sessions, but you can't or don't want to load C4::Auth. I propose creating a minimal Koha::Session module, which will initially only have 1 method for fetching a session object, but over time can be built up.
Created attachment 162167 [details] [review] Bug 36098: Add Koha::Session module to ease session handling This patch adds a Koha::Session module that makes it easier to work with Koha sessions without needing the full C4::Auth module. Test plan: 0. Apply the patch 1. Run the following unit tests: prove ./t/db_dependent/Auth.t prove ./t/db_dependent/Auth_with_cas.t prove ./t/db_dependent/Koha/Session.t 2. Observe that they all pass
Created attachment 162200 [details] [review] Bug 36098: Add Koha::Session module to ease session handling This patch adds a Koha::Session module that makes it easier to work with Koha sessions without needing the full C4::Auth module. Test plan: 0. Apply the patch 1. Run the following unit tests: prove ./t/db_dependent/Auth.t prove ./t/db_dependent/Auth_with_cas.t prove ./t/db_dependent/Koha/Session.t 2. Observe that they all pass Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 162201 [details] [review] Bug 36098: (QA follow-up) Add POD to Koha::Session Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This is a simple and clear patch, opting to straight QA it to keep things moving here. QA scripts happy, Unit tests happy, Tests added and passing. Passing QA
Please don't push now. Wait for the rest of the tree.
Created attachment 162204 [details] [review] Bug 36098: Allow to pass storage_method Will need this on follow-up bugs.
Created attachment 162205 [details] [review] Bug 36098: Allow to pass storage_method Will need this on follow-up bugs.
Comment on attachment 162205 [details] [review] Bug 36098: Allow to pass storage_method Review of attachment 162205 [details] [review]: ----------------------------------------------------------------- ::: Koha/Session.pm @@ +51,4 @@ > =cut > > sub _get_session_params { > + my ( $class, $storage_method ) = @_; $storage_method is a hashref in get_session() but treated as a string below. This should be updated to $args and $storage_method should be pulled from it.
(In reply to Jonathan Druart from comment #7) > Created attachment 162205 [details] [review] [review] > Bug 36098: Allow to pass storage_method > > Will need this on follow-up bugs. Could you explain why? I don't see why yet.
Created attachment 162228 [details] [review] Bug 36098: Fix storage_method pass
Created attachment 162231 [details] [review] Bug 36098: (follow-up) extend test to check driver
Thought I'd reset to "Needs Signoff" as we've added another 3 patches since Martin's original SO.
(In reply to David Cook from comment #9) > (In reply to Jonathan Druart from comment #7) > > Created attachment 162205 [details] [review] [review] [review] > > Bug 36098: Allow to pass storage_method > > > > Will need this on follow-up bugs. > > Could you explain why? I don't see why yet. On bug 36102, "Use Koha::Session from C4::InstallAuth".
Created attachment 162314 [details] [review] Bug 36098: Default to 'file' if pref does not exist During the installer process there is a bunch of warnings "Use of uninitialized value $storage_method in string eq at"
Comment on attachment 162314 [details] [review] Bug 36098: Default to 'file' if pref does not exist Review of attachment 162314 [details] [review]: ----------------------------------------------------------------- ::: Koha/Session.pm @@ +53,4 @@ > sub _get_session_params { > my ( $class, $args ) = @_; > my $storage_method = $args->{storage_method}; > + $storage_method ||= C4::Context->preference('SessionStorage') || 'file'; This isn't very human-friendly to read. I think we should either add a comment explaining it, or make it easier to read (which would admittedly be more verbose). Not the end of the world if we leave as is of course.
(In reply to David Cook from comment #15) > Comment on attachment 162314 [details] [review] [review] > Bug 36098: Default to 'file' if pref does not exist > > Review of attachment 162314 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Session.pm > @@ +53,4 @@ > > sub _get_session_params { > > my ( $class, $args ) = @_; > > my $storage_method = $args->{storage_method}; > > + $storage_method ||= C4::Context->preference('SessionStorage') || 'file'; > > This isn't very human-friendly to read. Really? This is trivial Perl...
Pushed to master for 24.05.00.
I'm actually changing this from "enhancement" to "normal" (although maybe it should be more severe), since it's needed to fix bug 34755 which is a bug that was reported around 6 months ago. (It took me a while to reproduce the problem and figure out the root cause, so here we are.) It would be great to get this backported to the stable branches.
This applies nicely on 23.11.x and works as expected.
Pushed to 23.11.x for 23.11.04
Backported to 23.05.x for upcoming 23.05.10
Hoping we can get this one and bug 34755 into 22.11
Backported to 22.11.x for 22.11.16
(In reply to Frédéric Demians from comment #23) > Backported to 22.11.x for 22.11.16 Many thanks :D