Bug 22824 - Replace YAML::Syck with YAML::XS
Summary: Replace YAML::Syck with YAML::XS
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 27673 27676
Blocks: 27874 28154 28230 28281 28317 29138
  Show dependency treegraph
 
Reported: 2019-05-02 06:58 UTC by Martin Renvoize
Modified: 2022-08-22 01:31 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
To bring consistency to the codebase, improve performance and reduce dependencies it was decided we should converge on a single YAML parsing library... we opted for YAML::XS.
Version(s) released in:
21.05.00


Attachments
Bug 22824: Add tests (3.20 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Replace YAML::Syck with YAML::XS in tests (1.15 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Fixing first error in tests (1.07 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Replace YAML::Syck with YAML::XS (13.31 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Fix searching.pref (2.00 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Correct on/off behaviour (5.67 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Fix yes/no vs 1/0 (131.42 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Replace wrong auto replacement (1.15 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc. (10.63 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Fix default values for YesNo (11.97 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Atomic update (1.22 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Add warning to the about page (5.63 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Remove YAML::Syck dependency (698 bytes, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Adjust tests to match final behaviours (1.60 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Fix ES reindex (1.04 KB, patch)
2021-02-11 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Add tests (3.27 KB, patch)
2021-02-11 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Add tests (3.27 KB, patch)
2021-02-11 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Replace YAML::Syck with YAML::XS in tests (1.22 KB, patch)
2021-02-11 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Fixing first error in tests (1.14 KB, patch)
2021-02-11 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Replace YAML::Syck with YAML::XS (13.38 KB, patch)
2021-02-11 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Fix searching.pref (2.07 KB, patch)
2021-02-11 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Correct on/off behaviour (5.74 KB, patch)
2021-02-11 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Fix yes/no vs 1/0 (131.49 KB, patch)
2021-02-11 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Replace wrong auto replacement (1.22 KB, patch)
2021-02-11 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc. (10.70 KB, patch)
2021-02-11 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Fix default values for YesNo (12.04 KB, patch)
2021-02-11 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Atomic update (1.29 KB, patch)
2021-02-11 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Add warning to the about page (5.70 KB, patch)
2021-02-11 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Remove YAML::Syck dependency (769 bytes, patch)
2021-02-11 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Adjust tests to match final behaviours (1.67 KB, patch)
2021-02-11 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Fix ES reindex (1.11 KB, patch)
2021-02-11 14:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22824: Adjust POD for koha-preferences (1014 bytes, patch)
2021-02-15 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Fix koha-preferences dump (834 bytes, patch)
2021-02-15 10:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Prevent YesNo to be filled with invalid values (2.15 KB, patch)
2021-02-15 10:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Prevent YesNo to be filled with invalid values (2.39 KB, patch)
2021-02-15 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Fix encoding of misc/admin/koha-preferences output (1.09 KB, patch)
2021-02-19 09:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Fix encoding of misc/admin/koha-preferences output (1.16 KB, patch)
2021-02-19 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Add tests (3.31 KB, patch)
2021-02-19 10:06 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Replace YAML::Syck with YAML::XS in tests (1.26 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Fixing first error in tests (1.18 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Replace YAML::Syck with YAML::XS (13.42 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Fix searching.pref (2.11 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Correct on/off behaviour (5.78 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Fix yes/no vs 1/0 (131.53 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Replace wrong auto replacement (1.26 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc. (10.75 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Fix default values for YesNo (12.08 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Atomic update (1.33 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Add warning to the about page (5.74 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Remove YAML::Syck dependency (813 bytes, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Adjust tests to match final behaviours (1.72 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Fix ES reindex (1.16 KB, patch)
2021-02-19 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Adjust POD for koha-preferences (1.05 KB, patch)
2021-02-19 10:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Fix koha-preferences dump (894 bytes, patch)
2021-02-19 10:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Prevent YesNo to be filled with invalid values (2.45 KB, patch)
2021-02-19 10:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Fix encoding of misc/admin/koha-preferences output (1.22 KB, patch)
2021-02-19 10:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 22824: Add tests (3.37 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Replace YAML::Syck with YAML::XS in tests (1.33 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Fixing first error in tests (1.25 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Replace YAML::Syck with YAML::XS (13.48 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Fix searching.pref (2.17 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Correct on/off behaviour (5.84 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Fix yes/no vs 1/0 (131.85 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Replace wrong auto replacement (1.32 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc. (10.81 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Fix default values for YesNo (12.15 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Atomic update (1.40 KB, patch)
2021-03-04 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Add warning to the about page (5.80 KB, patch)
2021-03-04 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Adjust tests to match final behaviours (1.78 KB, patch)
2021-03-04 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Fix ES reindex (1.22 KB, patch)
2021-03-04 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Adjust POD for koha-preferences (1.11 KB, patch)
2021-03-04 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Fix koha-preferences dump (959 bytes, patch)
2021-03-04 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Prevent YesNo to be filled with invalid values (2.52 KB, patch)
2021-03-04 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Fix encoding of misc/admin/koha-preferences output (1.28 KB, patch)
2021-03-04 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22824: Add warning to the about page (5.87 KB, patch)
2021-03-04 09:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22824: Add warning to the about page (5.94 KB, patch)
2021-03-04 10:02 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-05-02 06:58:36 UTC
We should settle on a single YAML implementation rather than requiring two. I would advocate YAML::XS as it's the defacto recommended module by the perl community at the moment (and has been for some time now) and it's available packaged on debian back to our oldest supported release at the time of this comment.
Comment 1 Jonathan Druart 2021-02-11 14:11:06 UTC
Created attachment 116722 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently
Comment 2 Jonathan Druart 2021-02-11 14:11:09 UTC
Created attachment 116723 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS in tests

In order to understand how things will work after the changes
Comment 3 Jonathan Druart 2021-02-11 14:11:11 UTC
Created attachment 116724 [details] [review]
Bug 22824: Fixing first error in tests

There was an encoding issue. The parent bug report have been fixed, see
bug 27673 and patches related to Encode
Comment 4 Jonathan Druart 2021-02-11 14:11:14 UTC
Created attachment 116725 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS
Comment 5 Jonathan Druart 2021-02-11 14:11:17 UTC
Created attachment 116726 [details] [review]
Bug 22824: Fix searching.pref

One .pref file make YAML::XS fails.
The correct display must be, before and after this patchset:
 If "Unescape escaped" is selected this will allow writing regular expressions "\/like this\/" while "/this/", "or/this" will be escaped and interpreted as regular strings.
Comment 6 Jonathan Druart 2021-02-11 14:11:20 UTC
Created attachment 116727 [details] [review]
Bug 22824: Correct on/off behaviour

Here things are starting to be a bit tricky.
IMO we don't want to replicate the ImplicitTyping behaviour from
YAML::Syck but handle correctly the different value.
We have the list of available values in the .pref file, the values in DB
must match them.

We want 1 or 0 in DB.

2 more occurrences:
finesMode => Dealt on bug 27676 ('off' is ok here)
itemBarcodeInputFilter => 'OFF' is correct here

Note that we don't use ~ or true/false in *.pref
Comment 7 Jonathan Druart 2021-02-11 14:11:24 UTC
Created attachment 116728 [details] [review]
Bug 22824: Fix yes/no vs 1/0

Patch generated with:
 perl -p -i -e 's# no: # 0: #' **/*.pref
 perl -p -i -e 's# yes: # 1: #' **/*.pref

no must be 0 and yes must be 1!
Comment 8 Jonathan Druart 2021-02-11 14:11:27 UTC
Created attachment 116729 [details] [review]
Bug 22824: Replace wrong auto replacement

This must be "no", it has been caught by the previous sed
Comment 9 Jonathan Druart 2021-02-11 14:11:30 UTC
Created attachment 116730 [details] [review]
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc.

It seems that we don't really need all this overhead.
YesNo must be a boolean and contain 1 or 0.
Comment 10 Jonathan Druart 2021-02-11 14:11:33 UTC
Created attachment 116731 [details] [review]
Bug 22824: Fix default values for YesNo

select variable, value from systempreferences where type="YesNo" and ( ( value <>'1' and value <>'0' ) or value is null );
Comment 11 Jonathan Druart 2021-02-11 14:11:36 UTC
Created attachment 116732 [details] [review]
Bug 22824: Atomic update
Comment 12 Jonathan Druart 2021-02-11 14:11:39 UTC
Created attachment 116733 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.
Comment 13 Jonathan Druart 2021-02-11 14:11:43 UTC
Created attachment 116734 [details] [review]
Bug 22824: Remove YAML::Syck dependency
Comment 14 Jonathan Druart 2021-02-11 14:11:46 UTC
Created attachment 116735 [details] [review]
Bug 22824: Adjust tests to match final behaviours

Finally we diverge quite a lot with how it worked prior to this patch
set. Reflect the changes in the tests
Comment 15 Jonathan Druart 2021-02-11 14:11:50 UTC
Created attachment 116736 [details] [review]
Bug 22824: Fix ES reindex

koha-elastic --rebuild was failing with
Unable to update mappings for index "koha_kohadev_biblios". Reason was: "Could not convert [marc_data.index] to boolean"
Comment 16 Jonathan Druart 2021-02-11 14:14:05 UTC
I am sorry, I lost myself!

This was supposed to be a quite trivial patch and it ended up huge!

I think the move is interesting and going into the right direction.
But I have the feeling that it's not finished and some tricky checks need to be done on the syspref table/.pref.

This patch set is dealing with the yes/no, on/off weird options we have, historically I bet, in our table and .pref files. In this patch we are removing all of that and expect a boolean to be 1 or 0 (wow!), and it removes the C4::Boolean weirdness.

So far the prefs, installer and translate scripts seem to be working correctly. The edge cases may be hard to catch!

Don't be afraid by the number of patches, they are all very trivial!

What do you think?
Comment 17 Kyle M Hall 2021-02-11 14:46:02 UTC
Created attachment 116741 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2021-02-11 14:46:32 UTC
Created attachment 116742 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2021-02-11 14:46:40 UTC
Created attachment 116743 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS in tests

In order to understand how things will work after the changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2021-02-11 14:46:43 UTC
Created attachment 116744 [details] [review]
Bug 22824: Fixing first error in tests

There was an encoding issue. The parent bug report have been fixed, see
bug 27673 and patches related to Encode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2021-02-11 14:46:47 UTC
Created attachment 116745 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2021-02-11 14:46:50 UTC
Created attachment 116746 [details] [review]
Bug 22824: Fix searching.pref

One .pref file make YAML::XS fails.
The correct display must be, before and after this patchset:
 If "Unescape escaped" is selected this will allow writing regular expressions "\/like this\/" while "/this/", "or/this" will be escaped and interpreted as regular strings.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2021-02-11 14:46:53 UTC
Created attachment 116747 [details] [review]
Bug 22824: Correct on/off behaviour

Here things are starting to be a bit tricky.
IMO we don't want to replicate the ImplicitTyping behaviour from
YAML::Syck but handle correctly the different value.
We have the list of available values in the .pref file, the values in DB
must match them.

We want 1 or 0 in DB.

2 more occurrences:
finesMode => Dealt on bug 27676 ('off' is ok here)
itemBarcodeInputFilter => 'OFF' is correct here

Note that we don't use ~ or true/false in *.pref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2021-02-11 14:46:58 UTC
Created attachment 116748 [details] [review]
Bug 22824: Fix yes/no vs 1/0

Patch generated with:
 perl -p -i -e 's# no: # 0: #' **/*.pref
 perl -p -i -e 's# yes: # 1: #' **/*.pref

no must be 0 and yes must be 1!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2021-02-11 14:47:01 UTC
Created attachment 116749 [details] [review]
Bug 22824: Replace wrong auto replacement

This must be "no", it has been caught by the previous sed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2021-02-11 14:47:05 UTC
Created attachment 116750 [details] [review]
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc.

It seems that we don't really need all this overhead.
YesNo must be a boolean and contain 1 or 0.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2021-02-11 14:47:08 UTC
Created attachment 116751 [details] [review]
Bug 22824: Fix default values for YesNo

select variable, value from systempreferences where type="YesNo" and ( ( value <>'1' and value <>'0' ) or value is null );

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2021-02-11 14:47:11 UTC
Created attachment 116752 [details] [review]
Bug 22824: Atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2021-02-11 14:47:14 UTC
Created attachment 116753 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2021-02-11 14:47:17 UTC
Created attachment 116754 [details] [review]
Bug 22824: Remove YAML::Syck dependency

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2021-02-11 14:47:20 UTC
Created attachment 116755 [details] [review]
Bug 22824: Adjust tests to match final behaviours

Finally we diverge quite a lot with how it worked prior to this patch
set. Reflect the changes in the tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2021-02-11 14:47:23 UTC
Created attachment 116756 [details] [review]
Bug 22824: Fix ES reindex

koha-elastic --rebuild was failing with
Unable to update mappings for index "koha_kohadev_biblios". Reason was: "Could not convert [marc_data.index] to boolean"

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall 2021-02-11 14:48:26 UTC
(In reply to Jonathan Druart from comment #16)
> What do you think?

Looks great to me! We've plenty of time before release in case there are any more edge cases that need catching, but I can't think of any!
Comment 34 Martin Renvoize 2021-02-12 09:46:39 UTC
(In reply to Kyle M Hall from comment #33)
> (In reply to Jonathan Druart from comment #16)
> > What do you think?
> 
> Looks great to me! We've plenty of time before release in case there are any
> more edge cases that need catching, but I can't think of any!

Indeed.. I've been down this path once before with another app.. I think this looks like a really solid start and covers all the cases that caused me issues in my last experience.

I'll happily kick the wheels with what we've got with a view to QAing; I'm confident I won't find every issue, but feel we're still early enough in the cycle to push if we push soon and sort out any fallout... I think devs' should quickly spot potential issues once it's in master.
Comment 35 Jonathan Druart 2021-02-12 10:08:47 UTC
misc/admin/koha-preferences
-$YAML::Syck::SortKeys = 1;

=> This line needs attention.
Comment 36 Julian Maurice 2021-02-12 15:55:11 UTC
There are some issues in misc/admin/koha-preferences:
- It now allows to set any value for boolean (YesNo) preferences. In master there is a warning message and value is set to 0 when an invalid value is given
- documentation probably needs an update ("If the syspref is YesNo, will accept only a boolean value, but the syntax for these is fairly lax (yes/no, on/off, 1/0, n/y, true/false are all accepted).")
- 'dump' subcommand does not work, it just prints 'HASH(0x12345)'.
Comment 37 Jonathan Druart 2021-02-15 10:10:26 UTC
Created attachment 116881 [details] [review]
Bug 22824: Adjust POD for koha-preferences
Comment 38 Jonathan Druart 2021-02-15 10:10:30 UTC
Created attachment 116882 [details] [review]
Bug 22824: Fix koha-preferences dump
Comment 39 Jonathan Druart 2021-02-15 10:10:56 UTC
(In reply to Julian Maurice from comment #36)
> There are some issues in misc/admin/koha-preferences:
> - It now allows to set any value for boolean (YesNo) preferences. In master
> there is a warning message and value is set to 0 when an invalid value is
> given

I think we are good with the warning I added on the about page, don't you think?
I don't really want to reintroduce the C4::Boolean module (or equivalent code) only for this script.

> - documentation probably needs an update ("If the syspref is YesNo, will
> accept only a boolean value, but the syntax for these is fairly lax (yes/no,
> on/off, 1/0, n/y, true/false are all accepted).")

Adjusted.

> - 'dump' subcommand does not work, it just prints 'HASH(0x12345)'.

Oops, fixed!
Comment 40 Julian Maurice 2021-02-15 10:14:21 UTC
(In reply to Jonathan Druart from comment #39)
> (In reply to Julian Maurice from comment #36)
> > There are some issues in misc/admin/koha-preferences:
> > - It now allows to set any value for boolean (YesNo) preferences. In master
> > there is a warning message and value is set to 0 when an invalid value is
> > given
> 
> I think we are good with the warning I added on the about page, don't you
> think?
> I don't really want to reintroduce the C4::Boolean module (or equivalent
> code) only for this script.

I think the script should prevent to set wrong value for those sysprefs. It could at least check that the value given is either "0" or "1".
Comment 41 Jonathan Druart 2021-02-15 10:38:49 UTC
Created attachment 116883 [details] [review]
Bug 22824: Prevent YesNo to be filled with invalid values

This patch prevents koha-preferences to set other values than 1/0 to be
used for YesNo prefs
Comment 42 Julian Maurice 2021-02-15 10:55:09 UTC
(In reply to Jonathan Druart from comment #41)
> Created attachment 116883 [details] [review] [review]
> Bug 22824: Prevent YesNo to be filled with invalid values
> 
> This patch prevents koha-preferences to set other values than 1/0 to be
> used for YesNo prefs

This patch does not work.
If I execute
  misc/admin/koha-preferences set viewMARC 2
it warns "System preference viewMARC is YesNo and expects 1 or 0. '2' was given, using '0' (OK)
but the value is set to '2'. (NOT OK)

And the dump subcommand output is different from master. On master it prints a serie of
  <sysprefName>: <sysprefValue>
but with the patches it prints
  <sysprefName>:
    type: <sysprefType>
    value: <sysprefValue>
    variable: <sysprefName>

which prevents to do things like:

  koha-preferences dump | koha-preferences load
Comment 43 Jonathan Druart 2021-02-15 11:16:39 UTC
Created attachment 116884 [details] [review]
Bug 22824: Prevent YesNo to be filled with invalid values

This patch prevents koha-preferences to set other values than 1/0 to be
used for YesNo prefs
Comment 44 Jonathan Druart 2021-02-15 11:17:54 UTC
(In reply to Julian Maurice from comment #42)
> (In reply to Jonathan Druart from comment #41)
> > Created attachment 116883 [details] [review] [review] [review]
> > Bug 22824: Prevent YesNo to be filled with invalid values
> > 
> > This patch prevents koha-preferences to set other values than 1/0 to be
> > used for YesNo prefs
> 
> This patch does not work.
> If I execute
>   misc/admin/koha-preferences set viewMARC 2
> it warns "System preference viewMARC is YesNo and expects 1 or 0. '2' was
> given, using '0' (OK)
> but the value is set to '2'. (NOT OK)

Right, I tested it very poorly, sorry about that.
Should work now.

> And the dump subcommand output is different from master. On master it prints
> a serie of
>   <sysprefName>: <sysprefValue>
> but with the patches it prints
>   <sysprefName>:
>     type: <sysprefType>
>     value: <sysprefValue>
>     variable: <sysprefName>
> 
> which prevents to do things like:
> 
>   koha-preferences dump | koha-preferences load

The dump|load actually worked, but not using a dump generated from master.
I restored the current behaviour.
Comment 45 Julian Maurice 2021-02-15 14:02:01 UTC
Sorry, but I think there is still a problem with dump/load

If I do
  koha-preferences dump | koha-preferences load

I get an error
  YAML::XS::Load Error: The problem:
    invalid trailing UTF-8 octet
  was found at document: 1

If I do
  koha-preferences dump > prefs && file prefs
I get
  prefs: ISO-8859 text, with very long lines

but If I do
  koha-preferences dump -o prefs && file prefs
I get
  prefs: UTF-8 Unicode text, with very long lines

If I remove the call to decode_utf8 in dump subcommand, it works (ie. I get UTF-8 Unicode in both cases, and dump|load seems to work)
Comment 46 Jonathan Druart 2021-02-15 14:35:40 UTC
Hum

$ perl misc/admin/koha-preferences dump | perl misc/admin/koha-preferences load
It works

$ perl misc/admin/koha-preferences dump > prefs && file prefs
prefs: ASCII text

indeed, but edit URLLinkText, set "✔ ❤️ ★"

$ perl misc/admin/koha-preferences dump > prefs && file prefs
prefs: UTF-8 Unicode text

$ perl misc/admin/koha-preferences dump | perl misc/admin/koha-preferences load
Still works

Edit URLLinkText, it contains the characters in the correct encoding.

Can you try adding
  binmode STDOUT, ':encoding(UTF-8)';
before Dump?
Comment 47 Jonathan Druart 2021-02-18 15:00:56 UTC
Switching back to NSO hoping to get other feedbacks.
Comment 48 Julian Maurice 2021-02-19 09:48:20 UTC
If I set "✔ ❤️ ★" to a syspref, `dump > prefs` give me a "wide-char-in-print" warning.
I think I found a fix and will attach the patch.
Comment 49 Julian Maurice 2021-02-19 09:49:50 UTC
Created attachment 117037 [details] [review]
Bug 22824: Fix encoding of misc/admin/koha-preferences output
Comment 50 Jonathan Druart 2021-02-19 10:05:31 UTC
Created attachment 117039 [details] [review]
Bug 22824: Fix encoding of misc/admin/koha-preferences output

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 51 Julian Maurice 2021-02-19 10:06:56 UTC
Created attachment 117040 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 52 Julian Maurice 2021-02-19 10:07:01 UTC
Created attachment 117041 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS in tests

In order to understand how things will work after the changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 53 Julian Maurice 2021-02-19 10:07:05 UTC
Created attachment 117042 [details] [review]
Bug 22824: Fixing first error in tests

There was an encoding issue. The parent bug report have been fixed, see
bug 27673 and patches related to Encode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 54 Julian Maurice 2021-02-19 10:07:10 UTC
Created attachment 117043 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 55 Julian Maurice 2021-02-19 10:07:14 UTC
Created attachment 117044 [details] [review]
Bug 22824: Fix searching.pref

One .pref file make YAML::XS fails.
The correct display must be, before and after this patchset:
 If "Unescape escaped" is selected this will allow writing regular expressions "\/like this\/" while "/this/", "or/this" will be escaped and interpreted as regular strings.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 56 Julian Maurice 2021-02-19 10:07:18 UTC
Created attachment 117045 [details] [review]
Bug 22824: Correct on/off behaviour

Here things are starting to be a bit tricky.
IMO we don't want to replicate the ImplicitTyping behaviour from
YAML::Syck but handle correctly the different value.
We have the list of available values in the .pref file, the values in DB
must match them.

We want 1 or 0 in DB.

2 more occurrences:
finesMode => Dealt on bug 27676 ('off' is ok here)
itemBarcodeInputFilter => 'OFF' is correct here

Note that we don't use ~ or true/false in *.pref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 57 Julian Maurice 2021-02-19 10:07:23 UTC
Created attachment 117046 [details] [review]
Bug 22824: Fix yes/no vs 1/0

Patch generated with:
 perl -p -i -e 's# no: # 0: #' **/*.pref
 perl -p -i -e 's# yes: # 1: #' **/*.pref

no must be 0 and yes must be 1!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 58 Julian Maurice 2021-02-19 10:07:27 UTC
Created attachment 117047 [details] [review]
Bug 22824: Replace wrong auto replacement

This must be "no", it has been caught by the previous sed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 59 Julian Maurice 2021-02-19 10:07:32 UTC
Created attachment 117048 [details] [review]
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc.

It seems that we don't really need all this overhead.
YesNo must be a boolean and contain 1 or 0.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 60 Julian Maurice 2021-02-19 10:07:36 UTC
Created attachment 117049 [details] [review]
Bug 22824: Fix default values for YesNo

select variable, value from systempreferences where type="YesNo" and ( ( value <>'1' and value <>'0' ) or value is null );

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 61 Julian Maurice 2021-02-19 10:07:40 UTC
Created attachment 117050 [details] [review]
Bug 22824: Atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 62 Julian Maurice 2021-02-19 10:07:45 UTC
Created attachment 117051 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 63 Julian Maurice 2021-02-19 10:07:49 UTC
Created attachment 117052 [details] [review]
Bug 22824: Remove YAML::Syck dependency

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 64 Julian Maurice 2021-02-19 10:07:54 UTC
Created attachment 117053 [details] [review]
Bug 22824: Adjust tests to match final behaviours

Finally we diverge quite a lot with how it worked prior to this patch
set. Reflect the changes in the tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 65 Julian Maurice 2021-02-19 10:07:58 UTC
Created attachment 117054 [details] [review]
Bug 22824: Fix ES reindex

koha-elastic --rebuild was failing with
Unable to update mappings for index "koha_kohadev_biblios". Reason was: "Could not convert [marc_data.index] to boolean"

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 66 Julian Maurice 2021-02-19 10:08:03 UTC
Created attachment 117055 [details] [review]
Bug 22824: Adjust POD for koha-preferences

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 67 Julian Maurice 2021-02-19 10:08:09 UTC
Created attachment 117056 [details] [review]
Bug 22824: Fix koha-preferences dump

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 68 Julian Maurice 2021-02-19 10:08:16 UTC
Created attachment 117057 [details] [review]
Bug 22824: Prevent YesNo to be filled with invalid values

This patch prevents koha-preferences to set other values than 1/0 to be
used for YesNo prefs

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 69 Julian Maurice 2021-02-19 10:08:22 UTC
Created attachment 117058 [details] [review]
Bug 22824: Fix encoding of misc/admin/koha-preferences output

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 70 Martin Renvoize 2021-03-04 09:19:16 UTC
Created attachment 117636 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2021-03-04 09:19:20 UTC
Created attachment 117637 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS in tests

In order to understand how things will work after the changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 72 Martin Renvoize 2021-03-04 09:19:24 UTC
Created attachment 117638 [details] [review]
Bug 22824: Fixing first error in tests

There was an encoding issue. The parent bug report have been fixed, see
bug 27673 and patches related to Encode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 73 Martin Renvoize 2021-03-04 09:19:28 UTC
Created attachment 117639 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 74 Martin Renvoize 2021-03-04 09:19:32 UTC
Created attachment 117640 [details] [review]
Bug 22824: Fix searching.pref

One .pref file make YAML::XS fails.
The correct display must be, before and after this patchset:
 If "Unescape escaped" is selected this will allow writing regular expressions "\/like this\/" while "/this/", "or/this" will be escaped and interpreted as regular strings.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 75 Martin Renvoize 2021-03-04 09:19:35 UTC
Created attachment 117641 [details] [review]
Bug 22824: Correct on/off behaviour

Here things are starting to be a bit tricky.
IMO we don't want to replicate the ImplicitTyping behaviour from
YAML::Syck but handle correctly the different value.
We have the list of available values in the .pref file, the values in DB
must match them.

We want 1 or 0 in DB.

2 more occurrences:
finesMode => Dealt on bug 27676 ('off' is ok here)
itemBarcodeInputFilter => 'OFF' is correct here

Note that we don't use ~ or true/false in *.pref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Martin Renvoize 2021-03-04 09:19:39 UTC
Created attachment 117642 [details] [review]
Bug 22824: Fix yes/no vs 1/0

Patch generated with:
 perl -p -i -e 's# no: # 0: #' **/*.pref
 perl -p -i -e 's# yes: # 1: #' **/*.pref

no must be 0 and yes must be 1!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Martin Renvoize 2021-03-04 09:19:43 UTC
Created attachment 117643 [details] [review]
Bug 22824: Replace wrong auto replacement

This must be "no", it has been caught by the previous sed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 78 Martin Renvoize 2021-03-04 09:19:47 UTC
Created attachment 117644 [details] [review]
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc.

It seems that we don't really need all this overhead.
YesNo must be a boolean and contain 1 or 0.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 79 Martin Renvoize 2021-03-04 09:19:52 UTC
Created attachment 117645 [details] [review]
Bug 22824: Fix default values for YesNo

select variable, value from systempreferences where type="YesNo" and ( ( value <>'1' and value <>'0' ) or value is null );

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 80 Martin Renvoize 2021-03-04 09:19:56 UTC
Created attachment 117646 [details] [review]
Bug 22824: Atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 81 Martin Renvoize 2021-03-04 09:20:00 UTC
Created attachment 117647 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 82 Martin Renvoize 2021-03-04 09:20:04 UTC
Created attachment 117648 [details] [review]
Bug 22824: Adjust tests to match final behaviours

Finally we diverge quite a lot with how it worked prior to this patch
set. Reflect the changes in the tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 83 Martin Renvoize 2021-03-04 09:20:08 UTC
Created attachment 117649 [details] [review]
Bug 22824: Fix ES reindex

koha-elastic --rebuild was failing with
Unable to update mappings for index "koha_kohadev_biblios". Reason was: "Could not convert [marc_data.index] to boolean"

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 84 Martin Renvoize 2021-03-04 09:20:11 UTC
Created attachment 117650 [details] [review]
Bug 22824: Adjust POD for koha-preferences

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 85 Martin Renvoize 2021-03-04 09:20:15 UTC
Created attachment 117651 [details] [review]
Bug 22824: Fix koha-preferences dump

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 86 Martin Renvoize 2021-03-04 09:20:19 UTC
Created attachment 117652 [details] [review]
Bug 22824: Prevent YesNo to be filled with invalid values

This patch prevents koha-preferences to set other values than 1/0 to be
used for YesNo prefs

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 87 Martin Renvoize 2021-03-04 09:20:23 UTC
Created attachment 117653 [details] [review]
Bug 22824: Fix encoding of misc/admin/koha-preferences output

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 88 Martin Renvoize 2021-03-04 09:53:57 UTC
Manual testing and I can't find any issues.. this is a great patchset and covers all the usual cases I've come across when making similar moves elsewhere.

Couple of QA script failures.. the first one has me confused:

FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/about.tt       
   FAIL   valid_template                                            
                parse error - /home/martin/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/about.tt line 210: unexpected token ( warnNoActiveCurrency)
  [% IF warnPrefBiblioAddsAuthorities || warnPrefEasyAnalyticalRecords || warnPrefAnonymousPatronOPACPrivacy || warnPrefAnonymousPatronAnonSuggestions || warnPrefAnonymousPatronOPACPrivacy_PatronDoesNotExist || warnPrefAnonymousPatronAnonSuggestions_PatronDoesNotExist || warnPrefKohaAdminEmailAddress || invalid_yesn
o.count || warnNoActiveCurrency || warnIsRootUser || xml_config_warnings.size || AutoSelfCheckPatronDoesNotHaveSelfCheckPerm || AutoSelfCheckPatronHasTooManyPerm || warnStatisticsFieldsError || warnNoTemplateCaching || warnILLConfiguration || has_ai_issues || oauth2_missing_deps || bad_yaml_prefs || warnRelationship
s || log4perl_errors || config_bcrypt_settings_no_set || warnHiddenBiblionumbers.size || warnConnectBroker %]

The second one is a false positive and can be ignored:

FAIL   t/db_dependent/yaml.t                             
   FAIL   spelling                                             
                 dont  ==> don't
Comment 89 Martin Renvoize 2021-03-04 09:55:08 UTC
Also.. I dropped a patch during rebase (the removal of YAML::Syck from cpanfile as it's already done in a followup to bug 27673)
Comment 90 Julian Maurice 2021-03-04 09:57:30 UTC
(In reply to Martin Renvoize from comment #88)
> FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/about.tt       
>    FAIL   valid_template                                            
I saw this one too. There is a space that is not really a space, probably a non-breaking space. I don't why know it breaks the tests whereas the template works fine otherwise.
Comment 91 Jonathan Druart 2021-03-04 09:58:23 UTC
Created attachment 117664 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

JD amended patch: there was a "bad space" between "warnNoActiveCurrency"
Comment 92 Jonathan Druart 2021-03-04 10:00:43 UTC
(In reply to Jonathan Druart from comment #91)
> Created attachment 117664 [details] [review] [review]
> Bug 22824: Add warning to the about page
> 
> Just in case, add a warning to the about page.
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> 
> JD amended patch: there was a "bad space" between "warnNoActiveCurrency"

^between^before^
Comment 93 Martin Renvoize 2021-03-04 10:02:42 UTC
Created attachment 117666 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

JD amended patch: there was a "bad space" between "warnNoActiveCurrency"

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 94 Martin Renvoize 2021-03-04 10:04:49 UTC
Excellent, that final patch squashed the warn in the QA script.

Going for PQA here. It wouldn't surprise me if there's something we've missed somewhere, but I don't think we're going to spot it here and now and we're better off pushing now and fixing an issue down the line during this cycle when/if we spot it.. with all the devs running this branch I'm sure it'll "come out in the wash if there is anything lurking"

Great work Jonathan, Koha's really moving forward at the moment, modernising and cleaning code.. great to see!
Comment 95 Jonathan Druart 2021-03-04 16:25:03 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 96 Hayley Pelham 2021-03-04 22:56:46 UTC
Hi all,

When I went to master in my devbox, I got this error when restarting services and my sites wouldn't load:

BEGIN failed--compilation aborted at /etc/koha/sites/kohadev/plack.psgi line 29.
Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't locate C4/Boolean.pm in @INC (you may need to install the C4::Boolean module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/kohaclone/installer /home/vagrant/kohaclone/lib/installer /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) at /etc/koha/sites/kohadev/plack.psgi line 29.

I removed the use line in /etc/koha/sites/kohadev/plack.psgi, but is there something else I should have done? And will this happen for other Kohas?

Cheers,
Hayley
Comment 97 Tomás Cohen Arazi 2021-03-04 23:01:32 UTC
(In reply to Hayley Pelham from comment #96)
> Hi all,
> 
> When I went to master in my devbox, I got this error when restarting
> services and my sites wouldn't load:
> 
> BEGIN failed--compilation aborted at /etc/koha/sites/kohadev/plack.psgi line
> 29.
> Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't locate
> C4/Boolean.pm in @INC (you may need to install the C4::Boolean module) (@INC
> contains: /home/vagrant/kohaclone /home/vagrant/kohaclone/installer
> /home/vagrant/kohaclone/lib/installer /etc/perl
> /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1
> /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5
> /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24
> /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) at
> /etc/koha/sites/kohadev/plack.psgi line 29.
> 
> I removed the use line in /etc/koha/sites/kohadev/plack.psgi, but is there
> something else I should have done? And will this happen for other Kohas?
> 
> Cheers,
> Hayley

reset_all should do the job of replacing the old plack.psgi.
Comment 98 Hayley Pelham 2021-03-04 23:54:55 UTC
(In reply to Tomás Cohen Arazi from comment #97)
> (In reply to Hayley Pelham from comment #96)
> > Hi all,
> > 
> > When I went to master in my devbox, I got this error when restarting
> > services and my sites wouldn't load:
> > 
> > BEGIN failed--compilation aborted at /etc/koha/sites/kohadev/plack.psgi line
> > 29.
> > Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't locate
> > C4/Boolean.pm in @INC (you may need to install the C4::Boolean module) (@INC
> > contains: /home/vagrant/kohaclone /home/vagrant/kohaclone/installer
> > /home/vagrant/kohaclone/lib/installer /etc/perl
> > /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1
> > /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5
> > /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24
> > /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) at
> > /etc/koha/sites/kohadev/plack.psgi line 29.
> > 
> > I removed the use line in /etc/koha/sites/kohadev/plack.psgi, but is there
> > something else I should have done? And will this happen for other Kohas?
> > 
> > Cheers,
> > Hayley
> 
> reset_all should do the job of replacing the old plack.psgi.

Ah, of course! Gracias :)
Comment 99 Fridolin Somers 2021-03-08 15:45:43 UTC
Enhancement not pushed to 20.11.x
Comment 100 Jonathan Druart 2021-10-07 09:32:27 UTC
@@ -78,14 +78,14 @@ Web services:
               choices:
                   persistent: will never be emptied or truncated (persistent)
                   transient: might be emptied or truncated at some point (transient)
-                  no: will never have any data in it (no)
+                  0: will never have any data in it (no)

Isn't this replacement wrong?