I propose to add a way to retrieve the decoded YAML from (YAML) system preferences. This way we don't need to write code in vain for decoding them in-place. Also, I propose we move pipe-separated sysprefs into YAML lists. This raised on reviewing 22996.
Created attachment 92690 [details] [review] Bug 23580: Unit tests
Created attachment 92691 [details] [review] Bug 23580: Add C4::Context->yaml_preference This patch adds a new methods to be used on YAML-based sysprefs. The idea is that we would use it instead of handling the YAML decoding in-place everywhere we use YAML syprefs. Ideally, we could even move pipe separated values sysprefs into YAML, etc. To test: - Apply this patches - Run: $ kshell k$ prove t/Context.t => SUCCESS: Tests pass! - Sign off :-D
Created attachment 92696 [details] [review] Bug 23580: Unit tests Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 92697 [details] [review] Bug 23580: Add C4::Context->yaml_preference This patch adds a new methods to be used on YAML-based sysprefs. The idea is that we would use it instead of handling the YAML decoding in-place everywhere we use YAML syprefs. Ideally, we could even move pipe separated values sysprefs into YAML, etc. To test: - Apply this patches - Run: $ kshell k$ prove t/Context.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Picked for QA
Created attachment 92707 [details] [review] Bug 23580: Unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 92708 [details] [review] Bug 23580: Add C4::Context->yaml_preference This patch adds a new methods to be used on YAML-based sysprefs. The idea is that we would use it instead of handling the YAML decoding in-place everywhere we use YAML syprefs. Ideally, we could even move pipe separated values sysprefs into YAML, etc. To test: - Apply this patches - Run: $ kshell k$ prove t/Context.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
One point of attention: C4/Circulation.pm: my $update_loc_rules = get_yaml_pref_hash('UpdateItemLocationOnCheckin'); C4/Circulation.pm: my $denyingrules = Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash(); This routine comes from Koha/Util/SystemPreferences.pm. It is parsing (probably) a small subset of YAML constructions, and is meant to always return a hashref. Surely, we could better use YAML to parse and expect both arrayrefs and hashrefs. So, I would recommend to remove get_yaml_pref_hash now too.
(In reply to Marcel de Rooy from comment #8) > One point of attention: > C4/Circulation.pm: my $update_loc_rules = > get_yaml_pref_hash('UpdateItemLocationOnCheckin'); > C4/Circulation.pm: my $denyingrules = > Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash(); > > This routine comes from Koha/Util/SystemPreferences.pm. > It is parsing (probably) a small subset of YAML constructions, and is meant > to always return a hashref. Surely, we could better use YAML to parse and > expect both arrayrefs and hashrefs. > > So, I would recommend to remove get_yaml_pref_hash now too. Excellent point. I personally mistrust *Utils libraries in general :-D I understand the use of course. I agree the implementation on this bugis far more flexible, and should be preferred. One thing I fancied about this was: shouldn't we add a new type of syspref or column so we can make ->preference actually return the decoded syspref if required?
(In reply to Tomás Cohen Arazi from comment #9) > One thing I fancied about this was: shouldn't we add a new type of syspref > or column so we can make ->preference actually return the decoded syspref if > required? I like the idea, although the need is not so high for now.
Nice work! Pushed to master for 19.11.00
Enhancement not pushed to 19.05.x