When building attributes list for the patron maintenance list, cloning is performed in suboptimal way that generates warnings.
Created attachment 22664 [details] [review] bug_11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves.
Galen, can I ask you to scrutinise this one please.
Created attachment 24412 [details] [review] bug_11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Srdjan and Jonathan, could you provide more information on the effect of this patch? I have tested the patron attributes on memberentry.pl and it seems to work, but it's a bit hard to tell what to look out for. Also, the warnings in my logs seem the same before and after applying the patch. Also, shouldn't this line be changed as well? 825 my $newentry = { map { $_ => $entry->{$_} } %$entry };
The fix: my $newentry = { %$entry }; takes the hash and generates a copy. This is what: my $newentry = { map { $_ => $entry->{$_} } %$entry }; is supposed to do, but it does it wrong. It could be fixed by making it: my $newentry = { map { $_ => $entry->{$_} } keys %$entry }; However, mapping and access functions to copy are much likely to be slower than just straight copying. Instead of copying { k1 => v1, k2 => v2, ..., kn => vn } The broken code generates { k1 => v1, v1 => undef, k2 => v2, v2 => undef, ... kn => vn, vn => undef } This can create some weird errors if the value of any of those keys is undef. This is also twice the size of the hash. And yes, this fix should be applied to the other line as well. Hope this clarifies, Katrin.
Created attachment 26619 [details] [review] bug_11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves.
Created attachment 26627 [details] [review] bug_11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 26702 [details] [review] [PASSED QA] Bug 11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Tested with different types of patron attributes: - repeatable - linked to an authorized value - free text Tested editing, adding, removing one of multiple, adding multiple, etc. No regressions found. Passes all tests and QA script.
Created attachment 26703 [details] [review] [PASSED QA] Bug 11184: correct attribute cloning for the maint screen To test - Patron details entry page: * Have ExtendedPatronAttributes enabled. Check that "Additional attributes and identifiers" section behaves. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Tested with different types of patron attributes: - repeatable - linked to an authorized value - free text Tested editing, adding, removing one of multiple, adding multiple, etc. No regressions found. Passes all tests and QA script.
Pushed to master. Thanks, Srdjan!
*** Bug 11913 has been marked as a duplicate of this bug. ***
Pushed to 3.14.x, will be in 3.14.10
Picked for 3.12.15