[Koha-patches] [PATCH 2/2] Bug 5415 Let calls of SimpleSearch utilize considtent interface
LAURENT Henri-Damien
henridamien.laurent at biblibre.com
Thu Nov 18 10:56:38 CET 2010
Hi Colin.
I have seen that you are using die....
There has already been a discussion on that on list.
No concensus achieved.
But I find it really clunky to have that kind of warning for users :
Koha Software error....
I acknowledge that a warning has to be raised.
Maybe dying is really good practise. But Having more failproof features
would be far more comfortable for users (and developers)... When seeing
Software error, they will undoubtedly open a new bug... on bugzilla,
which is already a bit crowded.
Your aim here is surely to HAVE problems reported rather than "hidden"
in logs file.
But I would have rather, when it is not really a blocking problem, just
try and raise a warning.
If dying is really a requirement in Modern::Perl then it HAS to be
handled in the pl script. And I donot think it is done. Gentlemen ?
Can we reach a good practise ?
--
Henri-Damien LAURENT
Le 17/11/2010 15:24, Colin Campbell a écrit :
> Remove some unnecessary checks when check of error is
> sufficient. Make the order in some cases more logical
> Should remove some possibilities of runtime warning noise.
> Although some calls belong to the 'Nothing could
> ever go wrong' school have added some warnings
> ---
> C4/AuthoritiesMarc.pm | 9 +++++++--
> C4/Heading.pm | 7 +++++++
> C4/Matcher.pm | 9 ++++++---
> acqui/neworderbiblio.pl | 13 ++++++-------
> authorities/authorities-list.pl | 3 +++
> cataloguing/addbiblio.pl | 6 +++---
> cataloguing/addbooks.pl | 4 ++--
> cataloguing/value_builder/unimarc_field_4XX.pl | 5 ++++-
> labels/label-item-search.pl | 4 ++--
> misc/migration_tools/bulkmarcimport.pl | 8 ++++----
> misc/migration_tools/remove_unused_authorities.pl | 4 ++++
> serials/subscription-bib-search.pl | 2 +-
> t/lib/KohaTest.pm | 4 ++--
> 13 files changed, 51 insertions(+), 27 deletions(-)
>
> diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm
> index d07cca4..606c1c8 100644
> --- a/C4/AuthoritiesMarc.pm
> +++ b/C4/AuthoritiesMarc.pm
> @@ -362,7 +362,12 @@ sub CountUsage {
> my $query;
> $query= "an=".$authid;
> my ($err,$res,$result) = C4::Search::SimpleSearch($query,0,10);
> - return ($result);
> + if ($err) {
> + warn "Error: $err from search $query";
> + $result = 0;
> + }
> +
> + return $result;
> }
> }
>
> @@ -903,7 +908,7 @@ sub FindDuplicateAuthority {
> }
> my ($error, $results, $total_hits) = C4::Search::SimpleSearch( $query, 0, 1, [ "authorityserver" ] );
> # there is at least 1 result => return the 1st one
> - if (@$results>0) {
> + if (!defined $error && @{$results} ) {
> my $marcrecord = MARC::File::USMARC::decode($results->[0]);
> return $marcrecord->field('001')->data,BuildSummary($marcrecord,$marcrecord->field('001')->data,$authtypecode);
> }
> diff --git a/C4/Heading.pm b/C4/Heading.pm
> index 9d7d98a..1ea5a74 100644
> --- a/C4/Heading.pm
> +++ b/C4/Heading.pm
> @@ -24,6 +24,7 @@ use MARC::Field;
> use C4::Context;
> use C4::Heading::MARC21;
> use C4::Search;
> +use Carp;
>
> our $VERSION = 3.00;
>
> @@ -109,6 +110,9 @@ sub authorities {
> my $query = qq(Match-heading,ext="$self->{'search_form'}");
> $query .= $self->_query_limiters();
> my ($error, $results, $total_hits) = SimpleSearch( $query, undef, undef, [ "authorityserver" ] );
> + if (defined $error) {
> + carp "Error:$error from search $query";
> + }
> return $results;
> }
>
> @@ -126,6 +130,9 @@ sub preferred_authorities {
> my $query = "Match-heading-see-from,ext='$self->{'search_form'}'";
> $query .= $self->_query_limiters();
> my ($error, $results, $total_hits) = SimpleSearch( $query, undef, undef, [ "authorityserver" ] );
> + if (defined $error) {
> + carp "Error:$error from search $query";
> + }
> return $results;
> }
>
> diff --git a/C4/Matcher.pm b/C4/Matcher.pm
> index 5915c12..9d1df67 100644
> --- a/C4/Matcher.pm
> +++ b/C4/Matcher.pm
> @@ -606,9 +606,12 @@ sub get_matches {
> # FIXME only searching biblio index at the moment
> my ($error, $searchresults, $total_hits) = SimpleSearch($query, 0, $max_matches);
>
> - warn "search failed ($query) $error" if $error;
> - foreach my $matched (@$searchresults) {
> - $matches{$matched} += $matchpoint->{'score'};
> + if (defined $error ) {
> + warn "search failed ($query) $error";
> + } else {
> + foreach my $matched (@{$searchresults}) {
> + $matches{$matched} += $matchpoint->{'score'};
> + }
> }
> }
>
> diff --git a/acqui/neworderbiblio.pl b/acqui/neworderbiblio.pl
> index 2333f89..9dd29d8 100755
> --- a/acqui/neworderbiblio.pl
> +++ b/acqui/neworderbiblio.pl
> @@ -112,16 +112,15 @@ if (defined $error) {
>
> my @results;
>
> -if ($marcresults) {
> - foreach my $result ( @{$marcresults} ) {
> - my $marcrecord = MARC::File::USMARC::decode( $result );
> - my $biblio = TransformMarcToKoha( C4::Context->dbh, $marcrecord, '' );
> +foreach my $result ( @{$marcresults} ) {
> + my $marcrecord = MARC::File::USMARC::decode( $result );
> + my $biblio = TransformMarcToKoha( C4::Context->dbh, $marcrecord, '' );
>
> - $biblio->{booksellerid} = $booksellerid;
> - push @results, $biblio;
> + $biblio->{booksellerid} = $booksellerid;
> + push @results, $biblio;
>
> - }
> }
> +
> $template->param(
> basketno => $basketno,
> booksellerid => $bookseller->{'id'},
> diff --git a/authorities/authorities-list.pl b/authorities/authorities-list.pl
> index 9c472f1..eec3233 100755
> --- a/authorities/authorities-list.pl
> +++ b/authorities/authorities-list.pl
> @@ -25,6 +25,9 @@ foreach my $authority (@$dataauthorities){
> $query= "an=".$authority->{'authid'};
> # search for biblios mapped
> my ($err,$res,$used) = C4::Search::SimpleSearch($query,0,10);
> + if (defined $err) {
> + $used = 0;
> + }
> if ($marcauthority && $marcauthority->field($authtypes{$authority->{'authtypecode'}}->{'tag'})){
> print qq("),$marcauthority->field($authtypes{$authority->{'authtypecode'}}->{"tag"})->as_string(),qq(";),qq($authority->{'authid'};"),$authtypes{$authority->{'authtypecode'}}->{'lib'},qq(";$used\n);
> }
> diff --git a/cataloguing/addbiblio.pl b/cataloguing/addbiblio.pl
> index 2c0c63d..e64f4e2 100755
> --- a/cataloguing/addbiblio.pl
> +++ b/cataloguing/addbiblio.pl
> @@ -768,16 +768,16 @@ AND (authtypecode IS NOT NULL AND authtypecode<>\"\")|);
> warn "BIBLIOADDSAUTHORITIES: $error";
> return (0,0) ;
> }
> - if ($results && scalar(@$results)==1) {
> + if ( @{$results} == 1) {
> my $marcrecord = MARC::File::USMARC::decode($results->[0]);
> $field->add_subfields('9'=>$marcrecord->field('001')->data);
> $countlinked++;
> - } elsif (scalar(@$results)>1) {
> + } elsif (@{$results} > 1) {
> #More than One result
> #This can comes out of a lack of a subfield.
> # my $marcrecord = MARC::File::USMARC::decode($results->[0]);
> # $record->field($data->{tagfield})->add_subfields('9'=>$marcrecord->field('001')->data);
> - $countlinked++;
> + $countlinked++;
> } else {
> #There are no results, build authority record, add it to Authorities, get authid and add it to 9
> ###NOTICE : This is only valid if a subfield is linked to one and only one authtypecode
> diff --git a/cataloguing/addbooks.pl b/cataloguing/addbooks.pl
> index 4c8a989..5e89ef8 100755
> --- a/cataloguing/addbooks.pl
> +++ b/cataloguing/addbooks.pl
> @@ -85,8 +85,8 @@ if ($query) {
>
> # format output
> # SimpleSearch() give the results per page we want, so 0 offet here
> - my $total = scalar @$marcresults;
> - my @newresults = searchResults( 'intranet', $query, $total, $results_per_page, 0, 0, @$marcresults );
> + my $total = @{$marcresults};
> + my @newresults = searchResults( 'intranet', $query, $total, $results_per_page, 0, 0, @{$marcresults} );
> $template->param(
> total => $total_hits,
> query => $query,
> diff --git a/cataloguing/value_builder/unimarc_field_4XX.pl b/cataloguing/value_builder/unimarc_field_4XX.pl
> index 84a6208..ddb6547 100755
> --- a/cataloguing/value_builder/unimarc_field_4XX.pl
> +++ b/cataloguing/value_builder/unimarc_field_4XX.pl
> @@ -335,7 +335,10 @@ sub plugin {
> my $orderby;
> $search = 'kw,wrdl='.$search.' and mc-itemtype='.$itype if $itype;
> my ( $errors, $results, $total_hits ) = SimpleSearch($search, $startfrom * $resultsperpage, $resultsperpage );
> - my $total = scalar(@$results);
> + if (defined $errors ) {
> + $results = [];
> + }
> + my $total = @{$results};
>
> # warn " biblio count : ".$total;
>
> diff --git a/labels/label-item-search.pl b/labels/label-item-search.pl
> index bdc2109..9b92009 100755
> --- a/labels/label-item-search.pl
> +++ b/labels/label-item-search.pl
> @@ -92,8 +92,8 @@ if ( $op eq "do_search" ) {
> ( $error, $marcresults, $total_hits ) =
> SimpleSearch( $ccl_query, $offset, $resultsperpage );
>
> - if (scalar($marcresults) > 0) {
> - $show_results = scalar @$marcresults;
> + if (!defined $error && @{$marcresults} ) {
> + $show_results = @{$marcresults};
> }
> else {
> $debug and warn "ERROR label-item-search: no results from SimpleSearch";
> diff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl
> index 3bb9ed8..1d31382 100755
> --- a/misc/migration_tools/bulkmarcimport.pl
> +++ b/misc/migration_tools/bulkmarcimport.pl
> @@ -222,15 +222,15 @@ RECORD: while ( ) {
> my ($error, $results,$totalhits)=C4::Search::SimpleSearch( $query, 0, 3, [$server] );
> die "unable to search the database for duplicates : $error" if (defined $error);
> #warn "$query $server : $totalhits";
> - if ($results && scalar(@$results)==1){
> + if ( @{$results} == 1 ){
> my $marcrecord = MARC::File::USMARC::decode($results->[0]);
> $id=GetRecordId($marcrecord,$tagid,$subfieldid);
> }
> - elsif ($results && scalar(@$results)>1){
> - $debug && warn "more than one match for $query";
> + elsif ( @{$results} > 1){
> + $debug && warn "more than one match for $query";
> }
> else {
> - $debug && warn "nomatch for $query";
> + $debug && warn "nomatch for $query";
> }
> }
> my $originalid;
> diff --git a/misc/migration_tools/remove_unused_authorities.pl b/misc/migration_tools/remove_unused_authorities.pl
> index 070a109..fbeee78 100755
> --- a/misc/migration_tools/remove_unused_authorities.pl
> +++ b/misc/migration_tools/remove_unused_authorities.pl
> @@ -62,6 +62,10 @@ while (my $data=$rqselect->fetchrow_hashref){
> $query= "an=".$data->{'authid'};
> # search for biblios mapped
> my ($err,$res,$used) = C4::Search::SimpleSearch($query,0,10);
> + if (defined $err) {
> + warn "error: $err on search $query\n";
> + next;
> + }
> print ".";
> print "$counter\n" unless $counter++ % 100;
> # if found, delete, otherwise, just count
> diff --git a/serials/subscription-bib-search.pl b/serials/subscription-bib-search.pl
> index d1b0f63..b33737a 100755
> --- a/serials/subscription-bib-search.pl
> +++ b/serials/subscription-bib-search.pl
> @@ -90,7 +90,6 @@ if ($op eq "do_search" && $query) {
> $resultsperpage = 20 if(!defined $resultsperpage);
>
> my ($error, $marcrecords, $total_hits) = SimpleSearch($query, $startfrom*$resultsperpage, $resultsperpage);
> - my $total = scalar @$marcrecords;
>
> if (defined $error) {
> $template->param(query_error => $error);
> @@ -98,6 +97,7 @@ if ($op eq "do_search" && $query) {
> output_html_with_http_headers $input, $cookie, $template->output;
> exit;
> }
> + my $total = @{$marcrecords};
> my @results;
>
> for(my $i=0;$i<$total;$i++) {
> diff --git a/t/lib/KohaTest.pm b/t/lib/KohaTest.pm
> index 323b55a..6fcedcf 100644
> --- a/t/lib/KohaTest.pm
> +++ b/t/lib/KohaTest.pm
> @@ -590,8 +590,8 @@ sub add_biblios {
>
> $self->reindex_marc();
> my $query = 'Finn Test';
> - my ( $error, $results ) = SimpleSearch( $query );
> - if ( $param{'count'} <= scalar( @$results ) ) {
> + my ( $error, $results, undef ) = SimpleSearch( $query );
> + if ( !defined $error && $param{'count'} <= @{$results} ) {
> pass( "found all $param{'count'} titles" );
> } else {
> fail( "we never found all $param{'count'} titles" );
More information about the Koha-patches
mailing list