Bug 25898 - Prohibit indirect object notation
Summary: Prohibit indirect object notation
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P1 - high blocker (vote)
Assignee: Julian Maurice
QA Contact: Katrin Fischer
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on:
Blocks: 26303
  Show dependency treegraph
 
Reported: 2020-06-30 08:04 UTC by Julian Maurice
Modified: 2020-11-13 15:43 UTC (History)
5 users (show)

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


Attachments
Bug 25898: Prohibit indirect object notation (266.95 KB, patch)
2020-06-30 08:06 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 25898: Prohibit indirect object notation (265.75 KB, patch)
2020-10-07 18:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25898: (follow-up) POD (1.23 KB, patch)
2020-10-07 18:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25898: Prohibit indirect object notation (267.36 KB, patch)
2020-10-14 10:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25898: (follow-up) POD (1.28 KB, patch)
2020-10-14 10:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25898: Fix new occurrences (2.38 KB, patch)
2020-10-15 11:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25898: fix Parallel::ForkManager call in debian/list-deps (916 bytes, patch)
2020-10-26 17:23 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2020-06-30 08:04:51 UTC
Indirect object notation is when you write:

  my $cgi = new CGI;

Recently announced Perl 7 (https://www.perl.com/article/announcing-perl-7/) will disable this notation by default.
We should write instead:

  my $cgi = CGI->new

Perl 7 does not have a release date yet, but it's better to be prepared :)

This bug is for adding a rule in .perlcriticrc and removing all indirect object notations.
Comment 1 Julian Maurice 2020-06-30 08:06:17 UTC
Created attachment 106423 [details] [review]
Bug 25898: Prohibit indirect object notation
Comment 2 Katrin Fischer 2020-06-30 17:08:15 UTC
Would it make sense to create a Perl 7 Omnibus bug we can use for these?
Comment 3 David Cook 2020-07-01 02:57:09 UTC
This is great to see, although also impossible to test?

I guess if we can get it into master ASAP though then there's a better chance of any issues being caught early in this release cycle.
Comment 4 Julian Maurice 2020-07-01 07:16:59 UTC
(In reply to Katrin Fischer from comment #2)
> Would it make sense to create a Perl 7 Omnibus bug we can use for these?

If I understood correctly there would be only 2 bugs under that omnibus : this one, and one for removing bareword filehandle. I'll let you decide if it makes sense to have an omnibus ;)

(In reply to David Cook from comment #3)
> This is great to see, although also impossible to test?

I agree. The only reasonable way to "test" would be to carefully review the patch change by change. If it can help, I did it with `git show --word-diff=color --word-diff-regex=.` (It takes time, prepare some coffee/tea/mate first)
Comment 5 David Cook 2020-08-26 23:03:41 UTC
(In reply to Katrin Fischer from comment #2)
> Would it make sense to create a Perl 7 Omnibus bug we can use for these?

Jonathan has opened one at Bug 26303
Comment 6 Tomás Cohen Arazi 2020-10-07 18:25:05 UTC
Created attachment 111351 [details] [review]
Bug 25898: Prohibit indirect object notation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2020-10-07 18:25:13 UTC
Created attachment 111352 [details] [review]
Bug 25898: (follow-up) POD

Our docs shouldn't suggest indirect object notation is accepted or
encouraged.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Katrin Fischer 2020-10-14 10:45:38 UTC
There are some POD and whitespace fails here, but I think they can be safely ignored for a patch like this one:

 FAIL 	misc/batchImportMARCWithBiblionumbers.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 81)
 FAIL	cataloguing/additem.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 168)
   OK	  git manipulation

BTW: Running the QA test tools on this one takes foreeeever.
Comment 9 Katrin Fischer 2020-10-14 10:54:01 UTC
C4/ILSDI/Services.pm:    my $cgi = new CGI;
C4/InstallAuth.pm:  my $query = new CGI;
C4/SIP/ILS.pm:  #               $item = new ILS::Item $i;
C4/SIP/ILS.pod:    my $ils = new ILS (institution => 'Foo Public Library');
C4/SIP/ILS.pod:    $ils = new ILS $institution
C4/SIP/ILS/Item.pod:    my $item = new ILS::Item $item_id;
C4/Utils/DataTables.pm:    my $input = new CGI;
admin/background_jobs.pl:my $input             = new CGI;
debian/list-deps:my $pm   = new Parallel::ForkManager( Sys::CPU::cpu_count() );

For some of those I am not sure - I suggest to deal with them in a follow-up and of course we will also need a check in the QA script!
Comment 10 Katrin Fischer 2020-10-14 10:57:33 UTC
Created attachment 111588 [details] [review]
Bug 25898: Prohibit indirect object notation

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2020-10-14 10:57:39 UTC
Created attachment 111589 [details] [review]
Bug 25898: (follow-up) POD

Our docs shouldn't suggest indirect object notation is accepted or
encouraged.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Jonathan Druart 2020-10-15 10:55:17 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 13 Jonathan Druart 2020-10-15 11:57:22 UTC
Created attachment 111714 [details] [review]
Bug 25898: Fix new occurrences
Comment 14 Jonathan Druart 2020-10-15 12:33:02 UTC
Added to the perlcriticrc used by the QA script

https://gitlab.com/koha-community/qa-test-tools/-/issues/41
Comment 15 Mason James 2020-10-26 17:23:56 UTC
Created attachment 112525 [details] [review]
Bug 25898: fix Parallel::ForkManager call in debian/list-deps

fix Parallel::ForkManager call in debian/list-deps
Comment 16 Jonathan Druart 2020-11-02 07:21:46 UTC
(In reply to Mason James from comment #15)
> Created attachment 112525 [details] [review] [review]
> Bug 25898: fix Parallel::ForkManager call in debian/list-deps
> 
> fix Parallel::ForkManager call in debian/list-deps

Patch pushed to master (by Tomas on Oct 26).
Comment 17 Lucas Gass 2020-11-13 15:43:50 UTC
does not apply cleanly to 20.05.x, no backport