Bug 36098 - Create Koha::Session module
Summary: Create Koha::Session module
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 34478 34755
  Show dependency treegraph
 
Reported: 2024-02-15 02:18 UTC by David Cook
Modified: 2024-04-03 22:15 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.10,22.11.16


Attachments
Bug 36098: Add Koha::Session module to ease session handling (6.74 KB, patch)
2024-02-15 02:50 UTC, David Cook
Details | Diff | Splinter Review
Bug 36098: Add Koha::Session module to ease session handling (6.82 KB, patch)
2024-02-15 11:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36098: (QA follow-up) Add POD to Koha::Session (1.79 KB, patch)
2024-02-15 11:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36098: Allow to pass storage_method (1.40 KB, patch)
2024-02-15 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36098: Allow to pass storage_method (1.45 KB, patch)
2024-02-15 13:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36098: Fix storage_method pass (762 bytes, patch)
2024-02-15 22:49 UTC, David Cook
Details | Diff | Splinter Review
Bug 36098: (follow-up) extend test to check driver (984 bytes, patch)
2024-02-15 23:11 UTC, David Cook
Details | Diff | Splinter Review
Bug 36098: Default to 'file' if pref does not exist (1.02 KB, patch)
2024-02-21 08:43 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2024-02-15 02:18:37 UTC
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.
Comment 1 David Cook 2024-02-15 02:50:46 UTC
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
Comment 2 Martin Renvoize 2024-02-15 11:57:56 UTC
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>
Comment 3 Martin Renvoize 2024-02-15 11:57:58 UTC
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>
Comment 4 Martin Renvoize 2024-02-15 11:58:49 UTC
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
Comment 5 Jonathan Druart 2024-02-15 13:03:54 UTC
Please don't push now. Wait for the rest of the tree.
Comment 6 Jonathan Druart 2024-02-15 13:05:46 UTC
Created attachment 162204 [details] [review]
Bug 36098: Allow to pass storage_method

Will need this on follow-up bugs.
Comment 7 Jonathan Druart 2024-02-15 13:08:19 UTC
Created attachment 162205 [details] [review]
Bug 36098: Allow to pass storage_method

Will need this on follow-up bugs.
Comment 8 David Cook 2024-02-15 22:21:44 UTC
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.
Comment 9 David Cook 2024-02-15 22:22:09 UTC
(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.
Comment 10 David Cook 2024-02-15 22:49:48 UTC
Created attachment 162228 [details] [review]
Bug 36098: Fix storage_method pass
Comment 11 David Cook 2024-02-15 23:11:21 UTC
Created attachment 162231 [details] [review]
Bug 36098: (follow-up) extend test to check driver
Comment 12 David Cook 2024-02-15 23:11:45 UTC
Thought I'd reset to "Needs Signoff" as we've added another 3 patches since Martin's original SO.
Comment 13 Jonathan Druart 2024-02-16 07:32:37 UTC
(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".
Comment 14 Jonathan Druart 2024-02-21 08:43:25 UTC
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 15 David Cook 2024-02-21 22:29:19 UTC
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.
Comment 16 Jonathan Druart 2024-03-01 08:14:11 UTC
(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...
Comment 17 Jonathan Druart 2024-03-01 13:20:06 UTC
Pushed to master for 24.05.00.
Comment 18 David Cook 2024-03-04 04:30:16 UTC
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.
Comment 19 Tomás Cohen Arazi 2024-03-08 19:26:19 UTC
This applies nicely on 23.11.x and works as expected.
Comment 20 Fridolin Somers 2024-03-11 09:03:19 UTC
Pushed to 23.11.x for 23.11.04
Comment 21 Lucas Gass 2024-03-19 15:04:41 UTC
Backported to 23.05.x for upcoming 23.05.10
Comment 22 David Cook 2024-04-02 23:09:52 UTC
Hoping we can get this one and bug 34755 into 22.11
Comment 23 Frédéric Demians 2024-04-03 08:39:33 UTC
Backported to 22.11.x for 22.11.16
Comment 24 David Cook 2024-04-03 22:15:47 UTC
(In reply to Frédéric Demians from comment #23)
> Backported to 22.11.x for 22.11.16

Many thanks :D