Bug 28489

Summary: CGI::Session is incorrectly serialized to DB in production env / when strict_sql_modes = 0
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: AuthenticationAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: dcook, dpavlin, fridolin.somers, jonathan.druart, julian.maurice, kyle, mtj, nick, nugged, resetka.bedi, stalkernoid, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.01
Bug Depends on: 28317    
Bug Blocks: 28519    
Attachments: Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)
Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)
Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)
Bug 28489: Fix issues with umlaut in session vars
Bug 28489: Add selenium tests
Bug 28489: Add selenium tests
Bug 28489: POD - CGI::Session::Serialize::yamlxs for CGI::Session
Bug 28489: Don't deal with encoding during the serialization
Bug 28489: Put CGI::Session::Serialize::yamlxs in lib directory
Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)
Bug 28489: Add selenium tests
Bug 28489: POD - CGI::Session::Serialize::yamlxs for CGI::Session
Bug 28489: Don't deal with encoding during the serialization
Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)
Bug 28489: Add selenium tests
Bug 28489: POD - CGI::Session::Serialize::yamlxs for CGI::Session
Bug 28489: Don't deal with encoding during the serialization
Bug 28489: Modify sessions.a_session from longtext to longblob
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)
Bug 28489: Add selenium tests
Bug 28489: POD - CGI::Session::Serialize::yamlxs for CGI::Session
Bug 28489: Don't deal with encoding during the serialization

Description Joonas Kylmälä 2021-05-31 08:15:31 UTC
The changes introduced in bug 28317 regarding CGI::Sessions serialization works OK with koha-testing-docker where KOHA_TESTING env and "strict_sql_modes" koha-conf.xml config is set to True. However, according to our documentation strict_sql_modes it should not be used in production environment. If it is disabled then the CGI::Session serialization encoding breaks.

To reproduce:

1) Set strict_sql_modes = 1 in koha-conf.xml (depending on your test environment variables you might also just wanna uncomment the relevant code in Koha/Database.pm)
2) Create branch with a display name "Testä" and switch to that branch in intranet. 
3) Go to some page in intranet and notice the branch is displayed incorrectly in the menu

Alternatively this can cause major trouble if for example a branchcode contains non-ascii letters, e.g. Ä. Then for example on returns.pl we get following error (and I think lots of other things break as well):

> Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 1 Joonas Kylmälä 2021-05-31 08:21:04 UTC
Relating to this is another bug but it only happens with strict_sql_modes = 1, so if we come up with a fix for this we should make sure it handles this case as well.

The other issue is:

> CGI::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do
> failed: Incorrect string value: '\xE4iskap...' for column
> `koha_kohadev`.`sessions`.`a_session` at row 1 [for Statement "INSERT INTO
> sessions (id, a_session) VALUES(?, ?) ON DUPLICATE KEY UPDATE a_session =
> ?"] at /usr/share/perl5/CGI/Session.pm line 251
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77

This happens when trying to to set on the page /cgi-bin/koha/circ/set-library.pl the active branch another time to what is selected already AND the branch display contains an "ä" character.
Comment 2 Joonas Kylmälä 2021-05-31 08:25:22 UTC
The problem is fixed by enabling the Mariadb/Mysql STRICT_TRANS_TABLES mode.
Comment 3 Joonas Kylmälä 2021-05-31 08:29:42 UTC
(In reply to Joonas Kylmälä from comment #2)
> The problem is fixed by enabling the Mariadb/Mysql STRICT_TRANS_TABLES mode.

Even though this fixes it I'm not convinved it is the right fix. Maybe we our output is not UTF8 to the database: https://metacpan.org/pod/CGI::Session#Influencing-the-encoding-of-the-program's-output

Also here more on this: https://stackoverflow.com/questions/31049125/perl-cgisession-mysql-thaw-utf-8-data
Comment 4 Joonas Kylmälä 2021-05-31 08:49:23 UTC
(In reply to Joonas Kylmälä from comment #0)
> 1) Set strict_sql_modes = 1 in koha-conf.xml (depending on your test
> environment variables you might also just wanna uncomment the relevant code

I typoed this, in fact the strict_sql_modes need to be set to 0 to reproduce this.
Comment 5 Joonas Kylmälä 2021-05-31 11:10:57 UTC
I found another workaround:

$datastr = decode_utf8(encode_utf8($datastr)); 

inside CGI::Session::Driver::mysql::store fixes the problem. The difference seems to be that $datastr is not marked as utf8 string. And after it is (decode_utf8 does it) the string is stored correctly to the DB.
Comment 6 Joonas Kylmälä 2021-05-31 11:30:12 UTC
Data::Dumper, which the default serializer uses, seems to produce iso-latin-1 strings: https://www.perlmonks.org/?node_id=759457 Our database connection is configured for utf8mb4 so that could be the issue here. I don't know if we can fix this from koha code other than by changing the database connection to iso-latin-1 for the sessions table. Or maybe changing sessions table to iso-latin-1 could make this work automatically?
Comment 7 Katrin Fischer 2021-05-31 11:57:29 UTC
Hm, I think we store some things like a user name in the sessions table, so not sure if changing the table to latin-1 is an option.
Comment 8 Julian Maurice 2021-05-31 13:29:58 UTC
The weird thing is that it works correctly with characters like ✓
It seems like the problem is that ä in Latin-1 is the same as the unicode code point (E4) which probably confuses some encoding auto-detection mechanism
Comment 9 Joonas Kylmälä 2021-05-31 13:33:03 UTC
(In reply to Julian Maurice from comment #8)
> The weird thing is that it works correctly with characters like ✓
> It seems like the problem is that ä in Latin-1 is the same as the unicode
> code point (E4) which probably confuses some encoding auto-detection
> mechanism

It only confuses the C code implementation of Data::Dumper, if we enable local $Data::Dumper::Useperl = 1; or just put in the default serializer directly the following line

>  $d->Useperl( 1 );

it starts working. I just don't know if we can make it a workable solution.
Comment 10 Julian Maurice 2021-06-02 13:54:44 UTC
If I modify sessions.a_session from longtext to longblob I no longer have the bug. Can you test if it works for you too ?
Comment 11 Julian Maurice 2021-06-02 14:18:16 UTC
Created attachment 121539 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This column contains serialized data, it should be a blob.
And it fixes encoding issues.

Test plan:
1. Apply patch and run updatedatabase
2. In $KOHA_CONF make sure that you have
   <strict_sql_modes>0</strict_sql_modes>
3. Create 2 libraries:
  a. TESTÄ/Testä
  b. TEST✓/Test✓
4. Go to the staff interface, change your library to Testä, then
   navigate to a few other pages, make sure there is no encoding problem
   with the library name in the top right corner
5. Do the same with Test✓
Comment 12 Peter Vashchuk 2021-06-02 15:05:45 UTC
Created attachment 121541 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This column contains serialized data, it should be a blob.
And it fixes encoding issues.

Test plan:
1. Apply patch and run updatedatabase
2. In $KOHA_CONF make sure that you have
   <strict_sql_modes>0</strict_sql_modes>
3. Create 2 libraries:
  a. TESTÄ/Testä
  b. TEST✓/Test✓
4. Go to the staff interface, change your library to Testä, then
   navigate to a few other pages, make sure there is no encoding problem
   with the library name in the top right corner
5. Do the same with Test✓

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 13 Andrii Nugged 2021-06-02 15:25:56 UTC
Yesterday I experimented with the same – I changed to "mediumblob" manually on my local test server to find a solution and it worked.

What I can say that I found that Data::Dumper when converting Perl strings scalars acts "fuzzily" (better: weirdly), and if it meets real Unicode chars it stores them in the string in \xXX perl-form, so they stored in DB properly and restored properly, for example, I created branch "Čentervälly":

it is stored in MySQL in a_session dumped in this way:

    'branchname' => "\x{10c}enterv\x{e4}lly"

so here it works and no error happens,

but when it meets only single "ä" for example "Hämeenlinna" (and maybe few other characters like that which suspect can be represented in old ASCII latin-1 or how that fuzziness fuzzies :)...) chars,
so it stores in such a way in MySQL longtext (broken):

    'branchname' => 'H?meenlinna',

because: see below hex dump of the blob, and when it saves to UTF-8-MySQL-field – it gets malformed, but yes, with blob it works:

    'branchname' => 'Hämeenlinna',

    and when I looked inside blob in HEX, I see:

    48 E4 6D 65 65 6E 6C 69 6E 6E 61   H‰meenlinna

because it stores ä as E4 and it is then get returned "in the same way" into "ä" (seems). But oh that's silvertape everywhere :).

This is just to say, that it weird but WORKS with blob, 

so – thanks Julian for the patch, and I say I support it,

(yet I'd blame Data::Dumper "fuzziness" :) )
Comment 14 David Cook 2021-06-03 02:04:17 UTC
/me still thinks we should've stuck with YAML
Comment 15 Andrii Nugged 2021-06-03 09:22:51 UTC
Julian, thanks for patch,

can you also add DB changes to kohastructure.sql as well, not just atomic update,

so we can then again Sign-off this?
Comment 16 Julian Maurice 2021-06-03 09:27:03 UTC
(In reply to Andrew Nugged from comment #15)
> Julian, thanks for patch,
> 
> can you also add DB changes to kohastructure.sql as well, not just atomic
> update,
> 
> so we can then again Sign-off this?

Ah yes I forgot that. I'll write the patch ASAP
Comment 17 Julian Maurice 2021-06-03 09:30:15 UTC
Created attachment 121563 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This patch modifies kohastructure.sql
Comment 18 Julian Maurice 2021-06-03 09:31:04 UTC
Created attachment 121564 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This column contains serialized data, it should be a blob.
And it fixes encoding issues.

Test plan:
1. Apply patch and run updatedatabase
2. In $KOHA_CONF make sure that you have
   <strict_sql_modes>0</strict_sql_modes>
3. Create 2 libraries:
  a. TESTÄ/Testä
  b. TEST✓/Test✓
4. Go to the staff interface, change your library to Testä, then
   navigate to a few other pages, make sure there is no encoding problem
   with the library name in the top right corner
5. Do the same with Test✓
Comment 19 Julian Maurice 2021-06-03 09:31:08 UTC
Created attachment 121565 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)
Comment 20 Peter Vashchuk 2021-06-03 10:26:07 UTC
Created attachment 121570 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This column contains serialized data, it should be a blob.
And it fixes encoding issues.

Test plan:
1. Apply patch and run updatedatabase
2. In $KOHA_CONF make sure that you have
   <strict_sql_modes>0</strict_sql_modes>
3. Create 2 libraries:
  a. TESTÄ/Testä
  b. TEST✓/Test✓
4. Go to the staff interface, change your library to Testä, then
   navigate to a few other pages, make sure there is no encoding problem
   with the library name in the top right corner
5. Do the same with Test✓

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 21 Peter Vashchuk 2021-06-03 10:26:11 UTC
Created attachment 121571 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 22 Andrii Nugged 2021-06-03 11:20:53 UTC
Created attachment 121584 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This column contains serialized data, it should be a blob.
And it fixes encoding issues.

Test plan:
1. Apply patch and run updatedatabase
2. In $KOHA_CONF make sure that you have
   <strict_sql_modes>0</strict_sql_modes>
3. Create 2 libraries:
  a. TESTÄ/Testä
  b. TEST✓/Test✓
4. Go to the staff interface, change your library to Testä, then
   navigate to a few other pages, make sure there is no encoding problem
   with the library name in the top right corner
5. Do the same with Test✓

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Andrew Nugged <nugged@gmail.com>
Comment 23 Andrii Nugged 2021-06-03 11:20:58 UTC
Created attachment 121585 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Andrew Nugged <nugged@gmail.com>
Comment 24 Victor Grousset/tuxayo 2021-06-03 13:59:55 UTC
Futher tests.

== Master ==
1.
Try to hold from the OPAC with delivery to TESTÄ/Testä or TEST✓/Test✓
=> ERROR: Internal error: incomplete hold request.
(odd, nothing in all the logs, just the HTTP request returning 200)

2.
Staff interface
set library: TESTÄ/Testä
transfer item to any another library
=> Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88
Oddly it works with TEST✓/Test✓

3.
Staff interface
set library: TESTÄ/Testä
checkout an item
=> Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88
Oddly it works with TEST✓/Test✓

4.
Staff interface
set library: NOT TESTÄ/Testä
checkout an item
set library: TESTÄ/Testä
partron page => checkout tab => show checkouts
try to check in
=> it's loads for ever
=> logs: Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88
refresh page
The item has actually been checked in. (not sure if DB is in a consistent state)

5.
Same but use the check in tab of the search box, next to the Koha logo

==

Does anyone know why TEST✓/Test✓ doesn't cause issue? At least it should cause as much issues as TESTÄ/Testä.


== With patches ==
1. fails!
2. passes
3. fails!
4. passes
5. fails! (odd)

So, QA fails!
Comment 25 Victor Grousset/tuxayo 2021-06-03 14:01:23 UTC
Does anyone have more ideas of tests?
Comment 26 Julian Maurice 2021-06-03 14:07:12 UTC
(In reply to Victor Grousset/tuxayo from comment #24)
> == With patches ==
> 1. fails!
> 2. passes
> 3. fails!
> 4. passes
> 5. fails! (odd)
> 
> So, QA fails!

Looks like it works better with the patch, so why do you say that ?
Comment 27 Julian Maurice 2021-06-03 14:08:55 UTC
(In reply to Victor Grousset/tuxayo from comment #24)
> Does anyone know why TEST✓/Test✓ doesn't cause issue? At least it should
> cause as much issues as TESTÄ/Testä.
see comment 8 and comment 13
Comment 28 Jonathan Druart 2021-06-03 14:09:02 UTC
Cannot recreate 1, but 3:

- New library created TESTÄ/TESTÄ
- Use it for logged in library
- Checkout

Invalid value passed, branchcode=\xC4 expected type is string at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 29 Victor Grousset/tuxayo 2021-06-03 14:38:49 UTC
comment 28
> Cannot recreate 1

Ok, it needs also to have TESTÄ/Testä and TEST✓/Test✓ as the library code.
The idea was {Asian, Arabic, Pakistanis, Cyrillic} writing libraries (and more) might not always transliterate. Is that actually correct?

Or is that out of scope? Are the test plan also out of scope? Assuming one or more of them still fail with ascii library code.


Comment 26
> > So, QA fails!
> Looks like it works better with the patch, so why do you say that ?

Indeed it works better.
That was based on the assumption that these tests are in the scope of this bug. Which is uncertain now.
Comment 30 Katrin Fischer 2021-06-03 14:56:02 UTC
I imagine cyrillic in library codes and library names to be a good test.
Comment 31 Jonathan Druart 2021-06-04 08:20:24 UTC
It can be recreated easily using:

use Koha::Cities;
use CGI::Session::Serialize::default;
#$Data::Dumper::Useperl = 1;
use utf8;
my $branchname = "TESTÄ/Testä";
my $hash = { branch => $branchname };
my $s = CGI::Session::Serialize::default->freeze($hash);
$hash = CGI::Session::Serialize::default->thaw($s);
Koha::City->new({city_name => $hash->{branch}})->store;


It fails with 
  Incorrect string value: '\xC4/Test...' for column 'city_name' at row 1

But inserts the city correctly if the Useperl line is uncommented.
Comment 32 Jonathan Druart 2021-06-04 08:45:18 UTC
Created attachment 121624 [details] [review]
Bug 28489: Fix issues with umlaut in session vars
Comment 33 Jonathan Druart 2021-06-04 08:45:54 UTC
This last patch seems to fix the problem, can someone test or give their opinions on it please?
Comment 34 Peter Vashchuk 2021-06-04 11:25:00 UTC Comment hidden (obsolete)
Comment 35 Peter Vashchuk 2021-06-04 12:49:34 UTC
> I imagine cyrillic in library codes and library names to be a good test.


I tested naming libraries with cyrillic symbols, like "Бібліотека" and it works 
fine with and without the patch. Then, I tested a bunch of other special 
characters from different languages, here's the list of them: 

These ones work fine without the patch:
ааа
ААА
яяя
ЯЯЯ
ііі
ІІІ
ЄЄЄ
єєє
łłł
ŁŁŁ
ŽŽŽ
ććć

Meanwhile these ones get shown as "???" without the patch:
äää
ÄÄÄ
ööö
ÖÖÖ
ààà
ááá
æææ
ÆÆÆ
ååå
ÅÅÅ
ôôô
ÔÔÔ
üüü
ÜÜÜ
ëëë
ËËË
ççç

meanwhile with patch all these symbols are displayed correctly.
Comment 36 Katrin Fischer 2021-06-04 12:53:56 UTC
Ok, so with the patch all is good...?
Comment 37 Peter Vashchuk 2021-06-04 13:50:24 UTC
(In reply to Victor Grousset/tuxayo from comment #24)
> Further tests.
> == Master ==
...
> == With patches ==
...

Hey, I couldn't reproduce it, I didn't manage to get any errors like that neither on master, nor with two previous patches that were signed off by me previously. Everything seems to magically work on my local koha so I am not sure what the problem is.
Comment 38 Jonathan Druart 2021-06-04 14:08:12 UTC
Wait a bit, Julian and me are trying to come with a better solution. My last patch is removing the different problems listed previously, but it's 1. ugly and 2. incorrect (we only fix the problem in set_userenv whereas it must be fixed at session level).
Comment 39 Andrii Nugged 2021-06-04 14:11:15 UTC
I also don't like direct utf8 flag change on lower level: later it can bring more errors even if we manage for now them not to happen :P.

But I also tried with Victor's findings and can't: Jonathan/Julian – have you able to reproduce what Victor said? Hint me how, for me it just worked (comment 24).
Comment 40 Jonathan Druart 2021-06-04 15:47:36 UTC
Julian, Andrew and me worked on it today but we have not found a correct solution so far.


The only viable solution would be:
1. For master: Replace CGI::Session with Data::Session using JSON serialization (see bug 17427 and note that there is still a problem there)
2. For 21.05: Reintroduce the YAML dependency and use YAML serialization with CGI::Session (we know it is a working solution and we are failing to provide a fix with Data::Dumper)
3. Developers will test Data::Session in master
4. Eventually backport the Data::Session move into 21.05

Note that the proposed solution on bug 17427 does not require a datatype change (sessions.a_session is still longtext).
Comment 41 Jonathan Druart 2021-06-04 16:37:14 UTC
*** Bug 28517 has been marked as a duplicate of this bug. ***
Comment 42 Andrii Nugged 2021-06-04 19:43:02 UTC Comment hidden (obsolete)
Comment 43 Andrii Nugged 2021-06-04 19:45:23 UTC
Thought on swimming 4K, thought on walking.
Such idea: what if we will create own

    CGI::Session::Serialize::yamlxs

and we're good to go then with all for now?

Proof of concept:
https://github.com/nugged/Koha/commit/2c62d311fba86576321337f2596283300013dd47
in:
https://github.com/nugged/Koha/commits/cgi_session_serialize_yamlxs

I there created "CGI::Session::Serialize::yamlxs" package just "inside" of C4::Auth file, to show that solution.

Jonathan and David: and voila, we might have YAML::XS now for sessions!
Comment 44 Jonathan Druart 2021-06-05 12:51:09 UTC
Created attachment 121637 [details] [review]
Bug 28489: Add selenium tests
Comment 45 Jonathan Druart 2021-06-05 12:53:25 UTC
Andrew,

Yes, that could work as well. I forgot this possibility!

I've written some selenium tests to help us testing the different patches/approach. I am running into the same problem I faced with Data::Session and using the file driver:

  YAML::XS::Load Error: The problem:
    control characters are not allowed
  was found at document: 0
Comment 46 Jonathan Druart 2021-06-05 13:14:46 UTC
Created attachment 121638 [details] [review]
Bug 28489: Add selenium tests
Comment 47 Jonathan Druart 2021-06-05 13:16:28 UTC
And those tests are passing on 20.11.x
Comment 48 Jonathan Druart 2021-06-05 13:29:09 UTC
Created attachment 121639 [details] [review]
Bug 28489: POD - CGI::Session::Serialize::yamlxs for CGI::Session
Comment 49 Jonathan Druart 2021-06-05 13:29:14 UTC
Created attachment 121640 [details] [review]
Bug 28489: Don't deal with encoding during the serialization
Comment 50 Jonathan Druart 2021-06-05 13:31:15 UTC
Looks like there is a 100% working solution using longblob and removing the encode/decode from the YAML::XS serializer.

Asking for a SO now, but we will need to provide a better solution to embed the serializer code (having it in C4::Auth is not nice).
Comment 51 Andrii Nugged 2021-06-05 19:35:23 UTC
We discussed with Jonathan about "where better to place the file" but we both agreed (JD, you say) so for a moment we can keep the package in C4/Atuh as additional one for quick-est fix so we can provide working solution for backporting,

but the question still open to decide for later (let's create a separate ticket after we accept this for not to forget, like TODO?): of course it's better to keep CGI::Session::Serialize::yamlxs in own file/path,
    .../CGI/Session/Serialize/yamlxs.pm
but that pollutes Koha lib folder - Jonathan said he keeps in mind to create something separate, but that for later discussion.

So, mates, for now – let's decide do we accept this 'yamlxs' serializer solution as a temporary but the full fix for this problem and backport,

and keep development in master later towards whatever planned (JD said target is Data::Session).
Comment 52 Andrii Nugged 2021-06-05 19:36:27 UTC Comment hidden (obsolete)
Comment 53 Andrii Nugged 2021-06-05 19:56:47 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2021-06-05 20:19:52 UTC
It works for me with longblob (so with all the patches attached on this bug report).
Comment 55 Andrii Nugged 2021-06-05 20:45:37 UTC
ah, right. Longblob, not longtext,
my bad then,

then yes, it should work.
Comment 56 David Cook 2021-06-06 23:19:41 UTC
(In reply to Andrew Nugged from comment #51)
> We discussed with Jonathan about "where better to place the file" but we
> both agreed (JD, you say) so for a moment we can keep the package in C4/Atuh
> as additional one for quick-est fix so we can provide working solution for
> backporting,
> 
> but the question still open to decide for later (let's create a separate
> ticket after we accept this for not to forget, like TODO?): of course it's
> better to keep CGI::Session::Serialize::yamlxs in own file/path,
>     .../CGI/Session/Serialize/yamlxs.pm
> but that pollutes Koha lib folder - Jonathan said he keeps in mind to create
> something separate, but that for later discussion.
 
Please no. This is how we get into trouble in Koha. We don't need more technical debt. 

The quickest fix is to revert Bug 28317, and to do this right. But I'll look at putting together a patch so that we can avoid this horror.
Comment 59 David Cook 2021-06-07 01:03:33 UTC
Created attachment 121648 [details] [review]
Bug 28489: Put CGI::Session::Serialize::yamlxs in lib directory

This patch adds a "lib" directory to the source tree which gets
mapped to the same directory as "C4" and "Koha" for single and
standard installations.

CGI::Session::Serialize::yamlxs is put into this "lib" directory.

This patch also includes some changes so that dev/git installations
work as well.
Comment 60 David Cook 2021-06-07 01:04:03 UTC
Dev test plan:

1. vi /etc/apache2/sites-enabled/kohadev.conf
2. Change "SetEnv PERL5LIB "/kohadevbox/koha" to SetEnv PERL5LIB "/kohadevbox/koha:/kohadevbox/koha/lib"
2b. service apache2 reload
3. cp debian/scripts/koha-functions.sh /usr/share/koha/bin/koha-functions.sh
4. koha-plack --restart kohadev
5. Log into http://localhost:8081/cgi-bin/koha/mainpage.pl
6. Go to CGI script http://localhost:8081/cgi-bin/koha/tools/export.pl 
(Note that this is still run in CGI even when using Plack)

NOTE: We'll need to update koha-gitify (https://gitlab.com/koha-community/koha-gitify) to get that Apache configuration. There's no harm in updating it, since this is a backwards compatible change. See https://gitlab.com/koha-community/koha-gitify/-/merge_requests/2. 

--

Deploy test plan:

Dev:
1. perl Makefile.PL
1a. Installation mode: dev
1b. Configuration directory: /opt/koha-dev
2. make
3. ls blib/PERL_MODULE_DIR/
4. Note the following: C4  CGI  cpanfile  Koha  Koha.pm
5. make install
5b. Note the output says the following:
In order to use Koha's command-line batch jobs,
you should set the following environment variables:

export KOHA_CONF=/opt/koha-dev/etc/koha-conf.xml
export PERL5LIB=/kohadevbox/koha:/kohadevbox/koha/lib
6. vi /opt/koha-dev/etc/koha-httpd.conf
7. Note the following: SetEnv PERL5LIB "/kohadevbox/koha:/kohadevbox/koha/lib"
6. rm -rf /opt/koha-dev
7. make clean

Single:
1. perl Makefile.PL
1a. Installation mode: single
1b. Please specify the directory in which to install Koha: /opt/koha
2. make
3. ls blib/PERL_MODULE_DIR/
4. Note the following: C4  CGI  cpanfile  Koha  Koha.pm
5. make install
5b. Note the output says the following:
In order to use Koha's command-line batch jobs,
you should set the following environment variables:

export KOHA_CONF=/opt/koha/etc/koha-conf.xml
export PERL5LIB=/opt/koha/lib
6. ls /opt/koha/lib
7. Note the following: C4  CGI  cpanfile  Koha  Koha.pm
8. vi /opt/koha/etc/koha-httpd.conf
9. Note the following: SetEnv PERL5LIB "/opt/koha/lib"
10. rm -rf /opt/koha
11. make clean

Standard:
1. perl Makefile.PL
1a. accept all the defaults
2. make
3. ls blib/PERL_MODULE_DIR/
4. Note the following: C4  CGI  cpanfile  Koha  Koha.pm
5. make install
5b. Note there is a set_koha_ownership failure because of a missing "koha" user, but that's OK since this is an overly intensive process within koha-testing-docker anyway, and the install has already succeeded at this point.
6. ls /usr/share/koha/lib
7. Note the following: C4  CGI  cpanfile  Koha  Koha.pm
Comment 61 David Cook 2021-06-07 01:14:24 UTC
An alternative to my patch would be to create a "PERL_MODULE_LOCAL_DIR" target/dirmap entry/token, and we could use "local_lib" or something like that. 

Another idea would be to keep PERL_MODULE_DIR as a target/dirmap, but change the token used in etc/misc files to something like "__PERL5LIB_DIRS__".

I think that would be more labour intensive though. 

That said... it looks like "backend_directory" will be broken in etc/koha-conf.xml and misc/bin/sip_run.sh will be broken with my proposed dev install atm...
Comment 62 David Cook 2021-06-07 01:15:10 UTC
I really dislike this whole session mess but... I'll move my follow-up to a separate ticket, as it is blowing out a bit...
Comment 63 David Cook 2021-06-07 02:27:40 UTC
Created attachment 121650 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This column contains serialized data, it should be a blob.
And it fixes encoding issues.

Test plan:
1. Apply patch and run updatedatabase
2. In $KOHA_CONF make sure that you have
   <strict_sql_modes>0</strict_sql_modes>
3. Create 2 libraries:
  a. TESTÄ/Testä
  b. TEST✓/Test✓
4. Go to the staff interface, change your library to Testä, then
   navigate to a few other pages, make sure there is no encoding problem
   with the library name in the top right corner
5. Do the same with Test✓

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Andrew Nugged <nugged@gmail.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 64 David Cook 2021-06-07 02:27:45 UTC
Created attachment 121651 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Andrew Nugged <nugged@gmail.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 65 David Cook 2021-06-07 02:27:49 UTC
Created attachment 121652 [details] [review]
Bug 28489: Add selenium tests

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 66 David Cook 2021-06-07 02:27:54 UTC
Created attachment 121653 [details] [review]
Bug 28489: POD - CGI::Session::Serialize::yamlxs for CGI::Session

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 67 David Cook 2021-06-07 02:27:59 UTC
Created attachment 121654 [details] [review]
Bug 28489: Don't deal with encoding during the serialization

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 68 David Cook 2021-06-07 02:30:49 UTC
I don't like the hack of putting CGI::Session::Serialize::yamlxs, but signing off anyway - with the assumption that we'll also push Bug 28519 before doing a stable release.
Comment 69 Victor Grousset/tuxayo 2021-06-09 19:31:50 UTC
Forget my comment 24 stuff. Just tried test 1 on a sandbox and a local master and it fails.
I will open a ticket.
Comment 70 Nick Clemens 2021-06-11 12:56:01 UTC
Created attachment 121831 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This column contains serialized data, it should be a blob.
And it fixes encoding issues.

Test plan:
1. Apply patch and run updatedatabase
2. In $KOHA_CONF make sure that you have
   <strict_sql_modes>0</strict_sql_modes>
3. Create 2 libraries:
  a. TESTÄ/Testä
  b. TEST✓/Test✓
4. Go to the staff interface, change your library to Testä, then
   navigate to a few other pages, make sure there is no encoding problem
   with the library name in the top right corner
5. Do the same with Test✓

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Andrew Nugged <nugged@gmail.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 71 Nick Clemens 2021-06-11 12:56:06 UTC
Created attachment 121832 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Andrew Nugged <nugged@gmail.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 72 Nick Clemens 2021-06-11 12:56:10 UTC
Created attachment 121833 [details] [review]
Bug 28489: Add selenium tests

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 73 Nick Clemens 2021-06-11 12:56:14 UTC
Created attachment 121834 [details] [review]
Bug 28489: POD - CGI::Session::Serialize::yamlxs for CGI::Session

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 74 Nick Clemens 2021-06-11 12:56:19 UTC
Created attachment 121835 [details] [review]
Bug 28489: Don't deal with encoding during the serialization

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Nick Clemens 2021-06-11 13:01:23 UTC
Note: The libraries added in the selenium tests are not removed during cleanup
Comment 76 Jonathan Druart 2021-06-11 14:05:07 UTC
Created attachment 121848 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob

This column contains serialized data, it should be a blob.
And it fixes encoding issues.

Test plan:
1. Apply patch and run updatedatabase
2. In $KOHA_CONF make sure that you have
   <strict_sql_modes>0</strict_sql_modes>
3. Create 2 libraries:
  a. TESTÄ/Testä
  b. TEST✓/Test✓
4. Go to the staff interface, change your library to Testä, then
   navigate to a few other pages, make sure there is no encoding problem
   with the library name in the top right corner
5. Do the same with Test✓

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Andrew Nugged <nugged@gmail.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 77 Jonathan Druart 2021-06-11 14:05:13 UTC
Created attachment 121849 [details] [review]
Bug 28489: Modify sessions.a_session from longtext to longblob (kohastructure.sql)

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Signed-off-by: Andrew Nugged <nugged@gmail.com>

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 78 Jonathan Druart 2021-06-11 14:05:18 UTC
Created attachment 121850 [details] [review]
Bug 28489: Add selenium tests

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

JD amended patch: Remove libraries the tests created
Comment 79 Jonathan Druart 2021-06-11 14:05:23 UTC
Created attachment 121851 [details] [review]
Bug 28489: POD - CGI::Session::Serialize::yamlxs for CGI::Session

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 80 Jonathan Druart 2021-06-11 14:05:28 UTC
Created attachment 121852 [details] [review]
Bug 28489: Don't deal with encoding during the serialization

Signed-off-by: David Cook <dcook@prosentient.com.au>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 81 Victor Grousset/tuxayo 2021-06-13 19:12:08 UTC
(In reply to Victor Grousset/tuxayo from comment #69)
> Forget my comment 24 stuff. Just tried test 1 on a sandbox and a local
> master and it fails.
> I will open a ticket.

bug 28560

That's still a mystery that here I was to only one to trigger it. The same mystery should happen in the above bug.
Comment 82 Jonathan Druart 2021-06-14 15:34:33 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 83 Kyle M Hall 2021-06-18 11:35:54 UTC
Pushed to 21.05.x for 21.05.01
Comment 84 Fridolin Somers 2021-06-21 12:36:26 UTC
Depends on Bug 28317 not in 20.11.x