|
Description
Nick Clemens (kidclamp)
2017-04-07 16:51:42 UTC
Created attachment 61976 [details] [review] Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting To fix the warns we use a copy of itemtypes which is not touched by other code To test: Apply patches Ensure search still works Hi Nick
When applying your patch (in a clean up to date branch) there is a merge conflict where you have changed the opac/opac-search.pl (see below). I fixed that locally by keeping the contents of lines 234-237 and removing the contents of lines 232,233 and 238.
232 <<<<<<< HEAD
233 =======
234 my $itemtypes_copy = { %$itemtypes }; #Sometime itemtypes can be c orrupted in advanced_srch_type loop
235 #Making a copy ensure it is clean
236 # Load the Type stuff without search categories for facets
237 my $itemtypes_nocategory = GetItemTypes;
238 >>>>>>> Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting
Then whilst testing I used the OPAC search and I got the following software error:
Bareword "GetItemTypes" not allowed while "strict subs" in use at /home/vagrant/kohaclone/opac/opac-search.pl line 235.
Execution of /home/vagrant/kohaclone/opac/opac-search.pl aborted due to compilation errors.
Created attachment 62224 [details] [review] Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting To fix the warns we use a copy of itemtypes which is not touched by other code To test: Apply patches Ensure search still works Created attachment 62232 [details] [review] Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting To fix the warns we use a copy of itemtypes which is not touched by other code To test: Apply patches Ensure search still works Followed test plan, search works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Looks like Marc actually signed off, but status wasn't switched! QA: Looking here now Created attachment 63416 [details] [review] Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting To fix the warns we use a copy of itemtypes which is not touched by other code To test: Apply patches Ensure search still works Followed test plan, search works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 63417 [details] [review] Bug 18400: [QA Follow-up] Move sort outside the loop GetItemTypesCategorized can return descriptions that are still undef since Authorized values does not enforce a description in lib and lib_opac. When I add one ITEMTYPECAT auth value without descriptions, I can still generate the string comparison warnings on the itemtypes sort. In order to prevent the warning, we should add an empty string in the assignment on line 229. We do not need to copy the itemtypes hash if we move the sort outside the @advanced_search_types foreach. There is no need to sort it more than once. Note that I did not see any reasons btw for corruption of the structure inside this loop. Note: If we use ITEMTYPECAT without descriptions, we should probably leave them out. No need to show a checkbox without description on Advanced Search, but I would recommend to solve that on its own report. The whole ITEMTYPECAT functionality has imo not been designed properly. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> QA Comment: See commit message of the follow-up. Pushed to master for 17.05, thanks Nick, Marcel! Patches don't apply cleanly on 16.11.x - please rebase if you think it should be in the next stable release! We also encounter this problem in 16.11. My teammate found out the reason for $itemtype corruption was line 291:
searchcategory => $itemtypes->{$thisitemtype}->{'searchcategory'},
This automatically add a hash ->{'searchcategory'} to $itemtypes.
The correct code should likely be:
searchcategory => $thisitemtype->{'searchcategory'},
I attached a patch for 16.11. We tested this on our 16.11 installation and it also fix the problem.
Created attachment 64718 [details] [review] Bug 18400 - [16.11] Noisy warns in opac-search.pl during itemtype sorting Thx for the 16.11.x patch! This patch has been pushed to 16.11.x and will be in 16.11.10. Thanks for pushing! Could you also consider it for 17.05 and master too? Although the noisy messages were gone by the previous patch, the code on line 291 is probably incorrect. Pushed to 16.05.x, for 16.05.15 release |