Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting
Summary: Noisy warns in opac-search.pl during itemtype sorting
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-07 16:51 UTC by Nick Clemens
Modified: 2017-07-31 12:01 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting (1.96 KB, patch)
2017-04-07 16:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting (1.98 KB, patch)
2017-04-17 16:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting (2.16 KB, patch)
2017-04-17 20:05 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18400 - Noisy warns in opac-search.pl during itemtype sorting (2.25 KB, patch)
2017-05-12 09:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18400: [QA Follow-up] Move sort outside the loop (3.50 KB, patch)
2017-05-12 09:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18400 - [16.11] Noisy warns in opac-search.pl during itemtype sorting (1022 bytes, patch)
2017-06-29 10:51 UTC, Pongtawat
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-04-07 16:51:42 UTC
Cannot reliably recreate, but seeing in 3.22 - 16.11 when searching in opac we get lots of:
opac-search.pl: Use of uninitialized value in string comparison (cmp) at /usr/share/koha/opac/cgi-bin/opac/opac-search.pl

Tracing the error it seems $itemtype can be corrupted and has keys added like this (when dumped):
$VAR1 = 'MICROFORM';
$VAR2 = '1VOLTREAT';
$VAR3 = 'HASH(0x954d948)';
$VAR4 = 'HASH(0x9083428)';

etc.

This filled a disk on one of our servers
Comment 1 Nick Clemens 2017-04-07 16:53:11 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
Comment 2 Alex Buckley 2017-04-17 06:46:43 UTC
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.
Comment 3 Nick Clemens 2017-04-17 16:27:21 UTC
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
Comment 4 Marc Véron 2017-04-17 20:05:28 UTC
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>
Comment 5 Katrin Fischer 2017-05-12 05:48:57 UTC
Looks like Marc actually signed off, but status wasn't switched!
Comment 6 Marcel de Rooy 2017-05-12 07:29:12 UTC
QA: Looking here now
Comment 7 Marcel de Rooy 2017-05-12 09:04:54 UTC
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>
Comment 8 Marcel de Rooy 2017-05-12 09:04:59 UTC
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>
Comment 9 Marcel de Rooy 2017-05-12 09:06:05 UTC
QA Comment:
See commit message of the follow-up.
Comment 10 Kyle M Hall 2017-05-12 12:57:49 UTC
Pushed to master for 17.05, thanks Nick, Marcel!
Comment 11 Katrin Fischer 2017-05-14 10:00:01 UTC
Patches don't apply cleanly on 16.11.x - please rebase if you think it should be in the next stable release!
Comment 12 Pongtawat 2017-06-29 10:49:18 UTC
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.
Comment 13 Pongtawat 2017-06-29 10:51:59 UTC
Created attachment 64718 [details] [review]
Bug 18400 - [16.11]  Noisy warns in opac-search.pl during itemtype sorting
Comment 14 Katrin Fischer 2017-07-16 12:43:50 UTC
Thx for the 16.11.x patch!

This patch has been pushed to 16.11.x and will be in 16.11.10.
Comment 15 Pongtawat 2017-07-16 15:51:01 UTC
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.
Comment 16 Mason James 2017-07-31 12:01:24 UTC
Pushed to 16.05.x, for 16.05.15 release