From a7296b6325b18f3e154e1b029b9c87fe541d468f Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 7 Jan 2020 12:26:01 +0100 Subject: [PATCH] Bug 24361: (bug 24217 follow-up) Fix several warnings in C4 modules With bug 24217 pushed, lot of warnings appears during the tests are run. Most of them are "Use of uninitialized value in " Test plan: Take a look at the output of run 1049 https://jenkins.koha-community.org/job/Koha_Master_D9/1049/consoleFull Most of the warnings from this run will be removed by this patch At least 2 are not fixed: Use of uninitialized value in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 791. t/db_dependent/Items_DelItemCheck.t => see 23463 --- C4/Budgets.pm | 36 +++++++++++++++++++----------------- C4/Circulation.pm | 21 +++++++++++---------- C4/Items.pm | 16 +++++++++------- C4/Koha.pm | 2 +- C4/Overdues.pm | 6 +++--- C4/Record.pm | 6 +++--- C4/Reserves.pm | 16 ++++++++++------ C4/Serials.pm | 2 +- Koha/Plugins/Base.pm | 2 +- 9 files changed, 58 insertions(+), 49 deletions(-) diff --git a/C4/Budgets.pm b/C4/Budgets.pm index 387985ca76..16eb5a22a0 100644 --- a/C4/Budgets.pm +++ b/C4/Budgets.pm @@ -342,7 +342,7 @@ sub GetBudgetSpent { datecancellationprinted IS NULL |); $sth->execute($budget_id); - my $sum = 0 + $sth->fetchrow_array; + my $sum = ( $sth->fetchrow_array || 0 ) + 0; $sth = $dbh->prepare(qq| SELECT SUM(shipmentcost) AS sum @@ -352,7 +352,7 @@ sub GetBudgetSpent { $sth->execute($budget_id); my ($shipmentcost_sum) = $sth->fetchrow_array; - $sum += $shipmentcost_sum; + $sum += ( $shipmentcost_sum || 0 ) + 0; my $adjustments = Koha::Acquisition::Invoice::Adjustments->search({budget_id => $budget_id, closedate => { '!=' => undef } },{ join => 'invoiceid' }); while ( my $adj = $adjustments->next ){ @@ -373,7 +373,7 @@ sub GetBudgetOrdered { datecancellationprinted IS NULL |); $sth->execute($budget_id); - my $sum = 0 + $sth->fetchrow_array; + my $sum = ( $sth->fetchrow_array || 0 ) + 0; my $adjustments = Koha::Acquisition::Invoice::Adjustments->search({budget_id => $budget_id, encumber_open => 1, closedate => undef},{ join => 'invoiceid' }); while ( my $adj = $adjustments->next ){ @@ -606,12 +606,12 @@ sub _recursiveAdd { _recursiveAdd($child, $budget, $hr_budget_spent, $hr_budget_spent_shipment, $hr_budget_ordered, $hr_budget_ordered_shipment, $hr_budget_spent_adjustment, $hr_budget_ordered_adjustment ); } - $budget->{budget_spent} += $hr_budget_spent->{$budget->{budget_id}}->{budget_spent}; - $budget->{budget_spent} += $hr_budget_spent_shipment->{$budget->{budget_id}}->{shipmentcost}; - $budget->{budget_spent} += $hr_budget_spent_adjustment->{$budget->{budget_id}}->{adjustments}; - $budget->{budget_ordered} += $hr_budget_ordered->{$budget->{budget_id}}->{budget_ordered}; - $budget->{budget_ordered} += $hr_budget_ordered_shipment->{$budget->{budget_id}}->{shipmentcost}; - $budget->{budget_ordered} += $hr_budget_ordered_adjustment->{$budget->{budget_id}}->{adjustments}; + $budget->{budget_spent} += $hr_budget_spent->{$budget->{budget_id}}->{budget_spent} || 0; + $budget->{budget_spent} += $hr_budget_spent_shipment->{$budget->{budget_id}}->{shipmentcost} || 0; + $budget->{budget_spent} += $hr_budget_spent_adjustment->{$budget->{budget_id}}->{adjustments} || 0; + $budget->{budget_ordered} += $hr_budget_ordered->{$budget->{budget_id}}->{budget_ordered} || 0; + $budget->{budget_ordered} += $hr_budget_ordered_shipment->{$budget->{budget_id}}->{shipmentcost} || 0; + $budget->{budget_ordered} += $hr_budget_ordered_adjustment->{$budget->{budget_id}}->{adjustments} || 0; $budget->{total_spent} += $budget->{budget_spent}; $budget->{total_ordered} += $budget->{budget_ordered}; @@ -636,30 +636,32 @@ sub _add_budget_children { } # ------------------------------------------------------------------- - +# FIXME Must be replaced by Koha::Acquisition::Fund->store sub AddBudget { my ($budget) = @_; return unless ($budget); - undef $budget->{budget_encumb} if $budget->{budget_encumb} eq ''; - undef $budget->{budget_owner_id} if $budget->{budget_owner_id} eq ''; + undef $budget->{budget_encumb} if defined $budget->{budget_encumb} && $budget->{budget_encumb} eq ''; + undef $budget->{budget_owner_id} if defined $budget->{budget_owner_id} && $budget->{budget_owner_id} eq ''; my $resultset = Koha::Database->new()->schema->resultset('Aqbudget'); return $resultset->create($budget)->id; } # ------------------------------------------------------------------- +# FIXME Must be replaced by Koha::Acquisition::Fund->store sub ModBudget { my ($budget) = @_; my $result = Koha::Database->new()->schema->resultset('Aqbudget')->find($budget); return unless($result); - undef $budget->{budget_encumb} if $budget->{budget_encumb} eq ''; - undef $budget->{budget_owner_id} if $budget->{budget_owner_id} eq ''; + undef $budget->{budget_encumb} if defined $budget->{budget_encumb} && $budget->{budget_encumb} eq ''; + undef $budget->{budget_owner_id} if defined $budget->{budget_owner_id} && $budget->{budget_owner_id} eq ''; $result = $result->update($budget); return $result->in_storage; } # ------------------------------------------------------------------- +# FIXME Must be replaced by Koha::Acquisition::Fund->delete sub DelBudget { my ($budget_id) = @_; my $dbh = C4::Context->dbh; @@ -809,14 +811,14 @@ sub GetBudgetsReport { ON bp.budget_period_id = b.budget_period_id INNER JOIN aqorders o ON b.budget_id = o.budget_id '; - if($activity ne ''){ + if ( $activity && $activity ne '' ) { $query .= 'WHERE bp.budget_period_active=? '; } $query .= 'AND (o.orderstatus != "cancelled") ORDER BY b.budget_name'; my $sth = $dbh->prepare($query); - if($activity ne ''){ + if ( $activity && $activity ne '' ) { $sth->execute($activity); } else{ @@ -1221,7 +1223,7 @@ sub CloneBudgetHierarchy { my @first_level_budgets = ( not defined $children_of ) ? map { ( not $_->{budget_parent_id} ) ? $_ : () } @$budgets - : map { ( $_->{budget_parent_id} == $children_of ) ? $_ : () } @$budgets; + : map { ( defined $_->{budget_parent_id} && $_->{budget_parent_id} == $children_of ) ? $_ : () } @$budgets; # get only the columns of aqbudgets my @columns = Koha::Database->new()->schema->source('Aqbudget')->columns; diff --git a/C4/Circulation.pm b/C4/Circulation.pm index a62d60e4e6..d2506275e3 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -748,11 +748,11 @@ sub CanBookBeIssued { return( { STATS => 1 }, {}); } - if ( $patron->gonenoaddress == 1 ) { + if ( $patron->gonenoaddress && $patron->gonenoaddress == 1 ) { $issuingimpossible{GNA} = 1; } - if ( $patron->lost == 1 ) { + if ( $patron->lost && $patron->lost == 1 ) { $issuingimpossible{CARD_LOST} = 1; } if ( $patron->is_debarred ) { @@ -1430,7 +1430,8 @@ sub AddIssue { } )->store; } - if ( $item_object->location eq 'CART' && $item_object->permanent_location ne 'CART' ) { + if ( $item_object->location && $item_object->location eq 'CART' + && ( !$item_object->permanent_location || $item_object->permanent_location ne 'CART' ) ) { ## Item was moved to cart via UpdateItemLocationOnCheckin, anything issued should be taken off the cart. CartToShelf( $item_object->itemnumber ); } @@ -1458,7 +1459,7 @@ sub AddIssue { ModItem( { - issues => $item_object->issues + 1, + issues => ( $item_object->issues || 0 ) + 1, holdingbranch => C4::Context->userenv->{'branch'}, itemlost => 0, onloan => $datedue->ymd(), @@ -1472,7 +1473,7 @@ sub AddIssue { # If it costs to borrow this book, charge it to the patron's account. my ( $charge, $itemtype ) = GetIssuingCharges( $item_object->itemnumber, $borrower->{'borrowernumber'} ); - if ( $charge > 0 ) { + if ( $charge && $charge > 0 ) { AddIssuingCharge( $issue, $charge, 'RENT' ); } @@ -2168,7 +2169,7 @@ sub MarkIssueReturned { my $issue_id = $issue->issue_id; my $anonymouspatron; - if ( $privacy == 2 ) { + if ( $privacy && $privacy == 2 ) { # The default of 0 will not work due to foreign key constraints # The anonymisation will fail if AnonymousPatron is not a valid entry # We need to check if the anonymous patron exist, Koha will fail loudly if it does not @@ -2195,7 +2196,7 @@ sub MarkIssueReturned { my $old_checkout = Koha::Old::Checkout->new($issue->unblessed)->store; # anonymise patron checkout immediately if $privacy set to 2 and AnonymousPatron is set to a valid borrowernumber - if ( $privacy == 2) { + if ( $privacy && $privacy == 2) { $old_checkout->borrowernumber($anonymouspatron)->store; } @@ -2327,7 +2328,7 @@ sub _debar_user_on_return { # if borrower was already debarred but does not get an extra debarment my $patron = Koha::Patrons->find( $borrower->{borrowernumber} ); my ($new_debarment_str, $is_a_reminder); - if ( $borrower->{debarred} eq $patron->is_debarred ) { + if ( $borrower->{debarred} && $borrower->{debarred} eq $patron->is_debarred ) { $is_a_reminder = 1; $new_debarment_str = $borrower->{debarred}; } else { @@ -2927,7 +2928,7 @@ sub AddRenewal { # Update the issues record to have the new due date, and a new count # of how many times it has been renewed. - my $renews = $issue->renewals + 1; + my $renews = ( $issue->renewals || 0 ) + 1; my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ? WHERE borrowernumber=? AND itemnumber=?" @@ -2936,7 +2937,7 @@ sub AddRenewal { $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $lastreneweddate, $borrowernumber, $itemnumber ); # Update the renewal count on the item, and tell zebra to reindex - $renews = $item_object->renewals + 1; + $renews = ( $item_object->renewals || 0 ) + 1; ModItem( { renewals => $renews, onloan => $datedue->strftime('%Y-%m-%d %H:%M')}, $item_object->biblionumber, $itemnumber, { log_action => 0 } ); # Charge a new rental fee, if applicable diff --git a/C4/Items.pm b/C4/Items.pm index 862e5e2d90..bd335d5563 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -552,7 +552,7 @@ sub ModItemTransfer { my $item = Koha::Items->find( $itemnumber ); # Remove the 'shelving cart' location status if it is being used. - CartToShelf( $itemnumber ) if ( $item->location eq 'CART' && $item->permanent_location ne 'CART' ); + CartToShelf( $itemnumber ) if $item->location && $item->location eq 'CART' && ( !$item->permanent_location || $item->permanent_location ne 'CART' ); $dbh->do("UPDATE branchtransfers SET datearrived = NOW(), comments = ? WHERE itemnumber = ? AND datearrived IS NULL", undef, "Canceled, new transfer from $frombranch to $tobranch created", $itemnumber); @@ -1445,11 +1445,13 @@ sub _do_column_fixes_for_mod { (not defined $item->{'withdrawn'} or $item->{'withdrawn'} eq '')) { $item->{'withdrawn'} = 0; } - if (exists $item->{location} - and $item->{location} ne 'CART' - and $item->{location} ne 'PROC' + if ( + exists $item->{location} + and ( !defined $item->{location} + || ( $item->{location} ne 'CART' and $item->{location} ne 'PROC' ) ) and not $item->{permanent_location} - ) { + ) + { $item->{'permanent_location'} = $item->{'location'}; } if (exists $item->{'timestamp'}) { @@ -2515,7 +2517,7 @@ sub PrepareItemrecordDisplay { $subfield_data{marc_value} = { type => 'select', values => \@authorised_values, - default => "$defaultvalue", + default => $defaultvalue // q{}, labels => \%authorised_lib, }; } elsif ( $tagslib->{$tag}->{$subfield}->{value_builder} ) { @@ -2528,7 +2530,7 @@ sub PrepareItemrecordDisplay { my $pars = { dbh => $dbh, record => undef, tagslib =>$tagslib, id => $subfield_data{id}, tabloop => undef }; $plugin->build( $pars ); if ( $itemrecord and my $field = $itemrecord->field($tag) ) { - $defaultvalue = $field->subfield($subfield); + $defaultvalue = $field->subfield($subfield) || q{}; } if( !$plugin->errstr ) { #TODO Move html to template; see report 12176/13397 diff --git a/C4/Koha.pm b/C4/Koha.pm index 518c75776a..d1c5771440 100644 --- a/C4/Koha.pm +++ b/C4/Koha.pm @@ -846,7 +846,7 @@ sub NormalizeISBN { my $string = $params->{isbn}; my $strip_hyphens = $params->{strip_hyphens}; - my $format = $params->{format}; + my $format = $params->{format} || q{}; my $return_invalid = $params->{return_invalid}; return unless $string; diff --git a/C4/Overdues.pm b/C4/Overdues.pm index 2bf280cd06..26ce74850d 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -247,7 +247,7 @@ sub CalcFine { my $fine_unit = $issuing_rule->lengthunit || 'days'; my $chargeable_units = get_chargeable_units($fine_unit, $start_date, $end_date, $branchcode); - my $units_minus_grace = $chargeable_units - $issuing_rule->firstremind; + my $units_minus_grace = $chargeable_units - ($issuing_rule->firstremind || 0); my $amount = 0; if ( $issuing_rule->chargeperiod && ( $units_minus_grace > 0 ) ) { my $units = C4::Context->preference('FinesIncludeGracePeriod') ? $chargeable_units : $units_minus_grace; @@ -261,9 +261,9 @@ sub CalcFine { $amount = $issuing_rule->overduefinescap if $issuing_rule->overduefinescap && $amount > $issuing_rule->overduefinescap; # This must be moved to Koha::Item (see also similar code in C4::Accounts::chargelostitem - $item->{replacementprice} ||= $itemtype->defaultreplacecost + $item->{replacementprice} //= $itemtype->defaultreplacecost if $itemtype - && $item->{replacementprice} == 0 + && ( ! defined $item->{replacementprice} || $item->{replacementprice} == 0 ) && C4::Context->preference("useDefaultReplacementCost"); $amount = $item->{replacementprice} if ( $issuing_rule->cap_fine_to_replacement_price && $item->{replacementprice} && $amount > $item->{replacementprice} ); diff --git a/C4/Record.pm b/C4/Record.pm index f54e368449..c15c2076f7 100644 --- a/C4/Record.pm +++ b/C4/Record.pm @@ -80,7 +80,7 @@ Returns an ISO-2709 scalar sub marc2marc { my ($marc,$to_flavour,$from_flavour,$encoding) = @_; my $error; - if ($to_flavour =~ m/marcstd/) { + if ($to_flavour && $to_flavour =~ m/marcstd/) { my $marc_record_obj; if ($marc =~ /^MARC::Record/) { # it's already a MARC::Record object $marc_record_obj = $marc; @@ -427,12 +427,12 @@ sub marc2csv { for my $itemnumber ( @$itemnumbers) { my $item = Koha::Items->find( $itemnumber ); my $biblionumber = $item->biblio->biblionumber; - $output .= marcrecord2csv( $biblionumber, $id, $firstpass, $csv, $fieldprocessing, [$itemnumber] ); + $output .= marcrecord2csv( $biblionumber, $id, $firstpass, $csv, $fieldprocessing, [$itemnumber] ) // ''; $firstpass = 0; } } else { foreach my $biblio (@$biblios) { - $output .= marcrecord2csv( $biblio, $id, $firstpass, $csv, $fieldprocessing ); + $output .= marcrecord2csv( $biblio, $id, $firstpass, $csv, $fieldprocessing ) // ''; $firstpass = 0; } } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index b953cf8961..503771e914 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -204,7 +204,7 @@ sub AddReserve { my $waitingdate; # If the reserv had the waiting status, we had the value of the resdate - if ( $found eq 'W' ) { + if ( $found && $found eq 'W' ) { $waitingdate = $resdate; } @@ -228,7 +228,7 @@ sub AddReserve { item_level_hold => $checkitem ? 1 : 0, } )->store(); - $hold->set_waiting() if $found eq 'W'; + $hold->set_waiting() if $found && $found eq 'W'; logaction( 'HOLDS', 'CREATE', $hold->id, Dumper($hold->unblessed) ) if C4::Context->preference('HoldsLog'); @@ -1067,7 +1067,9 @@ sub ModReserveStatus { $sth_set->execute( $newstatus, $itemnumber ); my $item = Koha::Items->find($itemnumber); - if ( ( $item->location eq 'CART' && $item->permanent_location ne 'CART' ) && $newstatus ) { + if ( $item->location && $item->location eq 'CART' + && ( !$item->permanent_location || $item->permanent_location ne 'CART' ) + && $newstatus ) { CartToShelf( $itemnumber ); } } @@ -1120,7 +1122,8 @@ sub ModReserveAffect { _FixPriority( { biblionumber => $biblionumber } ); my $item = Koha::Items->find($itemnumber); - if ( ( $item->location eq 'CART' && $item->permanent_location ne 'CART' ) ) { + if ( $item->location && $item->location eq 'CART' + && ( !$item->permanent_location || $item->permanent_location ne 'CART' ) ) { CartToShelf( $itemnumber ); } @@ -1472,7 +1475,7 @@ sub _FixPriority { if ( $rank eq "del" ) { # FIXME will crash if called without $hold $hold->cancel; } - elsif ( $rank eq "W" || $rank eq "0" ) { + elsif ( $reserve_id && ( $rank eq "W" || $rank eq "0" ) ) { # make sure priority for waiting or in-transit items is 0 my $query = " @@ -1500,11 +1503,12 @@ sub _FixPriority { push( @priority, $line ); } + # FIXME This whole sub must be rewritten, especially to highlight what is done when reserve_id is not given # To find the matching index my $i; my $key = -1; # to allow for 0 to be a valid result for ( $i = 0 ; $i < @priority ; $i++ ) { - if ( $reserve_id == $priority[$i]->{'reserve_id'} ) { + if ( $reserve_id && $reserve_id == $priority[$i]->{'reserve_id'} ) { $key = $i; # save the index last; } diff --git a/C4/Serials.pm b/C4/Serials.pm index e7e9e3aa13..2b9e1a62a5 100644 --- a/C4/Serials.pm +++ b/C4/Serials.pm @@ -2144,7 +2144,7 @@ sub abouttoexpire { } } - } elsif ($subscription->{numberlength}>0) { + } elsif ( $subscription->{numberlength} && $subscription->{numberlength}>0) { return (countissuesfrom($subscriptionid,$subscription->{'startdate'}) >=$subscription->{numberlength}-1); } diff --git a/Koha/Plugins/Base.pm b/Koha/Plugins/Base.pm index 9751b194a2..6a723c0c15 100644 --- a/Koha/Plugins/Base.pm +++ b/Koha/Plugins/Base.pm @@ -179,7 +179,7 @@ sub get_metadata { #FIXME: Why another encoding issue? For metadata containing non latin characters. my $metadata = $self->{metadata}; - utf8::decode($metadata->{$_}) for keys %$metadata; + $metadata->{$_} && utf8::decode($metadata->{$_}) for keys %$metadata; return $metadata; } -- 2.11.0