Bug 17427 - Replace CGI::Session with Data::Session
Summary: Replace CGI::Session with Data::Session
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords: dependency, rel_21_11_candidate, RM_priority
Depends on: 28519
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-11 13:34 UTC by Jonathan Druart
Modified: 2021-06-28 01:31 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17427: Replace CGI::Session with Data::Session (11.25 KB, patch)
2016-10-11 13:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (14.12 KB, patch)
2016-10-14 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (14.19 KB, patch)
2016-10-16 12:49 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (14.17 KB, patch)
2016-11-02 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (14.16 KB, patch)
2017-02-16 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (11.27 KB, patch)
2021-05-24 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use JSON serialization (2.13 KB, patch)
2021-06-04 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (11.44 KB, patch)
2021-06-07 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use JSON serialization (2.16 KB, patch)
2021-06-07 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use our own YAML::XS serializer (2.33 KB, patch)
2021-06-07 12:14 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 Jonathan Druart 2016-10-11 13:34:37 UTC
CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}


If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL
Comment 1 Jonathan Druart 2016-10-11 13:37:56 UTC
Created attachment 56185 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

This is just a POC and will only works with SessionStorage set to MySQL.

Please test it and tell me if you find bug. If it looks bug free I will submit
a follow-up to cover the other combinations of SessionStorage (temporary storage
and Memcached)
Comment 2 Benjamin Rokseth 2016-10-13 22:00:22 UTC
This is great detective work, and it makes real sense to deprecate a deprecated session handler! I have tested with api and finally tests with autocommit set to null actually rolls back, so I no longer am left with a skewed db.

please bring this on and I will happily test and sign off!
Comment 3 Jonathan Druart 2016-10-14 10:21:39 UTC
Created attachment 56504 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.
Comment 4 Jonathan Druart 2016-10-14 10:22:18 UTC
(In reply to Benjamin Rokseth from comment #2)
> This is great detective work, and it makes real sense to deprecate a
> deprecated session handler! I have tested with api and finally tests with
> autocommit set to null actually rolls back, so I no longer am left with a
> skewed db.
> 
> please bring this on and I will happily test and sign off!

It's your turn to play ;)
Comment 5 Jonathan Druart 2016-10-14 10:22:52 UTC
I should note: The first patch did not work as expected, it defaulted to File (instead of mysql).
Comment 6 Benjamin Rokseth 2016-10-16 12:49:28 UTC
Created attachment 56572 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 7 Benjamin Rokseth 2016-10-16 12:51:37 UTC
Works perfectly! Corrected a small typo in the PerlDependencies.

No sessions lost in tempfiles, mysql or memcached, and they survived a plack restart.
Comment 8 Martin Renvoize 2016-10-21 14:28:19 UTC
Comment on attachment 56572 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

Review of attachment 56572 [details] [review]:
-----------------------------------------------------------------

Generally looks good.. minor question about using ENV though.. not sure about that part of the change.

::: C4/Auth.pm
@@ +1111,4 @@
>                      $session->param( 'branchname',   $branchname );
>                      $session->param( 'flags',        $userflags );
>                      $session->param( 'emailaddress', $emailaddress );
> +                    $session->param( 'ip',           $ENV{REMOTE_ADDR} );

Did we test this against plack?  Can we really rely on ENV for remote_addr... and in fact.. do we not compare the session ip to the env remote_addr in places as a security check?
Comment 9 Tomás Cohen Arazi 2016-10-21 15:50:32 UTC
(In reply to Martin Renvoize from comment #8)
> Comment on attachment 56572 [details] [review] [review]
> Bug 17427: Replace CGI::Session with Data::Session
> 
> Review of attachment 56572 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Generally looks good.. minor question about using ENV though.. not sure
> about that part of the change.
> 
> ::: C4/Auth.pm
> @@ +1111,4 @@
> >                      $session->param( 'branchname',   $branchname );
> >                      $session->param( 'flags',        $userflags );
> >                      $session->param( 'emailaddress', $emailaddress );
> > +                    $session->param( 'ip',           $ENV{REMOTE_ADDR} );
> 
> Did we test this against plack?  Can we really rely on ENV for
> remote_addr... and in fact.. do we not compare the session ip to the env
> remote_addr in places as a security check?

$ENV{REMOTE_ADDR} is correctly filled on each request by the ReverseProxy middleware as far as I recall (I debugged plack+ENV several days before making the packages plack integration official).
Comment 10 Tomás Cohen Arazi 2016-11-01 12:47:50 UTC
Please rebase. I'd say this one depends only on Mirko's answer about packaging Data::Session.
Comment 11 Jonathan Druart 2016-11-02 09:02:42 UTC
Created attachment 57067 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 12 Jonathan Druart 2016-11-02 09:04:05 UTC
I do not recommend to push this patch into the next stable release (at least not at the beginning).
Comment 13 Jonathan Druart 2017-02-16 14:42:40 UTC
Created attachment 60355 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 14 Jonathan Druart 2017-02-16 14:43:03 UTC
Just a rebase...
Comment 15 Marcel de Rooy 2017-02-17 08:05:21 UTC
QA: Looking here now
Comment 16 Marcel de Rooy 2017-02-17 08:12:32 UTC
(In reply to Benjamin Rokseth from comment #7)
> Works perfectly! Corrected a small typo in the PerlDependencies.
> 
> No sessions lost in tempfiles, mysql or memcached, and they survived a plack
> restart.

Benjamin: What browsers did you test?
Comment 17 Marcel de Rooy 2017-02-17 09:03:54 UTC
This seems to break scripts like stage-marc-import and tools/upload.pl.
I am still testing a bit but after successfully uploading one file, I am not able to upload anymore.
Also saw this error:
stage-marc-import.pl: YAML::Tiny does not support C4::BackgroundJob references at /usr/local/share/perl/5.20.2/Data/Session/Serialize/YAML.pm line 18., 

Suspect that all scripts using BackgroungJob have issues now. Note that these jobs append information to the session.
Comment 18 Marcel de Rooy 2017-02-17 10:24:04 UTC
Data::Session creates a CGI object.
And we do not like that in scripts like upload-file.pl that call check_cookie_auth (calling get_session).
The created CGI object in this case makes the upload disappear.
Other scripts may have issues too going the similar cookie_auth way..
Comment 19 Jonathan Druart 2017-02-17 13:39:34 UTC
(In reply to Marcel de Rooy from comment #17)
> This seems to break scripts like stage-marc-import and tools/upload.pl.
> I am still testing a bit but after successfully uploading one file, I am not
> able to upload anymore.
> Also saw this error:
> stage-marc-import.pl: YAML::Tiny does not support C4::BackgroundJob
> references at /usr/local/share/perl/5.20.2/Data/Session/Serialize/YAML.pm
> line 18., 

That can be fixed using DataDumper or FreezeThaw instead of YAML as serializer.

(In reply to Marcel de Rooy from comment #18)
> Data::Session creates a CGI object.
> And we do not like that in scripts like upload-file.pl that call
> check_cookie_auth (calling get_session).
> The created CGI object in this case makes the upload disappear.
> Other scripts may have issues too going the similar cookie_auth way..

I do not know how to fix that.
Comment 20 Jonathan Druart 2021-05-24 14:14:44 UTC
Created attachment 121337 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.
Comment 21 Jonathan Druart 2021-05-24 14:15:11 UTC
(patch rebased)
Comment 22 David Cook 2021-05-25 01:09:01 UTC
(In reply to Jonathan Druart from comment #21)
> (patch rebased)

Should this be "Needs Signoff" now?
Comment 23 Jonathan Druart 2021-05-25 15:08:58 UTC
(In reply to David Cook from comment #22)
> (In reply to Jonathan Druart from comment #21)
> > (patch rebased)
> 
> Should this be "Needs Signoff" now?

I rebased it to see if it fixed bug 28158, it doesn't.
This patch can be tested, but I haven't much so far.

In my opinion that's still one we need, maybe a good candidate for early 21.11 adoption?

Feel free to test it!
Comment 24 Jonathan Druart 2021-06-04 15:35:46 UTC
Created attachment 121633 [details] [review]
Bug 17427: Use JSON serialization
Comment 25 Jonathan Druart 2021-06-04 15:41:24 UTC
We are now aiming to provide a long-term solution for the encoding issue found on bug 28489.

Using Data::Session using the JSON serializer seems to be the more robust solution.

However there is still an issue if SessionStorage=tmp file

Note that sessions.a_session must be longtext.
Comment 26 Jonathan Druart 2021-06-04 15:42:34 UTC
(In reply to Jonathan Druart from comment #25)
> However there is still an issue if SessionStorage=tmp file


root@kohadevbox$ more /var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967
{"_SESSION_CTIME":1622821220,"_SESSION_ETIME":0,"id":"koha","interface":"intranet","number":51,"ip":"172.20.0.1","lasttime":"1622821226","_SESSION_PTIME":{},"_SESSION_ATIME":1622821226,"flags":1,"surname":"koha"
,"cardnumber":"42","_SESSION_ID":"6967e16ccb3dddff1c32735b7930e967","branchname":"✔️ ❤️ ★","branch":"✔️❤️★"}
root@kohadevbox$ file /var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967
/var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967: JSON data
Comment 27 Katrin Fischer 2021-06-04 15:54:00 UTC
Bug 28517 might be related to this?
Comment 28 Jonathan Druart 2021-06-07 11:35:52 UTC
JSON serialization does not work (when SessionStorage=tmp), selenium tests from bug 28489 are failing.

With longblob:
    #   Failed test 'logged-in-branch-name set - SessionStorage=mysql, branchname=Test2❤️'                
    #   at t/db_dependent/selenium/regressions.t line 306.                      
    #          got: 'Test2â¤ï¸Â'                                                                  
    #     expected: 'Test2❤️'      

With longtext:
    #   Failed test 'logged-in-branch-name set - SessionStorage=tmp, branchname=Test2❤️'
    #   at t/db_dependent/selenium/regressions.t line 306.
    #          got: 'Test2â¤ï¸'
    #     expected: 'Test2❤️'
Comment 29 Jonathan Druart 2021-06-07 11:45:19 UTC
And it's not possible to add a serializer that is not in the same dir as others, because of this code in Data::Session:

 318 sub get_my_serializers
 319 {
 320     my($self)   = @_;
 321     my($path)   = $self -> _get_pm_path('Serialize');
 322 
 323     # Warning: Use sort map{} read_dir, not map{} sort read_dir. But, why?
 324 
 325     my(@serializer) = sort map{s/.pm//; $_} read_dir($path);
 326 
 327     ($#serializer < 0) && die __PACKAGE__ . '. No serializers available';
 328 
 329     ($self -> verbose > 1) && $self -> log('Serializers: ' . join(', ', @serializer) );
 330 
 331     $self -> my_serializers(\@serializer);
 332 
 333 } # End of get_my_serializers.
Comment 30 Jonathan Druart 2021-06-07 12:03:04 UTC
(In reply to Jonathan Druart from comment #28)

This comment was confusing, with JSON serialization we have:
With longtext it's working with SessionStorage=mysql
With longblob it's failing with mysql.

But it does not work with SessionStorage=tmp.
Comment 31 Jonathan Druart 2021-06-07 12:14:17 UTC
Created attachment 121657 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.
Comment 32 Jonathan Druart 2021-06-07 12:14:22 UTC
Created attachment 121658 [details] [review]
Bug 17427: Use JSON serialization
Comment 33 Jonathan Druart 2021-06-07 12:14:27 UTC
Created attachment 121659 [details] [review]
Bug 17427: Use our own YAML::XS serializer
Comment 34 Jonathan Druart 2021-06-07 12:15:21 UTC
And, FWIW, if I move the YAML::XS serializer (see last patch) in the Data/Session/Serialize directory, all tests pass (longblob).
Comment 35 David Cook 2021-06-07 23:16:14 UTC
(In reply to Jonathan Druart from comment #34)
> And, FWIW, if I move the YAML::XS serializer (see last patch) in the
> Data/Session/Serialize directory, all tests pass (longblob).

Does this have an unlisted dependency on Bug 28519? I'm just curious since the serializers are in a "lib" directory.
Comment 36 Jonathan Druart 2021-06-08 07:39:02 UTC
Yes David.
Comment 37 Mason James 2021-06-12 03:07:23 UTC
hi joubu
i've uploaded a libdata-session-perl pkg to koha-staging/dev, for testing

root@kohadevbox:/kohadevbox# apt install libdata-session-perl
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following additional packages will be installed:
  libautovivification-perl libdbd-sqlite3-perl libdbix-admin-createtable-perl
  libfile-slurper-perl libhash-fieldhash-perl libperlio-utf8-strict-perl
The following NEW packages will be installed:
  libautovivification-perl libdata-session-perl libdbd-sqlite3-perl
  libdbix-admin-createtable-perl libfile-slurper-perl libhash-fieldhash-perl
  libperlio-utf8-strict-perl
0 upgraded, 7 newly installed, 0 to remove and 2 not upgraded.
Need to get 376 kB of archives.

root@kohadevbox:/kohadevbox# apt-cache policy libdata-session-perl
libdata-session-perl:
  Installed: 1.18-1
  Candidate: 1.18-1
  Version table:
 *** 1.18-1 500
        500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages
        100 /var/lib/dpkg/status
Comment 38 David Cook 2021-06-25 04:43:19 UTC
(In reply to Jonathan Druart from comment #34)
> And, FWIW, if I move the YAML::XS serializer (see last patch) in the
> Data/Session/Serialize directory, all tests pass (longblob).

Wait this works despite https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ?
Comment 39 David Cook 2021-06-25 04:45:33 UTC
(In reply to David Cook from comment #38)
> (In reply to Jonathan Druart from comment #34)
> > And, FWIW, if I move the YAML::XS serializer (see last patch) in the
> > Data/Session/Serialize directory, all tests pass (longblob).
> 
> Wait this works despite
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ?

Oh maybe I misunderstand what you're saying... you meant in the vendor perl not the Koha perl lib... right?
Comment 40 Jonathan Druart 2021-06-25 06:30:40 UTC
(In reply to David Cook from comment #39)
> (In reply to David Cook from comment #38)
> > (In reply to Jonathan Druart from comment #34)
> > > And, FWIW, if I move the YAML::XS serializer (see last patch) in the
> > > Data/Session/Serialize directory, all tests pass (longblob).
> > 
> > Wait this works despite
> > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ?
> 
> Oh maybe I misunderstand what you're saying... you meant in the vendor perl
> not the Koha perl lib... right?

Yes, I copied it to /path/Data/Session/Serialize/yamlxs.pm where /path/Data/Session/ is `pmpath Data::Session`.
Comment 41 David Cook 2021-06-25 07:11:36 UTC
(In reply to Jonathan Druart from comment #40)
> > Oh maybe I misunderstand what you're saying... you meant in the vendor perl
> > not the Koha perl lib... right?
> 
> Yes, I copied it to /path/Data/Session/Serialize/yamlxs.pm where
> /path/Data/Session/ is `pmpath Data::Session`.

But that's not a real solution for production. Shouldn't "Needs Signoff" be changed back to 'Assigned' or "In Discussion"?
Comment 42 Jonathan Druart 2021-06-25 08:31:00 UTC
Ok, let's discuss then. What do we do?

I am ok with having our module where Data::Session is expecting it.
Comment 43 David Cook 2021-06-28 01:31:54 UTC
(In reply to Jonathan Druart from comment #42)
> Ok, let's discuss then. What do we do?
>
> I am ok with having our module where Data::Session is expecting it.

How do you propose to do that in production though? 

The only reasonable option I see would be creating our Data::Session::Serializer::yamlxs as a standalone library (with its own git repository), and then packaging that and storing it in the Koha Community APT repo, and then adding it as a Koha dependency.