Bug 22026 - Remove `use Modern::Perl` from Koha::REST::classes
Summary: Remove `use Modern::Perl` from Koha::REST::classes
Status: RESOLVED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Jasmine Amohia
QA Contact: Tomás Cohen Arazi
URL:
Whiteboard:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2018-12-19 07:53 UTC by Martin Renvoize
Modified: 2019-03-04 00:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22026: Removed 'use Modern::Perl;' from Koha::REST::classes (7.02 KB, patch)
2019-01-17 02:58 UTC, Jasmine Amohia
Details | Diff | Splinter Review
Bug 22026: Removed 'use Modern::Perl;' from Koha::REST::classes (7.07 KB, patch)
2019-01-18 10:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 22026: Removed 'use Modern::Perl;' from Koha::REST::classes (7.12 KB, patch)
2019-01-18 10:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2018-12-19 07:53:33 UTC
The REST api is based on Mojolicious and as such the controllers within this namespace are all based upon `Mojo::Base`, `use Mojo::Base 'Mojolicious::Controller';` to be specific.

Mojo::Base enables the following, which are the only useful things achieved by Modern::Perl in our case (as we limit ourselves to supporting perl 5.10, so any and all other features past perl 5.10 are unavailable to use anyway).

```
use strict;
use warnings;
use utf8;
use feature ':5.10';
use mro;
use IO::Handle ();
require SomeBaseClass;
push @ISA, 'SomeBaseClass';
sub has { Mojo::Base::attr(__PACKAGE__, @_) }
```

As such, adding the `use Modern::Perl` statement is just noise and needlessly loads a module.
Comment 1 Jonathan Druart 2018-12-19 18:07:10 UTC
Modern::Perl will be loaded by other packages anyway :)
Comment 2 Jasmine Amohia 2019-01-17 02:58:03 UTC
Created attachment 84119 [details] [review]
Bug 22026: Removed 'use Modern::Perl;' from Koha::REST::classes

Test plan:
1) Check all Koha/REST files and confirm that 'use Modern::Perl;' has
been removed.
Comment 3 David Nind 2019-01-18 10:26:06 UTC
Created attachment 84179 [details] [review]
Bug 22026: Removed 'use Modern::Perl;' from Koha::REST::classes

Test plan:
1) Check all Koha/REST files and confirm that 'use Modern::Perl;' has
been removed.

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Tomás Cohen Arazi 2019-01-18 10:58:22 UTC
Created attachment 84189 [details] [review]
Bug 22026: Removed 'use Modern::Perl;' from Koha::REST::classes

Test plan:
1) Check all Koha/REST files and confirm that 'use Modern::Perl;' has
been removed.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Julian Maurice 2019-01-18 11:11:00 UTC
https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL2:_Modern::Perl should probably be updated to mention this special case
Comment 6 Nick Clemens 2019-01-28 16:11:37 UTC
Awesome work all!

Pushed to master for 19.05
Comment 7 Martin Renvoize 2019-02-01 14:42:16 UTC
This cleanup isn't required for 18.11.x.
Comment 8 Jonathan Druart 2019-02-14 22:07:27 UTC
I am not sure this was a good move, compare what it brings with the changes we need to make.

Anyway, now we need to update the coding guidelines and update our t/perlcriticrc (I think).

Also Jenkins is failing because of that move, see https://jenkins.koha-community.org/job/Koha_Master_D8/163/
I do not understand why it is only failing for D8.
Comment 9 Jonathan Druart 2019-02-19 13:15:26 UTC
(In reply to Jonathan Druart from comment #8)
> Also Jenkins is failing because of that move, see
> https://jenkins.koha-community.org/job/Koha_Master_D8/163/
> I do not understand why it is only failing for D8.


% more /etc/debian_version 
9.6
% perlcritic --version
1.126
% perlcritic Koha/REST/V1.pm
Koha/REST/V1.pm source OK

% more /etc/debian_version
8.11
% perlcritic --version
1.122
% perlcritic Koha/REST/V1.pm
Code before strictures are enabled at line 37, column 1.  See page 429 of PBP.  (Severity: 5)
Comment 10 Jonathan Druart 2019-02-19 13:24:26 UTC
I guess it is:

https://github.com/Perl-Critic/Perl-Critic/blob/dev/Changes

1.124 2015-02-27

    * The RequireUseStrict and RequireUseWarnings policies now regard Moose,
      Moo, Mouse, Dancer, Mojolicious, and several other modules as equivalent
to the strict and warnings pragma.
Comment 11 Martin Renvoize 2019-02-26 15:59:03 UTC
Ack, as this would require use packaging a more recent version of perlcritic I don't feel it's a big enough win to warrant the cleanup.. I'm inclined to agree with Jonathan that this point and suggest we revert it until a point in the future.
Comment 12 Nick Clemens 2019-02-26 19:10:28 UTC
Reverted from master