| Summary: | Add C4::Context->yaml_preference | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Component: | Architecture, internals, and plumbing | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
| Severity: | enhancement | ||
| Priority: | P5 - low | CC: | fridolin.somers, m.de.rooy, martin.renvoize, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22996 | ||
| Change sponsored?: | --- | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: |
This trivial patch adds a convenient way to retrieve YAML-based system preferences in the code, avoiding the need to handle the decoding in each place they are used.
|
Version(s) released in: |
19.11.00
|
| Circulation function: | |||
| Bug Depends on: | |||
| Bug Blocks: | 33431 | ||
| Attachments: |
Bug 23580: Unit tests
Bug 23580: Add C4::Context->yaml_preference Bug 23580: Unit tests Bug 23580: Add C4::Context->yaml_preference Bug 23580: Unit tests Bug 23580: Add C4::Context->yaml_preference |
||
|
Description
Tomás Cohen Arazi (tcohen)
2019-09-10 14:40:24 UTC
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 |