An example: * Simple search for "jones" in the intranet gives: search.pl?q=jones 55 result(s) found for 'kw,wrdl: jones' in Koha Catalog. Reordering by e.g. "Title (A-Z)" gives: search.pl?x=kw&q=jones&sort_by=title_az 55 result(s) found for 'kw,wrdl: jones' in Koha Catalog. * Advanced search for "jones" in the default field ("Keyword") gives: search.pl?idx=kw&q=jones&idx=kw&idx=kw&sort_by=relevance 55 result(s) found for 'kw,wrdl: jones' in SKSK Catalog. Reordering by "Title (A-Z)" gives: search.pl?x=kw&q=jones&sort_by=title_az 55 result(s) found for 'kw,wrdl: jones' in SKSK Catalog. idx=kw is turned into x=kw, but we still get the same number of results. * Choosing "Author" as the index to search, and then searching for "jones" gives: search.pl?idx=au&q=jones&idx=kw&idx=kw&sort_by=relevance 41 result(s) found for 'au,wrdl: jones' in SKSK Catalog. And here is the biggie, reordering by "Title (A-Z)" gives: search.pl?x=au&q=jones&sort_by=title_az 55 result(s) found for 'kw,wrdl: jones' in SKSK Catalog. Choosing a different sort criteria results in a different number of hits, presumably because idx=au has been turned into x=au. Searching for "jones" as "Author" and choosing to sort by "Title (A-Z)" on the advanced search page does gives the expected result: search.pl?idx=au&q=jones&idx=kw&idx=kw&sort_by=title_az 41 result(s) found for 'au,wrdl: jones' in SKSK Catalog. The same thing seems to hapen no matter what index you choose to limit your search to, and what criteria you choose to sort on. So why does re-sorting turn idx into x?
After some more digging, it looks like the problem stems from the regex on line 474 of catalogue/search.pl: 472 for my $this_cgi ( split('&',$query_cgi) ) { 473 next unless $this_cgi; 474 $this_cgi =~ m/(.?)=(.*)/; 475 my $input_name = $1; 476 my $input_value = $2; 477 push @query_inputs, { input_name => $input_name, input_value => $input_value }; 478 if ($input_name eq 'idx') { 479 $scan_index_to_use = $input_value; # unless $scan_index_to_use; 480 } 481 } 482 $template->param ( QUERY_INPUTS => \@query_inputs, 483 scan_index_to_use => $scan_index_to_use ); .? is non-greedy and matches the minimum number of characters, which is the "x" part of "idx", resulting in the behaviour described earlier. A patch proposing to change that line to this: $this_cgi =~ m/(.*)=(.*)/; is coming up...
Created attachment 4493 [details] [review] Proposed patch To test: - In advanced search in the intranet choose Author as the search index - Do a search for an author, check the number of hits - Choose another value than the default from Sort by - Check that the number of hits is the same as for the original search, once the hits have been re-ordered
The problem is not present in the OPAC, where this code takes care of the parsing: 368 sub _input_cgi_parse ($) { 369 my @elements; 370 for my $this_cgi ( split('&',shift) ) { 371 next unless $this_cgi; 372 $this_cgi =~ /(.*?)=(.*)/; 373 push @elements, { input_name => $1, input_value => $2 }; 374 } 375 return @elements; 376 } Not sure if there is any practical difference between my proposed fix: $this_cgi =~ /(.*)=(.*)/; and the one used in opac/opac-search.pl: $this_cgi =~ /(.*?)=(.*)/; ?
I think the correct fix would be the one from the OPAC. I think: .? matches 0 or 1 characters which seems incorrect; .*= matches any characters, potentially up to the last =, which seems incorrect; .*?= matches any characters up to the first =. I'm not QA so I'm putting this back from "needs signoff" to "---". Hope that's OK.
Created attachment 4494 [details] [review] Revised patch Revised patch, taking into account the comments from MJ Ray. Uses .*? instead of .*
Created attachment 4499 [details] [review] Bug 6510 - [REVISED] Sort by in intranet search alters search and number of hits This patch uses .*? instead of .* To test: - In advanced search in the intranet choose Author as the search index - Do a search for an author, check the number of hits - Choose another value than the default from Sort by - Check that the number of hits is the same as for the original search, once the hits have been re-ordered Signed-off-by: MJ Ray <mjr@phonecoop.coop>
Pushed please test
Tested and the patch applied in 3.4.2 fixes the problem.