[Bug 32478] New: Remove Koha::Config::SysPref->find since bypasses cache
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Bug ID: 32478 Summary: Remove Koha::Config::SysPref->find since bypasses cache Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: glasklas@gmail.com QA Contact: testopia@bugs.koha-community.org Using Koha::Config::SysPrefs->find('UpdateItemLocationOnCheckin')->get_yaml_pref_hash() to retrieve yaml sysprefs will bypass caching negating the very minor performance gain per call get_yaml_pref_hash was meant to achieve since it in some cases is being called repeatedly a very large number of times. Replacing it with C4::Context->yaml_preference('ItemsDeniedRenewal') will utilize the syspref cache and also not support a very small (not even valid) subset of YAML. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #1 from David Gustafsson <glasklas@gmail.com> --- Created attachment 144633 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144633&action=edit Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache get_yaml_pref_hash also allows invalid YAML and only parses a limited subset so remove this method to avoid future issues. To test): Since tests already exists for C4::Context->yaml_preference and this is a trivial change, do we really need a test plan for this? Sponsored-by: Gothenburg University Library -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=31735 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |glasklas@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |15494 CC| |jonathan.druart+koha@gmail. | |com --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I don't think the removal here is correct, the method does more than C4::Context->yaml_preference Nick maybe? Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15494 [Bug 15494] Block renewals by arbitrary item values -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #3 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #2)
I don't think the removal here is correct, the method does more than C4::Context->yaml_preference
Nick maybe?
perl -e 'use Koha::Config::SysPrefs; use C4::Context; my $sp = Koha::Config::SysPrefs->find("ItemsDeniedRenewal")->get_yaml_pref_hash; warn Data::Dumper::Dumper($sp); warn Data::Dumper::Dumper( C4::Context->yaml_preference("ItemsDeniedRenewal"));' It doesn't convert from 'NULL' to undef - the expectation is that we can pass the return value into a DBIC search directly -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Nick Clemens from comment #3)
(In reply to Jonathan Druart from comment #2)
I don't think the removal here is correct, the method does more than C4::Context->yaml_preference
Nick maybe?
perl -e 'use Koha::Config::SysPrefs; use C4::Context; my $sp = Koha::Config::SysPrefs->find("ItemsDeniedRenewal")->get_yaml_pref_hash; warn Data::Dumper::Dumper($sp); warn Data::Dumper::Dumper( C4::Context->yaml_preference("ItemsDeniedRenewal"));'
It doesn't convert from 'NULL' to undef - the expectation is that we can pass the return value into a DBIC search directly
Perhaps we removed the wrong copy here: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27154 One was an object, the other took a pref name, so would have used cache -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #5 from David Gustafsson <glasklas@gmail.com> --- Created attachment 150685 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150685&action=edit Bug 32478: Fix handling of undef values in ItemsDeniedRenewal -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #6 from David Gustafsson <glasklas@gmail.com> --- (In reply to Nick Clemens from comment #4)
(In reply to Nick Clemens from comment #3)
(In reply to Jonathan Druart from comment #2)
I don't think the removal here is correct, the method does more than C4::Context->yaml_preference
Nick maybe?
perl -e 'use Koha::Config::SysPrefs; use C4::Context; my $sp = Koha::Config::SysPrefs->find("ItemsDeniedRenewal")->get_yaml_pref_hash; warn Data::Dumper::Dumper($sp); warn Data::Dumper::Dumper( C4::Context->yaml_preference("ItemsDeniedRenewal"));'
It doesn't convert from 'NULL' to undef - the expectation is that we can pass the return value into a DBIC search directly
Perhaps we removed the wrong copy here: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27154
One was an object, the other took a pref name, so would have used cache
Yes, it would have used cache, but still a good thing to remove it as C4::Context->yaml_preference exists using a proper yaml-parser (and utilized cache). Thanks for pointing out the nuance of the special handling of NULL values. null (lowercase) is valid yaml for null values (converted to undef by the perl parser). So the problem can be solved by using that instead of NULL. Provided a patch to fix this. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144633|0 |1 is obsolete| | Attachment #150685|0 |1 is obsolete| | --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 150687 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150687&action=edit Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache get_yaml_pref_hash also allows invalid YAML and only parses a limited subset so remove this method to avoid future issues. To test): Since tests already exists for C4::Context->yaml_preference and this is a trivial change, do we really need a test plan for this? Sponsored-by: Gothenburg University Library Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 150688 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150688&action=edit Bug 32478: Fix handling of undef values in ItemsDeniedRenewal Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 150689 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150689&action=edit Bug 32478: (follow-up) UpdateItemLocationOnCheckin no longer needs flattening This patch removes a line flattening the arrays generated by get_yaml_pref_hash as it is no longer necessary Conditionals are adjusted to avoid warnings in tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #10 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 150690 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150690&action=edit Bug 32478: (follow-up) Tidy code Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Status|Signed Off |Failed QA --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- # Subtest: is_denied_renewal 1..11 ok 1 - Renewal allowed when no rules ok 2 - Renewal blocked when 1 rules (withdrawn) ok 3 - Renewal allowed when 1 rules not matched (withdrawn) ok 4 - Renewal blocked when 2 rules matched (withdrawn, itype) ok 5 - Renewal allowed when 2 rules not matched (withdrawn, itype) ok 6 - Renewal blocked when 3 rules matched (withdrawn, itype, location) ok 7 - Renewal allowed when 3 rules not matched (withdrawn, itype, location) not ok 8 - Renewal blocked for undef when NULL in pref # Failed test 'Renewal blocked for undef when NULL in pref' # at t/db_dependent/Koha/Item.t line 2157. # got: '0' # expected: '1' ok 9 - Renewal not blocked for undef when "" in pref ok 10 - Renewal not blocked for "" when NULL in pref ok 11 - Renewal blocked for empty string when "" in pref # Looks like you failed 1 test of 11. not ok 28 - is_denied_renewal -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- The replace NULL by null thing feels a bit odd btw.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #13 from David Gustafsson <glasklas@gmail.com> --- Ok! Suppose due to forgot to fix the tests that still uses the 'NULL' value, will do so. Why is it odd? null is valid json, NULL is not. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Gustafsson from comment #13)
Ok! Suppose due to forgot to fix the tests that still uses the 'NULL' value, will do so. Why is it odd? null is valid json, NULL is not.
Some things are hard to explain :) The fact that you need to type null as a valid JSON expression in a textarea is not very appealing as to user interface. (Or even stricter, valid YAML.) But I could certainly live with it :) Just document it clearly. And going bit out of scope, but why are we specifying a: [ b, c ] instead of the pure YAML approach with something like: a: - b - c We probably need some discussion/consensus about handling all such preferences. The fact that is all kind of YAML now, does however make a difference. See next comment. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- We remove these lines completely now and just trust ->yaml_preference: - foreach my $line (@lines){ - my ($field,$array) = split /:/, $line; - next if !$array; - $field =~ s/^\s*|\s*$//g; - $array =~ s/[ [\]\r]//g; - my @array = split /,/, $array; - @array = map { $_ eq '""' || $_ eq "''" ? '' : $_ } @array; - @array = map { $_ eq 'NULL' ? undef : $_ } @array; - $pref_as_hash->{$field} = \@array; - } But this is not the same. If I pasted correct YAML previously like below, it would not work. ccode : - null - NULL - undefined - 2 - '3' Now it does. But perhaps more serious is the opposite, if I am entering some valid YAML array by 'forgetting' the first line: - null - NULL - undefined - 2 - '3' I will get back an array, while the former routine returned an empty hash ! This will trigger an error like: Not a HASH reference at /usr/share/koha/Koha/Item.pm line 2071. => foreach my $field (keys %$denyingrules) { This is a bit long way of saying: I still think that we may need an intermediate routine between pure YAML (yaml_preference) and what we expect for specific preferences? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #16 from David Gustafsson <glasklas@gmail.com> --- (In reply to Marcel de Rooy from comment #14)
(In reply to David Gustafsson from comment #13)
Ok! Suppose due to forgot to fix the tests that still uses the 'NULL' value, will do so. Why is it odd? null is valid json, NULL is not.
Some things are hard to explain :) The fact that you need to type null as a valid JSON expression in a textarea is not very appealing as to user interface. (Or even stricter, valid YAML.) But I could certainly live with it :) Just document it clearly.
And going bit out of scope, but why are we specifying a: [ b, c ] instead of the pure YAML approach with something like: a: - b - c We probably need some discussion/consensus about handling all such preferences.
The fact that is all kind of YAML now, does however make a difference. See next comment.
Yes, but previously you still had to type and invalid subset of YAML, which should be worse? That you can use null values is documented in the preference description, where the patch changes the mentions of NULL to lowercase. both a: [ b, c ] and a: - b - c is valid YAML. The syspref documentation doesn't explicitly say the format is YAML, but I don't really see any reason to inform the user about the alternative syntax. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #17 from David Gustafsson <glasklas@gmail.com> --- (In reply to Marcel de Rooy from comment #15)
We remove these lines completely now and just trust ->yaml_preference: - foreach my $line (@lines){ - my ($field,$array) = split /:/, $line; - next if !$array; - $field =~ s/^\s*|\s*$//g; - $array =~ s/[ [\]\r]//g; - my @array = split /,/, $array; - @array = map { $_ eq '""' || $_ eq "''" ? '' : $_ } @array; - @array = map { $_ eq 'NULL' ? undef : $_ } @array; - $pref_as_hash->{$field} = \@array; - }
But this is not the same. If I pasted correct YAML previously like below, it would not work. ccode : - null - NULL - undefined - 2 - '3' Now it does.
But perhaps more serious is the opposite, if I am entering some valid YAML array by 'forgetting' the first line: - null - NULL - undefined - 2 - '3' I will get back an array, while the former routine returned an empty hash ! This will trigger an error like: Not a HASH reference at /usr/share/koha/Koha/Item.pm line 2071. => foreach my $field (keys %$denyingrules) {
This is a bit long way of saying: I still think that we may need an intermediate routine between pure YAML (yaml_preference) and what we expect for specific preferences?
I see your point, but was it not already possible to make that code crash (or even worse things) by for example using key values that does not exist on the item object, or for example using "delete" as key :/ sub is_denied_renewal { my ( $self ) = @_; my $denyingrules = Koha::Config::SysPrefs->find('ItemsDeniedRenewal')->get_yaml_pref_hash(); return 0 unless $denyingrules; foreach my $field (keys %$denyingrules) { my $val = $self->$field; Validation would be nice, but feels like out of the scope for this issue. I will update the patch to check that an actual hash before iterating over the keys though, as this particular case was not a problem before. If I'm not missing something $self->$field; where $field is user input is a security vulnerability and a pretty bad one at that. Should use get_column instead! I guess should create a new issue for this, or if we handle it in this one? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion --- Comment #18 from David Gustafsson <glasklas@gmail.com> --- I'm leaning towards that it's better to leave the patch as it is with regards to the problem of crashing on non hash-values. The same problem applies for 'OpacHiddenItems', that is currently using C4::Context->yaml_preference and not get_yaml_pref_hash. So feel like this issue should be handled in another issue and solved properly instead of some band aid solution like validating everywhere the syspref is used instead of on input. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #19 from David Gustafsson <glasklas@gmail.com> --- Created attachment 151136 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151136&action=edit Bug 32478: Fix syspref NULL values in tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- sub is_denied_renewal foreach my $field (keys %$denyingrules) { my $val = $self->$field; This is out of scope, but there is not even a check if it is a valid field. The method Koha::Item->a is not covered by tests! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150687|0 |1 is obsolete| | --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151726 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151726&action=edit Bug 32478: Remove Koha::Config::SysPref->find since bypasses cache get_yaml_pref_hash also allows invalid YAML and only parses a limited subset so remove this method to avoid future issues. To test): Since tests already exists for C4::Context->yaml_preference and this is a trivial change, do we really need a test plan for this? Sponsored-by: Gothenburg University Library Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150688|0 |1 is obsolete| | --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151727 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151727&action=edit Bug 32478: Fix handling of undef values in ItemsDeniedRenewal Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150689|0 |1 is obsolete| | --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151728&action=edit Bug 32478: (follow-up) UpdateItemLocationOnCheckin no longer needs flattening This patch removes a line flattening the arrays generated by get_yaml_pref_hash as it is no longer necessary Conditionals are adjusted to avoid warnings in tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150690|0 |1 is obsolete| | --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151729 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151729&action=edit Bug 32478: (follow-up) Tidy code Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151136|0 |1 is obsolete| | --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151730 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151730&action=edit Bug 32478: Fix syspref NULL values in tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151731 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151731&action=edit Bug 32478: (QA follow-up) Shebang and chmod for Koha/Config/SysPrefs.t Test plan: Run t/db_dependent/Koha/Config/SysPrefs.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151732 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151732&action=edit Bug 32478: (QA follow-up) Keep current hashref behavior Prevent a crash on wrong contents for ItemsDeniedRenewal pref as we did before. Note: Could be a provisional measure (no band aid to repeat anywhere) until we resolve this in preferences.pl. Test plan: Without this patch: Change ItemsDeniedRenewal to 'nonsense' Run perl -MKoha::Items -e'Koha::Items->find(X)->is_denied_renewal; print "OK\n"' => Replace X by a valid itemnumber Crashes with: Can't use string ("nonsense") as a HASH ref ... No OK print. Apply this patch Run perl -MKoha::Items -e'Koha::Items->find(X)->is_denied_renewal; print "OK\n"' => Replace X by a valid itemnumber Warns only with: Hashref expected for ItemsDeniedRenewal. You got OK. Clear ItemsDeniedRenewal Try again. No warning anymore. Run t/Context.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to David Gustafsson from comment #17)
I see your point, but was it not already possible to make that code crash (or even worse things) by for example using key values that does not exist on the item object, or for example using "delete" as key :/
Hmm. You already said it. Read too quick. The delete example is even worse! Will add a report. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |33844 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33844 [Bug 33844] item->is_denied_renewal should validate fields from associated pref -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #29 from David Gustafsson <glasklas@gmail.com> --- Ok, good! Yes, even though highly unlikely anyone would do that by mistake or malicious intent, but still serious issue. As wrote above get_column should be used here instead of dynamic method invokation. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #30 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|23.11.00 |23.11.00,23.05.01 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #31 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.05. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|23.11.00,23.05.01 |23.11.00,23.05.01,22.11.08 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32478 --- Comment #32 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Nice work everyone! Pushed to 22.11.x for next release -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org