Bug 28317 - Remove CGI::Session::Serialize::yaml dependency by using the default serializer
Summary: Remove CGI::Session::Serialize::yaml dependency by using the default serializer
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 22824
Blocks: 28489
  Show dependency treegraph
 
Reported: 2021-05-11 10:38 UTC by Julian Maurice
Modified: 2021-05-31 08:17 UTC (History)
11 users (show)

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


Attachments
Bug 23817: Use the default CGI::Session serializer (2.91 KB, patch)
2021-05-11 12:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28317: Use the default CGI::Session serializer (3.65 KB, patch)
2021-05-11 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28317: Use the default CGI::Session serializer (5.07 KB, patch)
2021-05-11 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28317: Use the default CGI::Session serializer (5.13 KB, patch)
2021-05-11 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28317: Use the default CGI::Session serializer (5.17 KB, patch)
2021-05-11 13:32 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2021-05-11 10:38:07 UTC
Recently we replaced every use of YAML::Syck or YAML by YAML::XS, and removed YAML::Syck and YAML from cpanfile.
But we use CGI::Session::Serialize::yaml which does not work with YAML::XS and need either YAML::Syck or YAML

So we shoud either:
1) Reintroduce one of YAML or YAML::Syck into cpanfile, or
2) Use a different serializer, or
3) Implement in Koha a serializer that use YAML::XS (should be easy, look at https://metacpan.org/release/CGI-Session-Serialize-yaml/source/lib/CGI/Session/Serialize/yaml.pm)

Which one do you prefer ?
Comment 1 Joonas Kylmälä 2021-05-11 10:56:11 UTC
Why does it need to be listed in the cpanfile? Is it because it is needed so that it is included in the debian control file? If I run ./debian/list-deps it adds libyaml-libyaml-perl but is it by random chance (I don't know)?
Comment 2 Joonas Kylmälä 2021-05-11 10:59:59 UTC
(In reply to Joonas Kylmälä from comment #1)
> Why does it need to be listed in the cpanfile? Is it because it is needed so
> that it is included in the debian control file? If I run ./debian/list-deps
> it adds libyaml-libyaml-perl but is it by random chance (I don't know)?

just realized libyaml-libyaml-perl is the YAML::XS version... Let me run it a bit longer and see if any other packages appear (the list-deps script is slow).
Comment 3 Jonathan Druart 2021-05-11 11:04:14 UTC
Why don't we use CGI::Session::Serialize::default instead?
Comment 4 Joonas Kylmälä 2021-05-11 11:13:17 UTC
(In reply to Jonathan Druart from comment #3)
> Why don't we use CGI::Session::Serialize::default instead?

It was switched to yaml because of a bug in the default serializer: session management: use YAML::Syck for serialization (a858e8a8b895640f2). Could check if that diacritic bug is still valid.
Comment 5 Joonas Kylmälä 2021-05-11 11:28:09 UTC
I tried to use čollčá as the library name and the default serializer and the bug reported a decade a code doesn't happen anymore, so I think we could use the default serializer. Just have to come up with a upgrade plan. At least the sessions table need to be emptied but do we have update scripts for emptying memcached and temp files where the sesssions might also be stored?
Comment 6 Jonathan Druart 2021-05-11 11:30:43 UTC
I've tried as well and I didn't face any encoding issues.
Comment 7 Jonathan Druart 2021-05-11 11:34:45 UTC
(In reply to Joonas Kylmälä from comment #5)
> I tried to use čollčá as the library name and the default serializer and the
> bug reported a decade a code doesn't happen anymore, so I think we could use
> the default serializer. Just have to come up with a upgrade plan. At least
> the sessions table need to be emptied but do we have update scripts for
> emptying memcached and temp files where the sesssions might also be stored?

Why that?
Comment 8 Joonas Kylmälä 2021-05-11 11:45:33 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Joonas Kylmälä from comment #5)
> > I tried to use čollčá as the library name and the default serializer and the
> > bug reported a decade a code doesn't happen anymore, so I think we could use
> > the default serializer. Just have to come up with a upgrade plan. At least
> > the sessions table need to be emptied but do we have update scripts for
> > emptying memcached and temp files where the sesssions might also be stored?
> 
> Why that?

Maybe it is not needed after all, I thought it would somehow try to parse all the sessions and possibly break, e.g. maybe if a malicous user tries to use the session id for the old YAML session could it break something or allow unauthorized access?

Also now I found an encoding issue with the default serializer, if you use branchname and branchcode "ÄÄÄ~ÄãÃ" it breaks and shows question marks in intranet:


[WARN]    (in cleanup) CGI::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do failed: Incorrect string value: '\xC4\xC4\xC4~\xC4\xE3...' 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

This works with the YAML serializer.
Comment 9 Jonathan Druart 2021-05-11 12:21:12 UTC
Indeed, I only tried with the SessionStorage=tmp file

I managed to fix this problem if I remove the decode in set_userenv, but need to find side-effects.
Comment 10 Joonas Kylmälä 2021-05-11 12:23:37 UTC
(In reply to Jonathan Druart from comment #9)
> Indeed, I only tried with the SessionStorage=tmp file
> 
> I managed to fix this problem if I remove the decode in set_userenv, but
> need to find side-effects.

I was also investigating if this happens because of some wrong DB connection initialization in Koha::Database.pm code. Nothing interesting found yet. I got the idea from this discussion: https://stackoverflow.com/questions/10957238/incorrect-string-value-when-trying-to-insert-utf-8-into-mysql-via-jdbc
Comment 11 Jonathan Druart 2021-05-11 12:28:06 UTC
Created attachment 120835 [details] [review]
Bug 23817: Use the default CGI::Session serializer
Comment 12 Jonathan Druart 2021-05-11 12:29:18 UTC
Everything seems to work correctly with that patch.
Tested all the 3 combinaisons of SessionStorage and the following branchcode/branchname: "CPL", "ÄÄÄ~ÄãÃ", "✔️ ❤️ ★"
Comment 13 Joonas Kylmälä 2021-05-11 12:33:28 UTC
Thanks for the code Joubu. I have not checked the solution yet but if you think it is right, could you please do similar changes for C4/InstallAuth.pm?
Comment 14 Julian Maurice 2021-05-11 12:40:38 UTC
(In reply to Joonas Kylmälä from comment #13)
> Thanks for the code Joubu. I have not checked the solution yet but if you
> think it is right, could you please do similar changes for C4/InstallAuth.pm?

and remove CGI::Session::Serializer::yaml from cpanfile ? :)

The patch works great for me
Comment 15 Jonathan Druart 2021-05-11 12:54:24 UTC
Created attachment 120836 [details] [review]
Bug 28317: Use the default CGI::Session serializer

We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS.
However we need one of them for CGI::Session::Serialize::yaml
It's preferable to change the serializer and use the default one instead
of writing one based on YAML::XS (or patch the existing ::yaml that does
not seem maintained).

There was an encoding bug reported on the default serializer (see commit
a858e8a8b895640f2) but we fail to recreate it.

Test plan:
Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★"
Use the 3 options of SessionStorage and switch from one logged in
library to another.
Confirm that everything is working correctly (ie. no ending issue in the
library name at the top-right corner)
Comment 16 Jonathan Druart 2021-05-11 12:59:09 UTC
Created attachment 120837 [details] [review]
Bug 28317: Use the default CGI::Session serializer

We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS.
However we need one of them for CGI::Session::Serialize::yaml
It's preferable to change the serializer and use the default one instead
of writing one based on YAML::XS (or patch the existing ::yaml that does
not seem maintained).

There was an encoding bug reported on the default serializer (see commit
a858e8a8b895640f2) but we fail to recreate it.

Test plan:
Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★"
Use the 3 options of SessionStorage and switch from one logged in
library to another.
Confirm that everything is working correctly (ie. no ending issue in the
library name at the top-right corner)
Comment 17 Kyle M Hall 2021-05-11 13:29:25 UTC
Created attachment 120839 [details] [review]
Bug 28317: Use the default CGI::Session serializer

We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS.
However we need one of them for CGI::Session::Serialize::yaml
It's preferable to change the serializer and use the default one instead
of writing one based on YAML::XS (or patch the existing ::yaml that does
not seem maintained).

There was an encoding bug reported on the default serializer (see commit
a858e8a8b895640f2) but we fail to recreate it.

Test plan:
Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★"
Use the 3 options of SessionStorage and switch from one logged in
library to another.
Confirm that everything is working correctly (ie. no ending issue in the
library name at the top-right corner)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Julian Maurice 2021-05-11 13:32:52 UTC
Created attachment 120840 [details] [review]
Bug 28317: Use the default CGI::Session serializer

We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS.
However we need one of them for CGI::Session::Serialize::yaml
It's preferable to change the serializer and use the default one instead
of writing one based on YAML::XS (or patch the existing ::yaml that does
not seem maintained).

There was an encoding bug reported on the default serializer (see commit
a858e8a8b895640f2) but we fail to recreate it.

Test plan:
Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★"
Use the 3 options of SessionStorage and switch from one logged in
library to another.
Confirm that everything is working correctly (ie. no ending issue in the
library name at the top-right corner)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 19 Julian Maurice 2021-05-11 13:34:40 UTC
Followed the test plan + tested the install process after making sure I had no YAML or YAML::Syck installed.
No problem detected
Comment 20 David Cook 2021-05-11 23:45:42 UTC
(In reply to Jonathan Druart from comment #3)
> Why don't we use CGI::Session::Serialize::default instead?

It's a shame that we're losing the YAML serialization. 

As noted on https://metacpan.org/pod/CGI::Session::Serialize::yaml, using YAML makes it easy to share session data across different languages. 

I have third-party apps that can use Koha session data by parsing the YAML. It looks like I'll have to think of something much more complicated to deal with the change in serialization.
Comment 21 David Cook 2021-05-11 23:49:30 UTC
(In reply to Julian Maurice from comment #0)
> Recently we replaced every use of YAML::Syck or YAML by YAML::XS, and
> removed YAML::Syck and YAML from cpanfile.
> But we use CGI::Session::Serialize::yaml which does not work with YAML::XS
> and need either YAML::Syck or YAML
> 
> So we shoud either:
> 1) Reintroduce one of YAML or YAML::Syck into cpanfile, or
> 2) Use a different serializer, or
> 3) Implement in Koha a serializer that use YAML::XS (should be easy, look at
> https://metacpan.org/release/CGI-Session-Serialize-yaml/source/lib/CGI/
> Session/Serialize/yaml.pm)
> 
> Which one do you prefer ?

We could fork CGI::Session::Serialize::yaml and make a CGI::Session::Serialize::Koha.

We could also contribute a patch or raise an enhancement request to CGI::Session::Serialize::yaml, so that it can use YAML::XS. 

It looks like the module got a new maintainer last year https://rt.cpan.org/Public/Bug/Display.html?id=131715
Comment 22 David Cook 2021-05-11 23:56:03 UTC
Interestingly, Tina Muller who has taken over CGI::Session::Serialize::yaml is also the maintainer of YAML::XS. 

Ahhh CGI::Session::Serialize::yaml also has an undocumented feature. If you look at the source code, you'll see that you can set $CGI::Session::Serialize::yaml::Flavour to whatever YAML module you want that can use Dump/Load methods. (Note that you'd have to preload it yourself with 'use' or 'require'.)

So... it looks like CGI::Session::Serialize::yaml can work with YAML::XS.

Personally, it would be awesome if we could retain the use of CGI::Session::Serialize::yaml. 

I'm going to move this to "In Discussion", but if no one else cares, feel free to move back to "Passed QA".
Comment 23 David Cook 2021-05-12 00:01:30 UTC
I've opened a couple issues at https://rt.cpan.org/Dist/Display.html?Name=CGI-Session-Serialize-yaml regarding documentation and module options.
Comment 24 Julian Maurice 2021-05-12 06:43:54 UTC
(In reply to David Cook from comment #20)
> I have third-party apps that can use Koha session data by parsing the YAML.
> It looks like I'll have to think of something much more complicated to deal
> with the change in serialization.
It's not very complicated, you just have to use `CGI::Session::Serialize::default->thaw($session_data)`

> We could fork CGI::Session::Serialize::yaml and make a
> CGI::Session::Serialize::Koha.
> 
> Ahhh CGI::Session::Serialize::yaml also has an undocumented feature. If you
> look at the source code, you'll see that you can set
> $CGI::Session::Serialize::yaml::Flavour to whatever YAML module you want
> that can use Dump/Load methods. (Note that you'd have to preload it yourself
> with 'use' or 'require'.)
> 
> So... it looks like CGI::Session::Serialize::yaml can work with YAML::XS.
> 
> Personally, it would be awesome if we could retain the use of
> CGI::Session::Serialize::yaml. 
If we really want to stick with YAML, I'd prefer Koha to have its own implementation rather than relying on an undocumented "feature" (from my point of view that is not a feature, this is just messing with a module's internal variables)
Comment 25 Jonathan Druart 2021-05-12 06:45:49 UTC
David, thanks for the investigation.

All what you suggest is more work than what we have now, and I won't have more time to dedicate to this, we are close to the release.
We should open a new bug report to reintroduce yaml using YAML::XS, later.
Unless someone else has time to do it before the end of the week.
Comment 26 David Cook 2021-05-12 23:32:07 UTC
(In reply to Julian Maurice from comment #24)
> (In reply to David Cook from comment #20)
> > I have third-party apps that can use Koha session data by parsing the YAML.
> > It looks like I'll have to think of something much more complicated to deal
> > with the change in serialization.
> It's not very complicated, you just have to use
> `CGI::Session::Serialize::default->thaw($session_data)`

My third-party apps neither use Perl nor have the capacity to use Perl, so that's not an option :p. 

> > We could fork CGI::Session::Serialize::yaml and make a
> > CGI::Session::Serialize::Koha.
> > 
> > Ahhh CGI::Session::Serialize::yaml also has an undocumented feature. If you
> > look at the source code, you'll see that you can set
> > $CGI::Session::Serialize::yaml::Flavour to whatever YAML module you want
> > that can use Dump/Load methods. (Note that you'd have to preload it yourself
> > with 'use' or 'require'.)
> > 
> > So... it looks like CGI::Session::Serialize::yaml can work with YAML::XS.
> > 
> > Personally, it would be awesome if we could retain the use of
> > CGI::Session::Serialize::yaml. 
> If we really want to stick with YAML, I'd prefer Koha to have its own
> implementation rather than relying on an undocumented "feature" (from my
> point of view that is not a feature, this is just messing with a module's
> internal variables)

It's not an internal variable. It's a package variable. That's why they have scoped it that way. Based on my conversations with the CGI::Session::Serialize::yaml maintainer, we'll probably add a class method for explicitly choosing the YAML implementation.
Comment 27 David Cook 2021-05-12 23:38:25 UTC
(In reply to Jonathan Druart from comment #25)
> David, thanks for the investigation.
> 
> All what you suggest is more work than what we have now, and I won't have
> more time to dedicate to this, we are close to the release.
> We should open a new bug report to reintroduce yaml using YAML::XS, later.
> Unless someone else has time to do it before the end of the week.

Surely the least amount of work would be to just keep the YAML::Syck dependency for CGI::Session::Serialize::yaml for the time being, and then change it to YAML::XS later?

If we change it to the default serializer now, I think it would much harder to switch back to YAML later.
Comment 28 Jonathan Druart 2021-05-13 07:03:02 UTC
Then write an alternative patch and test it please.
Comment 29 Kyle M Hall 2021-05-13 10:55:00 UTC
(In reply to David Cook from comment #27)
> (In reply to Jonathan Druart from comment #25)
> > David, thanks for the investigation.
> > 
> > All what you suggest is more work than what we have now, and I won't have
> > more time to dedicate to this, we are close to the release.
> > We should open a new bug report to reintroduce yaml using YAML::XS, later.
> > Unless someone else has time to do it before the end of the week.
> 
> Surely the least amount of work would be to just keep the YAML::Syck
> dependency for CGI::Session::Serialize::yaml for the time being, and then
> change it to YAML::XS later?
> 
> If we change it to the default serializer now, I think it would much harder
> to switch back to YAML later.

Wouldn't it make more sense for your to add an API endpoint for outputting deserialized session info? That way you aren't messing with Koha's internals, which are subject to change.
Comment 30 Tomás Cohen Arazi 2021-05-13 10:57:58 UTC
(In reply to Kyle M Hall from comment #29)
> (In reply to David Cook from comment #27)
> > (In reply to Jonathan Druart from comment #25)
> > > David, thanks for the investigation.
> > > 
> > > All what you suggest is more work than what we have now, and I won't have
> > > more time to dedicate to this, we are close to the release.
> > > We should open a new bug report to reintroduce yaml using YAML::XS, later.
> > > Unless someone else has time to do it before the end of the week.
> > 
> > Surely the least amount of work would be to just keep the YAML::Syck
> > dependency for CGI::Session::Serialize::yaml for the time being, and then
> > change it to YAML::XS later?
> > 
> > If we change it to the default serializer now, I think it would much harder
> > to switch back to YAML later.
> 
> Wouldn't it make more sense for your to add an API endpoint for outputting
> deserialized session info? That way you aren't messing with Koha's
> internals, which are subject to change.

+1
Comment 31 Victor Grousset/tuxayo 2021-05-13 15:51:35 UTC
(In reply to David Cook from comment #26)
> My third-party apps neither use Perl nor have the capacity to use Perl, so
> that's not an option :p. 

Would writing a small Perl proxy(is that's the right term) work?
Comment 32 David Cook 2021-05-14 00:19:01 UTC
(In reply to Kyle M Hall from comment #29)
> Wouldn't it make more sense for your to add an API endpoint for outputting
> deserialized session info? That way you aren't messing with Koha's
> internals, which are subject to change.

I shouldn't have said third-party apps, as they're not really third-party. They're just non-Koha apps served off the same domain. 

Regarding your question, yes and no.

1. No:
Firstly, everything is subject to change. I've seen API integrations break when the API endpoint gets updated. In this case, I can see Koha changes coming, and the code can get updated. Plus, the sessions are fairly stable overall (when serialized as YAML at least). It's not a drama. 

Secondly, at the moment I can use the session without any local customizations to Koha. 

2. Yes:
If I were to use an API endpoint, it would require customization, and I probably wouldn't create an endpoint outputting session data. Instead, I'd probably make multiple API endpoints to get the specific data that I need (like effective user permissions). 

In this case, I have access to the CGISESSID cookie, so I have the session ID, and Koha is working kind of like a SSO system. If I were to use an API, I would need an endpoint that could convert a session ID to a borrowernumber. But then Koha doesn't have any authorizations/permissions that would really be appropriate for that kind of administrative process. 

And I would consider it an administrative process. As such, I wouldn't love the idea of it being part of Koha's public Internet-accessible API. I would want to lock down that API endpoint within a separate Koha Admin API which could be limited to specific IP addresses or a local network. 

Koha also doesn't have a concept of "system users" that are reserved just for API user (and which can't be deleted by library staff). 

So... while using an API might make sense... it would be a tonne of work. Much more work than just reading the session from the database. 

--

At the end of the day, I imagine we'll switch the serializer to default, and then I'll have to think up some kind of hacked together API solution. Or scrap the other apps. 

Koha really needs an administrator level above superlibrarian, or some other management interface. But it all takes considerable work.
Comment 33 David Cook 2021-05-14 00:21:20 UTC
(In reply to Julian Maurice from comment #24)
> If we really want to stick with YAML, I'd prefer Koha to have its own
> implementation rather than relying on an undocumented "feature" (from my
> point of view that is not a feature, this is just messing with a module's
> internal variables)

(In reply to Jonathan Druart from comment #28)
> Then write an alternative patch and test it please.

Ok. I figured Julian had already disagreed with my proposal, but I can do it anyway.
Comment 34 David Cook 2021-05-14 00:34:37 UTC
(In reply to David Cook from comment #33)
> (In reply to Julian Maurice from comment #24)
> > If we really want to stick with YAML, I'd prefer Koha to have its own
> > implementation rather than relying on an undocumented "feature" (from my
> > point of view that is not a feature, this is just messing with a module's
> > internal variables)
> 
> (In reply to Jonathan Druart from comment #28)
> > Then write an alternative patch and test it please.
> 
> Ok. I figured Julian had already disagreed with my proposal, but I can do it
> anyway.

Ah, alas, due to the Debian packaging of https://packages.debian.org/buster/libcgi-session-serialize-yaml-perl, you cannot remove libyaml-perl and libyaml-syck-perl without removing libcgi-session-serialize-yaml-perl.

I'm still not sure why we have to remove YAML or YAML::Syck. I understand that it would be annoying to have one of the modules around just for handling the session, but it would be nice to keep the session serialized as YAML.
Comment 35 David Cook 2021-05-14 01:37:30 UTC
Well, I just looked at CGI::Session::Serialize::default and did a little test using its freeze method. The data ends up looking like this:

$D = {'firstname' => 'David','surname' => 'Cook','id' => '1'};;$D

I could probably parse that...
Comment 36 David Cook 2021-05-14 02:02:21 UTC Comment hidden (obsolete)
Comment 37 David Cook 2021-05-14 02:02:27 UTC
Interestingly, CGI::Session::Serialize::default writes out a Perl data structure and then uses the equivalent of eval() to inject it back into Perl: https://metacpan.org/release/CGI-Session/source/lib/CGI/Session/Serialize/default.pm#L39.
Comment 38 David Cook 2021-05-18 03:07:51 UTC
If we're set on removing the YAML and YAML::Syck dependencies, then I say we move this back to "Passed QA". While in theory we could use YAML::XS with CGI::Session::Serialize::yaml, the Debian packaging of it means we would have to keep either YAML or YAML::Syck installed on the system, even if it was unused.
Comment 39 Jonathan Druart 2021-05-19 13:08:22 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 40 Fridolin Somers 2021-05-20 12:48:17 UTC
Depends on Bug 22824 not in 20.11.x
Comment 41 Joonas Kylmälä 2021-05-31 05:21:09 UTC
On Koha-testing-docker I get randomly the following error when trying to select a new branch on intranet (/cgi-bin/koha/circ/set-library.pl):

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

The branch I try to select has the following name: "Alkuperäiskappaleet, ei lainata".
Comment 42 Joonas Kylmälä 2021-05-31 05:25:00 UTC
(In reply to Joonas Kylmälä from comment #41)
> On Koha-testing-docker I get randomly the following error when trying to
> select a new branch on intranet (/cgi-bin/koha/circ/set-library.pl):
> 
> 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
> 
> The branch I try to select has the following name: "Alkuperäiskappaleet, ei
> lainata".

Aha, I am able to reproduce the error now always: switch first to "Alkuperäiskappaleet, ei lainata" branch and then try it to set it again and you will get this error.
Comment 43 Joonas Kylmälä 2021-05-31 08:17:55 UTC
(In reply to Joonas Kylmälä from comment #41)
> On Koha-testing-docker I get randomly the following error when trying to
> select a new branch on intranet (/cgi-bin/koha/circ/set-library.pl):
> 
> 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
> 
> The branch I try to select has the following name: "Alkuperäiskappaleet, ei
> lainata".

I made another report now as bug 28489, I think these two are related. This doesn't happen when strict_sql_modes = 0, only with strict_sql_modes = 1