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.
Created attachment 106423 [details] [review] Bug 25898: Prohibit indirect object notation
Would it make sense to create a Perl 7 Omnibus bug we can use for these?
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.
(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)
(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
Created attachment 111351 [details] [review] Bug 25898: Prohibit indirect object notation Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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.
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!
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>
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>
Pushed to master for 20.11, thanks to everybody involved!
Created attachment 111714 [details] [review] Bug 25898: Fix new occurrences
Added to the perlcriticrc used by the QA script https://gitlab.com/koha-community/qa-test-tools/-/issues/41
Created attachment 112525 [details] [review] Bug 25898: fix Parallel::ForkManager call in debian/list-deps fix Parallel::ForkManager call in debian/list-deps
(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).
does not apply cleanly to 20.05.x, no backport