Summary: | "Sort by" in intranet search alters search and number of hits | ||
---|---|---|---|
Product: | Koha | Reporter: | Magnus Enger <magnus> |
Component: | Searching | Assignee: | Magnus Enger <magnus> |
Status: | CLOSED FIXED | QA Contact: | Bugs List <koha-bugs> |
Severity: | normal | ||
Priority: | PATCH-Sent (DO NOT USE) | CC: | chris, mjr, pelletiermaxime |
Version: | 3.6 | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Proposed patch
Revised patch Bug 6510 - [REVISED] Sort by in intranet search alters search and number of hits |
Description
Magnus Enger
2011-06-16 09:18:56 UTC
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. |