Bug 30110 - Potential bug source: plenty of "my" declarations with conditional assignments
Summary: Potential bug source: plenty of "my" declarations with conditional assignments
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-15 13:48 UTC by Andrii Nugged
Modified: 2023-12-28 20:42 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05


Attachments
Bug 30110: Fix concatenation during assignements (6.36 KB, patch)
2022-02-21 09:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30110: Fix concatenation during assignements (6.41 KB, patch)
2022-03-23 00:53 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30110: Fix concatenation during assignements (6.47 KB, patch)
2022-03-24 16:56 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrii Nugged 2022-02-15 13:48:06 UTC
We have a lot of these things like "my $var .= ...", what is shouldn't be (if no var before "my" so why conditional assignments?) - this looks like refactoring remnants since old times, here I found some to analyze (and fix):


C4/Circulation.pm:
  859:         my $guarantors_non_issues_charges += $patron->relationships_debt({ include_guarantors => 1, only_this_guarantor => 0, include_this_patron => 1 });
  
C4/Message.pm:
  165:     my $format ||= sub { $_[0] || "" };

C4/Search.pm:
  891:     my $weighted_query .= "(rk=(";    # Specifies that we're applying rank

C4/Utils/DataTables/VirtualShelves.pm:
   47:         my $join_vs .= q|

circ/circulation.pl:
  477:         my $guarantors_non_issues_charges += $patron->relationships_debt({ include_guarantors => 1, only_this_guarantor => 0, include_this_patron => 1 });

Koha/DateUtils.pm:
  146:     my $time_re .= qr|

t/Labels_split_ccn.t:
   41:         my $split_num += scalar(@{$ccns->{$_}});

t/Labels_split_ddcn.t:
   40:         my $split_num += scalar(@{$ddcns->{$_}});

t/Labels_split_lccn.t:
   43:         my $split_num += scalar(@{$lccns->{$_}});

t/db_dependent/SIP/Message.t:
  127:     my $resp .= C4::SIP::Sip::maybe_add( FID_CALL_NUMBER, $sip_item->hold_patron_name, $server );
  840:     my $resp .= C4::SIP::Sip::maybe_add( FID_CALL_NUMBER, $sip_item->hold_patron_bcode, $server );
Comment 1 Jonathan Druart 2022-02-21 09:54:34 UTC
Created attachment 130937 [details] [review]
Bug 30110: Fix concatenation during assignements

There are several occurrences of `my $var += ` or .= in the code.
It should not cause problems but it's confusing.

Test plan:
Read the patch and confirm that the changes make sense.
Comment 2 Fridolin Somers 2022-03-23 00:52:58 UTC
Surely a lot of copy/paste errors.

Looks good to me.
Comment 3 Fridolin Somers 2022-03-23 00:53:35 UTC
Created attachment 132043 [details] [review]
Bug 30110: Fix concatenation during assignements

There are several occurrences of `my $var += ` or .= in the code.
It should not cause problems but it's confusing.

Test plan:
Read the patch and confirm that the changes make sense.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 4 Martin Renvoize 2022-03-24 16:56:51 UTC
Created attachment 132188 [details] [review]
Bug 30110: Fix concatenation during assignements

There are several occurrences of `my $var += ` or .= in the code.
It should not cause problems but it's confusing.

Test plan:
Read the patch and confirm that the changes make sense.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2022-03-24 16:57:11 UTC
Funky.. nice bit of clean up, no regressions found.

PQA
Comment 6 Fridolin Somers 2022-03-25 00:25:37 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 7 Kyle M Hall 2022-03-25 14:40:42 UTC
Pushed to 21.11.x for 21.11.05