Bug 23084 - Replace grep {^$var$} with grep {$var eq $_}
Summary: Replace grep {^$var$} with grep {$var eq $_}
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 23166 25137
  Show dependency treegraph
 
Reported: 2019-06-08 19:51 UTC by Jonathan Druart
Modified: 2020-04-14 09:59 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} (35.48 KB, patch)
2019-06-20 02:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} (37.07 KB, patch)
2019-06-20 02:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} (37.07 KB, patch)
2019-06-20 02:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} (37.60 KB, patch)
2020-02-11 12:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} (37.69 KB, patch)
2020-02-11 13:29 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} (37.06 KB, patch)
2020-02-14 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} (36.99 KB, patch)
2020-02-14 15:14 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
j# Attachment to Bug 23084 - Replace grep {^$var$} with grep {$var eq $_} (37.05 KB, patch)
2020-02-14 16:08 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-06-08 19:51:04 UTC
See bug 23006 and bug 22941.
Comment 1 M. Tompsett 2019-06-13 19:33:32 UTC
See also bug 17526 to deal with OPAC lists.
Comment 2 Jonathan Druart 2019-06-20 02:01:33 UTC
Created attachment 90820 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!
Comment 3 Jonathan Druart 2019-06-20 02:03:37 UTC
Created attachment 90821 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!
Comment 4 Jonathan Druart 2019-06-20 02:18:01 UTC
Created attachment 90823 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!
Comment 5 M. Tompsett 2019-06-20 15:33:18 UTC
Comment on attachment 90823 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

Review of attachment 90823 [details] [review]:
-----------------------------------------------------------------

I only thing that really stood out was the C4/Search.pm
"Why is the \Q and \E missing?"?

Explain that, I may sign this off.

Everything else is a result of me seeing tcohen wanting to improve readability of code. "unless grep" is better read as "if none".

::: C4/ImportBatch.pm
@@ +1114,4 @@
>      my $dbh = C4::Context->dbh;
>  
>      my $order_by = $parameters->{order_by} || 'import_record_id';
> +    ( $order_by ) = grep( { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? $order_by : 'import_record_id';

Wouldn't 'any' be semantically easier to read? It also has the added benefit of removing the whole need for () around $order_by, because $order_by is a string.

$order_by = any { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? $order_by : 'import_record_id';

Actually, the whole point of the code is even clearer with 'none':

$order_by = none { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? 'import_record_id' : $order_by;

::: C4/Installer/PerlModules.pm
@@ +76,4 @@
>      no warnings
>        ;  # perl throws warns for invalid $VERSION numbers which some modules use
>      my ( $self, $module ) = @_;
> +    return -1 unless grep { $_ eq $module } keys(%$PERL_DEPS);

any is better, since it is boolean.

::: C4/Items.pm
@@ +2101,4 @@
>          push @columns, Koha::Database->new()->schema()->resultset('Biblioitem')->result_source->columns;
>          my @operators = qw(= != > < >= <= like);
>          my $field = $filter->{field};
> +        if ( (0 < grep { $_ eq $field } @columns) or (substr($field, 0, 5) eq 'marc:') ) {

any is better, since it checking if the field is in the list of columns.

@@ +2105,4 @@
>              my $op = $filter->{operator};
>              my $query = $filter->{query};
>  
> +            if (!$op or (0 == grep { $_ eq $op } @operators)) {

none would be better than a '0 ==' check.

@@ +2601,5 @@
>          |;
>          for my $condition ( @$conditions ) {
>              if (
> +                 grep { $_ eq $condition->{field} } @item_columns
> +              or grep { $_ eq $condition->{field} } @biblioitem_columns

any's would be nicer.

::: C4/OAI/Sets.pm
@@ +500,4 @@
>              my $value = $mapping->{'marcvalue'};
>              my @subfield_values = $record->subfield($field, $subfield);
>              if ($operator eq 'notequal') {
> +                if(0 == grep { $_ eq $value } @subfield_values) {

none.

@@ +505,5 @@
>                      last;
>                  }
>              }
>              else {
> +                if(0 < grep { $_ eq $value } @subfield_values) {

any.

::: C4/Overdues.pm
@@ +766,4 @@
>      # Put 'print' in first if exists
>      # It avoid to sent a print notice with an email or sms template is no email or sms is defined
>      @mtts = uniq( 'print', @mtts )
> +        if grep { $_ eq 'print' } @mtts;

any.

::: C4/Search.pm
@@ +731,4 @@
>  
>                  my $data = $field->as_string( $subfield_letters, $facet->{ sep } );
>  
> +                unless ( grep { $_ eq $data } @used_datas ) {

Why is the \Q and \E missing?

@@ +1490,4 @@
>          # this happens when selecting a subject on the opac-detail page
>          @limits = grep {!/^$/} @limits;
>          my $original_q = $q; # without available part
> +        unless ( grep { $_ eq 'available' } @limits ) {

unless is harder to read construction, if none would be clearer, I think.

@@ +1494,5 @@
>              $q =~ s| and \( \( allrecords,AlwaysMatches:'' not onloan,AlwaysMatches:''\) and \(lost,st-numeric=0\) \)||;
>              $original_q = $q;
>          }
>          if ( @limits ) {
> +            if ( grep { $_ eq 'available' } @limits ) {

any.

::: C4/Serials.pm
@@ +1579,4 @@
>          ### Would use substr and index But be careful to previous presence of ()
>          $recievedlist .= "; $serialseq" unless ( index( $recievedlist, $serialseq ) > 0 );
>      }
> +    if ( grep { $_ eq $status } (MISSING_STATUSES) ) {

any.

::: C4/Stats.pm
@@ +108,4 @@
>      }
>      my @invalid_params = ();
>      for my $myparam (keys %$params ) {
> +        push @invalid_params, $myparam unless grep { $_ eq $myparam } @allowed_keys;

if none would be easier to read.

::: C4/Tags.pm
@@ +192,4 @@
>  			carp "Empty argument key to get_tag_rows: ignoring!";
>  			next;
>  		}
> +		unless (1 == scalar grep { $_ eq $key } @ok_fields) {

if none would be easier to read.

@@ +233,4 @@
>  			carp "Empty argument key to get_tags: ignoring!";
>  			next;
>  		}
> +		unless (1 == scalar grep { $_ eq $key } @ok_fields) {

if none would be easier to read.

@@ +302,4 @@
>  			carp "Empty argument key to get_approval_rows: ignoring!";
>  			next;
>  		}
> +		unless (1 == scalar grep { $_ eq $key } @ok_fields) {

if none would be easier to read.

::: C4/Utils/DataTables/Members.pm
@@ +38,4 @@
>      # Do that after iTotalQuery!
>      if ( defined $branchcode and $branchcode ) {
>          @restricted_branchcodes = @restricted_branchcodes
> +            ? grep { $_ eq $branchcode } @restricted_branchcodes

@restricted_branchcodes = ((scalar @restricted_branchcodes > 0) &&
           (none { $_ eq $branchcode } @restricted_branchcodes))
    ? (undef)
    : ($branchcode);

Nested ternary's are harder to read.

::: Koha/Object.pm
@@ +236,4 @@
>      my @columns = @{$self->_columns()};
>  
>      foreach my $p ( keys %$properties ) {
> +        unless ( grep { $_ eq $p } @columns ) {

if none.

@@ +442,4 @@
>  
>      my @columns = @{$self->_columns()};
>      # Using direct setter/getter like $item->barcode() or $item->barcode($barcode);
> +    if ( grep { $_ eq $method } @columns ) {

if any.

@@ +457,4 @@
>      Koha::Exceptions::Object::MethodNotCoveredByTests->throw(
>          error      => sprintf("The method %s->%s is not covered by tests!", ref($self), $method),
>          show_trace => 1
> +    ) unless grep { $_ eq $method } @known_methods;

if none.

::: Koha/Objects.pm
@@ +384,4 @@
>      $method =~ s/.*:://;
>  
>  
> +    unless ( grep { $_ eq $method } @known_methods ) {

if none.

::: acqui/duplicate_orders.pl
@@ +86,4 @@
>  if ( $op eq 'select' ) {
>      @result_order_loop = map {
>          my $order = $_;
> +        ( grep {$_ eq $order->{ordernumber}} @ordernumbers ) ? () : $order

Too ugly to suggest something.

@@ +132,4 @@
>      for my $field (
>          qw(currency budget_id order_internalnote order_vendornote sort1 sort2 ))
>      {
> +        next if grep { $_ eq $field } @fields_to_copy;

any.

::: admin/preferences.pl
@@ +117,4 @@
>                  {
>                      text     => $options{multiple}->{$option_value},
>                      value    => $option_value,
> +                    selected => (grep { $_ eq $option_value } @values) ? 1 : 0,

any without the ternary?

::: admin/searchengine/elasticsearch/mappings.pl
@@ +124,4 @@
>              my $mapping_suggestible = $mapping_suggestible[$i];
>              my $mapping_sort        = $mapping_sort[$i];
>              $mapping_sort = undef if $mapping_sort eq 'undef';
> +            $mapping_facet = ( grep { $_ eq $search_field_name } @facetable_field_names ) ? $mapping_facet : 0;

$mapping_facet = 0 if none { $_ eq $search_field_name } @facetable_field_names;

@@ +200,4 @@
>              sort               => $s->get_column('sort') // 'undef', # To avoid warnings "Use of uninitialized value in lc"
>              suggestible        => $s->get_column('suggestible'),
>              facet              => $s->get_column('facet'),
> +            is_facetable       => ( grep { $_ eq $name } @facetable_field_names ) ? 1 : 0,

any without the ternary?

::: catalogue/ISBDdetail.pl
@@ +152,4 @@
>      my $basket = $myorder->{'basketno'};
>      if ((defined $myorder->{'datecancellationprinted'}) and  ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){
>          push @deletedorders_using_biblio, $myorder;
> +        unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none.

@@ +157,5 @@
>          }
>      }
>      else {
>          push @orders_using_biblio, $myorder;
> +        unless (grep{ $_ eq $basket } @baskets_orders){

if none, and fix the brace indentation below.

::: catalogue/MARCdetail.pl
@@ +328,4 @@
>      my $basket = $myorder->{'basketno'};
>      if ((defined $myorder->{'datecancellationprinted'}) and  ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){
>          push @deletedorders_using_biblio, $myorder;
> +        unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none

@@ +333,5 @@
>          }
>      }
>      else {
>          push @orders_using_biblio, $myorder;
> +        unless (grep { $_ eq $basket } @baskets_orders){

if none with brace indentation fix below.

::: catalogue/detail.pl
@@ +522,4 @@
>      my $basket = $myorder->{'basketno'};
>      if ((defined $myorder->{'datecancellationprinted'}) and  ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){
>          push @deletedorders_using_biblio, $myorder;
> +        unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none

@@ +527,5 @@
>          }
>      }
>      else {
>          push @orders_using_biblio, $myorder;
> +        unless (grep{ $_ eq $basket } @baskets_orders){

if none with brace indentation below.

::: catalogue/imageviewer.pl
@@ +89,4 @@
>      my $basket = $myorder->{'basketno'};
>      if ((defined $myorder->{'datecancellationprinted'}) and  ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){
>          push @deletedorders_using_biblio, $myorder;
> +        unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none

@@ +99,1 @@
>              push @baskets_orders,$myorder->{'basketno'};

if none with brace indentation below.

::: catalogue/itemsearch.pl
@@ +63,4 @@
>                  push @f, $columns[$i];
>                  push @c, 'and';
>  
> +                if ( grep { $_ eq $columns[$i] } qw( ccode homebranch holdingbranch location itype notforloan itemlost ) ) {

any would be better.

::: catalogue/labeledMARCdetail.pl
@@ +135,4 @@
>      my $basket = $myorder->{'basketno'};
>      if ((defined $myorder->{'datecancellationprinted'}) and  ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){
>          push @deletedorders_using_biblio, $myorder;
> +        unless (grep { $_ eq $basket } @baskets_deletedorders){

if none

@@ +140,5 @@
>          }
>      }
>      else {
>          push @orders_using_biblio, $myorder;
> +        unless (grep{ $_ eq $basket } @baskets_orders){

if none with brace indentation fix below.

::: catalogue/moredetail.pl
@@ +235,4 @@
>      my $basket = $myorder->{'basketno'};
>      if ((defined $myorder->{'datecancellationprinted'}) and  ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){
>          push @deletedorders_using_biblio, $myorder;
> +        unless (grep{ $_ eq $basket } @baskets_deletedorders){

if none

@@ +240,5 @@
>          }
>      }
>      else {
>          push @orders_using_biblio, $myorder;
> +        unless (grep { $_ eq $basket } @baskets_orders){

if none with brace fix indentation below.

::: circ/circulation.pl
@@ +126,4 @@
>      $template_name = q|circ/circulation_batch_checkouts.tt|;
>      my @batch_category_codes = split '\|', C4::Context->preference('BatchCheckoutsValidCategories');
>      my $categorycode = $patron->categorycode;
> +    if ( $categorycode && grep { $_ eq $categorycode } @batch_category_codes ) {

any

Though, I wonder if this $batch_allowed and $barcodes[] could be optimized a bit with the any logic.

::: clubs/templates-add-modify.pl
@@ +92,4 @@
>            ? Koha::Club::Template::Fields->find($field_id)
>            : Koha::Club::Template::Field->new();
>  
> +        if ( grep { $_ eq $field_id } @field_delete ) {

any

@@ +126,4 @@
>            ? Koha::Club::Template::EnrollmentFields->find($field_id)
>            : Koha::Club::Template::EnrollmentField->new();
>  
> +        if ( grep { $_ eq $field_id } @field_delete ) {

any

::: misc/cronjobs/gather_print_notices.pl
@@ +91,4 @@
>  @all_messages = map {
>      my $letter_code = $_->{letter_code};
>      (
> +        grep { $_ eq $letter_code } @letter_codes

ugly code map code. no patently obvious suggestions.

::: misc/cronjobs/longoverdue.pl
@@ +301,4 @@
>  $skip_borrower_category = [ map { uc $_} @$skip_borrower_category ];
>  my %category_to_process;
>  for my $cat ( @$borrower_category ) {
> +    unless ( grep { $_ eq $cat } @available_categories ) {

if none

@@ +311,4 @@
>  }
>  if ( @$skip_borrower_category ) {
>      for my $cat ( @$skip_borrower_category ) {
> +        unless ( grep { $_ eq $cat } @available_categories ) {

if none

@@ +329,4 @@
>  $skip_itemtype = [ map { uc $_} @$skip_itemtype ];
>  my %itemtype_to_process;
>  for my $it ( @$itemtype ) {
> +    unless ( grep { $_ eq $it } @available_itemtypes ) {

if none

@@ +339,4 @@
>  }
>  if ( @$skip_itemtype ) {
>      for my $it ( @$skip_itemtype ) {
> +        unless ( grep { $_ eq $it } @available_itemtypes ) {

if none

::: misc/migration_tools/rebuild_zebra.pl
@@ +146,4 @@
>  }
>  
>  our @tables_allowed_for_select = ( 'biblioitems', 'items', 'biblio', 'biblio_metadata' );
> +unless ( grep { $_ eq $table } @tables_allowed_for_select ) {

if none

@@ +476,4 @@
>  
>  sub select_all_biblios {
>      $table = 'biblioitems'
> +      unless grep { $_ eq $table } @tables_allowed_for_select;

if none

::: misc/translator/LangInstaller.pm
@@ +634,4 @@
>              next if $entry =~ /^\./;
>              my $relentry = File::Spec->catfile($dir, $entry);
>              my $abspath = File::Spec->catfile($basedir, $relentry);
> +            if (-d $abspath and not grep { $_ eq $relentry } @blacklist) {

none

::: opac/opac-detail.pl
@@ +720,4 @@
>      
>      if ( C4::Context->preference('OPACAcquisitionDetails') ) {
>          $itm->{on_order} = 1
> +          if grep { $_ eq $itm->{itemnumber} } @itemnumbers_on_order;

any. possible optimization?

::: opac/opac-memberentry.pl
@@ +77,4 @@
>  
>  my @libraries = Koha::Libraries->search;
>  if ( my @libraries_to_display = split '\|', C4::Context->preference('PatronSelfRegistrationLibraryList') ) {
> +    @libraries = map { my $b = $_; my $branchcode = $_->branchcode; grep { $_ eq $branchcode } @libraries_to_display ? $b : () } @libraries;

ugly code! no suggestions.

::: opac/opac-search-history.pl
@@ +70,4 @@
>                  @searches = map { $_->{type} ne $type ? $_ : () } @searches;
>              }
>              if ( @id ) {
> +                @searches = map { my $search = $_; ( grep { $_ eq $search->{id} } @id ) ? () : $_ } @searches;

so many ugly map{ grep {...}}'s.

::: opac/opac-search.pl
@@ +431,4 @@
>  @sort_by = $cgi->multi_param('sort_by');
>  $sort_by[0] = $default_sort_by if !$sort_by[0] && defined($default_sort_by);
>  foreach my $sort (@sort_by) {
> +    if ( grep { $_ eq $sort } @allowed_sortby ) {

any

::: opac/opac-shelves.pl
@@ +114,3 @@
>      if ( $shelf ) {
>          $op = $referer;
>          my $sortfield = $query->param('sortfield');

if $sortfield is undef, next line will explode.
// 'title'; would be good.

@@ +114,4 @@
>      if ( $shelf ) {
>          $op = $referer;
>          my $sortfield = $query->param('sortfield');
> +        $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded );

if none.

::: reports/borrowers_stats.pl
@@ +162,4 @@
>          my $attribute_type = $1;
>          return unless (grep {$attribute_type eq $_->{code}} @attribute_types);
>      } else {
> +        return unless (grep { $_ eq $line } @valid_names);

if none

@@ +167,5 @@
>      if ($column =~ /^patron_attr\.(.*)/) {
>          my $attribute_type = $1;
>          return unless (grep {$attribute_type eq $_->{code}} @attribute_types);
>      } else {
> +        return unless (grep { $_ eq $column } @valid_names);

if none

@@ +172,3 @@
>      }
>      return if ($digits and $digits !~ /^\d+$/);
> +    return if ($status and (grep { $_ eq $status } qw(debarred gonenoaddress lost)) == 0);

none

@@ +172,4 @@
>      }
>      return if ($digits and $digits !~ /^\d+$/);
> +    return if ($status and (grep { $_ eq $status } qw(debarred gonenoaddress lost)) == 0);
> +    return if ($activity and (grep { $_ eq $activity } qw(active nonactive)) == 0);

none

::: tools/export.pl
@@ +75,4 @@
>      if ( $filename ) {
>          my $mimetype = $query->uploadInfo($filename)->{'Content-Type'};
>          my @valid_mimetypes = qw( application/octet-stream text/csv text/plain application/vnd.ms-excel );
> +        unless ( grep { $_ eq $mimetype } @valid_mimetypes ) {

if none

::: tools/letter.pl
@@ +263,4 @@
>      my $preview_is_available = 0;
>  
>      if ($code) {
> +        $preview_is_available = grep {$_ eq $code } qw( CHECKIN CHECKOUT HOLD_SLIP );

could optimize with an any.

::: tools/modborrowers.pl
@@ +278,4 @@
>      for my $field ( qw/surname firstname branchcode categorycode city state zipcode country sort1 sort2 dateenrolled dateexpiry borrowernotes opacnote/ ) {
>          my $value = $input->param($field);
>          $infos->{$field} = $value if $value;
> +        $infos->{$field} = "" if grep { $_ eq $field } @disabled;

if (any...) {
    $infos->{$field} = q{};
} else {
    $infos->{$field} = $value if $value;
}

-- double assign shorter vertically, but two assigns are slower than one. Plus, any is better than grep.

@@ +313,4 @@
>              # If this borrower is not in the category of this attribute, we don't want to modify this attribute
>              ++$i and next if $attr_type->{category_code} and $attr_type->{category_code} ne $borrower_categorycode;
>              my $valuename = "attr" . $i . "_value";
> +            if ( grep { $_ eq $valuename } @disabled ) {

any

::: virtualshelves/shelves.pl
@@ +102,4 @@
>      if ( $shelf ) {
>          $op = $referer;
>          my $sortfield = $query->param('sortfield');
> +        $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded );

if none. also might want a "|| 'title'" added above.

@@ +234,3 @@
>      if ( $shelf ) {
>          if ( $shelf->can_be_viewed( $loggedinuser ) ) {
>              my $sortfield = $query->param('sortfield') || $shelf->sortfield || 'title';    # Passed in sorting overrides default sorting

Actually, the following line is switch to title as default if the passed sortfield isn't one of the valid ones. Comment is wrong.

@@ +234,4 @@
>      if ( $shelf ) {
>          if ( $shelf->can_be_viewed( $loggedinuser ) ) {
>              my $sortfield = $query->param('sortfield') || $shelf->sortfield || 'title';    # Passed in sorting overrides default sorting
> +            $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded );

if none.
Comment 6 Jonathan Druart 2019-06-22 01:30:58 UTC
(In reply to M. Tompsett from comment #5)
> I only thing that really stood out was the C4/Search.pm
> "Why is the \Q and \E missing?"?
> 
> Explain that, I may sign this off.

This is the whole point of the patch. Have a look at the related bugs, and what this patch is trying to prevent. If the variable contains a regex character (like parenthesis in the case of bug 22941, or $ in the case of bug 23006), then you need to surround your var with \Q\E, see man perlre).

So the patch could have replaced
  grep {/^$var$/} @array
with
  grep {/^\Q$var\E$/} @array

But I preferred the other solution, I am sure you understand why :)

> Everything else is a result of me seeing tcohen wanting to improve
> readability of code. "unless grep" is better read as "if none".

I am not tcohen :)
The patch is a bugfix (updating the status). I would like to prevent other issues like the ones listed in see also. Replacing them with none, any or anything else will need more grey matter to make sure the changes will not introduce a regression. I am not against what you suggest but I think it should be done separately.
Comment 7 M. Tompsett 2019-06-24 15:45:18 UTC
> This is the whole point of the patch. Have a look at the related bugs, and
> what this patch is trying to prevent. If the variable contains a regex
> character (like parenthesis in the case of bug 22941, or $ in the case of
> bug 23006), then you need to surround your var with \Q\E, see man perlre).
> 
> So the patch could have replaced
>   grep {/^$var$/} @array
> with
>   grep {/^\Q$var\E$/} @array
> 
> But I preferred the other solution, I am sure you understand why :)

Yes, anything that requires an escaped solution adds a level of complexity and poor readability which makes it prone to breaking.


> > Everything else is a result of me seeing tcohen wanting to improve
> > readability of code. "unless grep" is better read as "if none".
> 
> I am not tcohen :)
> The patch is a bugfix (updating the status). I would like to prevent other
> issues like the ones listed in see also. Replacing them with none, any or
> anything else will need more grey matter to make sure the changes will not
> introduce a regression. I am not against what you suggest but I think it
> should be done separately.

I agree with that, which is why I just wanted an explanation of the \Q \E thing. Thanks for that.
Comment 8 M. Tompsett 2019-06-24 15:57:20 UTC
Comment on attachment 90823 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

Review of attachment 90823 [details] [review]:
-----------------------------------------------------------------

These are the two places I will actually test, because everything else is a easy eyeball by comparison.

::: opac/opac-memberentry.pl
@@ +77,4 @@
>  
>  my @libraries = Koha::Libraries->search;
>  if ( my @libraries_to_display = split '\|', C4::Context->preference('PatronSelfRegistrationLibraryList') ) {
> +    @libraries = map { my $b = $_; my $branchcode = $_->branchcode; grep { $_ eq $branchcode } @libraries_to_display ? $b : () } @libraries;

Super ugly code. There won't be scoping issues the $_?

::: opac/opac-search-history.pl
@@ +70,4 @@
>                  @searches = map { $_->{type} ne $type ? $_ : () } @searches;
>              }
>              if ( @id ) {
> +                @searches = map { my $search = $_; ( grep { $_ eq $search->{id} } @id ) ? () : $_ } @searches;

Super ugly code. Test for scoping issues.
Comment 9 M. Tompsett 2019-06-24 16:56:52 UTC
Comment on attachment 90823 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

Review of attachment 90823 [details] [review]:
-----------------------------------------------------------------

C4/Tags fails qa test tools.
Comment 10 M. Tompsett 2019-06-24 16:57:15 UTC
Failing because of C4/Tags
Comment 11 Jonathan Druart 2019-06-24 17:08:08 UTC
The failure is:
 FAIL   C4/Tags.pm
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 195)
                forbidden pattern: tab char (line 236)
                forbidden pattern: tab char (line 305)

They existed before, not blocker.
Comment 12 Bernardo Gonzalez Kriegel 2020-02-06 12:12:44 UTC
Sorry, does not apply, conflicts in 

C4/Items.pm
C4/OAI/Sets.pm
C4/Search.pm
admin/searchengine/elasticsearch/mappings.pl

also a few more cases.

C4/OAI/Sets.pm:        if(0 == grep /^$value$/, @subfield_values) {
C4/OAI/Sets.pm:        if(0 < grep /^$value$/, @subfield_values) {
misc/translator/translate:    if ( $cmd ne 'create' and $lang and not grep( /^$lang$/, @{ $installer->{langs} } ) ) {
t/db_dependent/Acquisition.t:          unless grep ( /^$field$/, @$exp_fields );
t/db_dependent/Budgets.t:        my $is_in_new_budgets = grep /^$budget->{budget_id}$/, @new_budget_ids;
t/db_dependent/Budgets.t:        my $is_in_old_budgets = grep /^$budget->{budget_id}$/, @old_budget_ids;
t/db_dependent/Serials.t:my @arrived_missing = map { my $status = $_->{status}; ( grep { /^$status$/ } qw( 2 4 41 42 43 44 5 ) ) ? $_ : () } @serials;
t/db_dependent/Serials.t:my @others = map { my $status = $_->{status}; ( grep { /^$status$/ } qw( 2 4 41 42 43 44 5 ) ) ? () : $_ } @serials;
t/db_dependent/Serials.t:@arrived_missing = map { my $status = $_->{status}; ( grep { /^$status$/ } qw( 2 4 41 42 43 44 5 ) ) ? $_ : () } @serials;
t/db_dependent/Serials.t:@others = map { my $status = $_->{status}; ( grep { /^$status$/ } qw( 2 4 41 42 43 44 5 ) ) ? () : $_ } @serials;
Comment 13 Jonathan Druart 2020-02-11 12:06:29 UTC
Created attachment 98701 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!
Comment 14 Jonathan Druart 2020-02-11 12:06:58 UTC
Rebased.
Remaining occurrences are in tests, should not be a problem.
Comment 15 Bernardo Gonzalez Kriegel 2020-02-11 12:43:09 UTC
It applies now, no (new) qa errors.

Just a question:
In C4/TAGS.pm you replace

  scalar grep {/^ $key $/x} @ok_fields

with

  scalar grep { $_ eq $key } @ok_fields

Is this correct?
The spaces in '/^ $key $/x' are in the matching string (by way of /x), then perhaps

  scalar grep { $_ eq " $key " } @ok_fields

is preferable.
Comment 16 Jonathan Druart 2020-02-11 12:50:28 UTC
the 'x' modifier allows whitespaces, so they must be explicitly provided by \w to be taken into account. So I think we are good here.
Comment 17 Bernardo Gonzalez Kriegel 2020-02-11 13:29:43 UTC
Created attachment 98707 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Looks correct, no errors.
Comment 18 Bernardo Gonzalez Kriegel 2020-02-11 13:30:21 UTC
(In reply to Jonathan Druart from comment #16)
> the 'x' modifier allows whitespaces, so they must be explicitly provided by
> \w to be taken into account. So I think we are good here.

You are right!
Comment 19 Jonathan Druart 2020-02-14 10:56:41 UTC
Created attachment 98926 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Looks correct, no errors.
Comment 20 Katrin Fischer 2020-02-14 15:06:53 UTC
Please rebase - should be a quick one, but I messed it up :(
Comment 21 Bernardo Gonzalez Kriegel 2020-02-14 15:14:33 UTC
Created attachment 98960 [details] [review]
Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Looks correct, no errors.
Rebased.
Comment 22 Bernardo Gonzalez Kriegel 2020-02-14 15:15:11 UTC
(In reply to Katrin Fischer from comment #20)
> Please rebase - should be a quick one, but I messed it up :(

Rebased!
Comment 23 Katrin Fischer 2020-02-14 15:21:18 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #22)
> (In reply to Katrin Fischer from comment #20)
> > Please rebase - should be a quick one, but I messed it up :(
> 
> Rebased!

Thx Bernardo!
Comment 24 Katrin Fischer 2020-02-14 16:02:33 UTC
Could we keep them from creeping back in with a new addition to the QA test tools?
Comment 25 Katrin Fischer 2020-02-14 16:08:53 UTC
Created attachment 98968 [details] [review]
j# Attachment to Bug 23084 - Replace grep {^$var$} with grep {$var eq $_}

Bug 23084: Replace grep {^$var$} with grep {$_ eq $var}

We certainly faced 3 similar bugs due to this syntax: bug 23006, bug
22941 and bug 17526.

To prevent other issues related to this syntax this patch suggests to
replace them all in one go.

Test plan:
Confirm that the 2 syntaxes are similar
Eyeball the patch and confirm that there is no typo!

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Looks correct, no errors.
Rebased.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Martin Renvoize 2020-02-17 10:46:03 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Jonathan Druart 2020-02-17 11:11:54 UTC
(In reply to Katrin Fischer from comment #24)
> Could we keep them from creeping back in with a new addition to the QA test
> tools?

I did not manage to end up with something that would not raise too many false positives.
Comment 28 Katrin Fischer 2020-02-17 12:24:52 UTC
(In reply to Jonathan Druart from comment #27)
> (In reply to Katrin Fischer from comment #24)
> > Could we keep them from creeping back in with a new addition to the QA test
> > tools?
> 
> I did not manage to end up with something that would not raise too many
> false positives.

Thx for checking Jonathan. Wonder if this is important enough for a note in the coding guidelines? Our case with a 100 libraries using $ in their barcode might be a bit specific... but I also think the new form of writing this is nicer and more readable.
Comment 29 Joy Nelson 2020-03-05 01:27:14 UTC
does not apply to 19.11.x.  not backporting