Bug 20000 - use Modern::Perl in installer perl scripts
Summary: use Modern::Perl in installer perl scripts
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords: Academy
Depends on:
Blocks: 2505
  Show dependency treegraph
 
Reported: 2018-01-17 04:46 UTC by Aleisha Amohia
Modified: 2019-05-27 12:18 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Code cleanup which improves the readability, and therefore reliability, of Koha.
Version(s) released in:


Attachments
Bug 20000: Changing 'use strict: use warnings;' to 'use Modern::Perl;' (4.47 KB, patch)
2018-01-17 22:49 UTC, Zoe Bennett
Details | Diff | Splinter Review
Bug 20000: Fixing problems with the change to 'use strict: use warnings;' to 'use Modern::Perl;' in the follwing installer script (895 bytes, patch)
2018-01-18 20:38 UTC, Zoe Bennett
Details | Diff | Splinter Review
Bug 20000: Fixing problems with the change to 'use strict: use warnings;' to 'use Modern::Perl;' in the following installer scripts (1.47 KB, patch)
2018-01-18 20:43 UTC, Zoe Bennett
Details | Diff | Splinter Review
Bug 20000: Changing 'use strict: use warnings;' to 'use Modern::Perl;' (4.52 KB, patch)
2018-01-23 14:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20000: use Modern::Perl in installer perl scripts (1.56 KB, patch)
2018-01-23 14:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20000: use Modern::Perl in installer perl scripts (4.79 KB, patch)
2018-02-04 13:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20000: Removing Modern::Perl from old scripts (1.29 KB, patch)
2018-03-01 00:09 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 20000: Use Modern::Perl in installer scripts (4.50 KB, patch)
2018-12-19 20:36 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 20000: Use Modern::Perl in installer scripts (4.55 KB, patch)
2019-01-04 22:11 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 20000: Use Modern::Perl in installer scripts (4.66 KB, patch)
2019-02-02 20:39 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 Aleisha Amohia 2018-01-17 04:46:33 UTC
use Modern::Perl should be used instead of use strict; use warnings;

Relevant in:
data/mysql/backfill_statistics.pl
data/mysql/labels_upgrade.pl
data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl
data/mysql/updatedatabase.pl
externalmodules.pl
html-template-to-template-toolkit.pl
install.pl
Comment 1 Zoe Bennett 2018-01-17 22:49:20 UTC
Created attachment 70648 [details] [review]
Bug 20000: Changing 'use strict: use warnings;' to 'use Modern::Perl;'

Test Plan:
-  Check that it now says 'use Modern::Perl' and not 'use strict; use
warnings;' in the following installer perl scripts:

data/mysql/backfill_statistics.pl
data/mysql/labels_upgrade.pl
data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl
data/mysql/updatedatabase.pl
externalmodules.pl
html-template-to-template-toolkit.pl
install.pl
Comment 2 M. Tompsett 2018-01-17 23:04:46 UTC
Comment on attachment 70648 [details] [review]
Bug 20000: Changing 'use strict: use warnings;' to 'use Modern::Perl;'

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

::: installer/data/mysql/labels_upgrade.pl
@@ +17,4 @@
>  # You should have received a copy of the GNU General Public License
>  # along with Koha; if not, see <http://www.gnu.org/licenses>.
>  
> +use Modern::Perl;

Commented out with '# FIXME - Bug 2505' would make more sense.

@@ -17,5 @@
>  # You should have received a copy of the GNU General Public License
>  # along with Koha; if not, see <http://www.gnu.org/licenses>.
>  
> -#use strict;
> -#use warnings; FIXME - Bug 2505

These are commented out.

::: installer/data/mysql/patroncards_upgrade.pl
@@ +17,4 @@
>  # You should have received a copy of the GNU General Public License
>  # along with Koha; if not, see <http://www.gnu.org/licenses>.
>  
> +use Modern::Perl;

not commented?

@@ -17,5 @@
>  # You should have received a copy of the GNU General Public License
>  # along with Koha; if not, see <http://www.gnu.org/licenses>.
>  
> -#use strict;
> -#use warnings; FIXME - Bug 2505

commented.

::: installer/data/mysql/update22to30.pl
@@ -11,5 @@
>  # - Would also be a good idea to offer to do a backup at this time...
>  
>  # NOTE:  If you do something more than once in here, make it table driven.
> -use strict;
> -#use warnings; FIXME - Bug 2505

This is a strange case.
Do we have anyone really using pre-3.0 still? I thought this script might be pointless by now.
Comment 3 Marcel de Rooy 2018-01-18 11:17:06 UTC
Looks like this is a Failed QA
Comment 4 Zoe Bennett 2018-01-18 20:38:59 UTC
Created attachment 70710 [details] [review]
Bug 20000: Fixing problems with the change to 'use strict: use warnings;' to 'use Modern::Perl;' in the follwing installer script

data/mysql/labels_upgrade.pl
Comment 5 Zoe Bennett 2018-01-18 20:43:04 UTC
Created attachment 70711 [details] [review]
Bug 20000: Fixing problems with the change to 'use strict: use warnings;' to 'use Modern::Perl;' in the following installer scripts

data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl
Comment 6 Owen Leonard 2018-01-23 14:24:07 UTC
Created attachment 70840 [details] [review]
Bug 20000: Changing 'use strict: use warnings;' to 'use Modern::Perl;'

Test Plan:
-  Check that it now says 'use Modern::Perl' and not 'use strict; use
warnings;' in the following installer perl scripts:

data/mysql/backfill_statistics.pl
data/mysql/labels_upgrade.pl
data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl
data/mysql/updatedatabase.pl
externalmodules.pl
html-template-to-template-toolkit.pl
install.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Owen Leonard 2018-01-23 14:24:10 UTC
Created attachment 70841 [details] [review]
Bug 20000: use Modern::Perl in installer perl scripts

Fixing problems with the change to 'use strict: use warnings;' to 'use
Modern::Perl;' in the following installer scripts

data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Katrin Fischer 2018-02-04 13:40:41 UTC
Created attachment 71191 [details] [review]
Bug 20000: use Modern::Perl in installer perl scripts

Test Plan:
-  Check that it now says 'use Modern::Perl' and not 'use strict; use
warnings;' in the following installer perl scripts:

data/mysql/backfill_statistics.pl
data/mysql/labels_upgrade.pl
data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl
data/mysql/updatedatabase.pl
externalmodules.pl
html-template-to-template-toolkit.pl
install.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2018-02-04 13:41:19 UTC
Squashed patches as both had the same author and were signed by Owen.
Comment 10 Jonathan Druart 2018-02-05 12:33:48 UTC
The patch adds use strict for scripts that were used in a very old upgrade (3.01). I do not think it is a good idea to update them (or spend time to improve them).
Comment 11 Aleisha Amohia 2018-03-01 00:09:14 UTC
Created attachment 72297 [details] [review]
Bug 20000: Removing Modern::Perl from old scripts

as per Jonathan's comment 10

Sponsored-by: Catalyst IT
Comment 12 M. Tompsett 2018-03-27 12:58:56 UTC
Comment on attachment 71191 [details] [review]
Bug 20000: use Modern::Perl in installer perl scripts

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

::: installer/data/mysql/update22to30.pl
@@ +11,5 @@
>  # - Would also be a good idea to offer to do a backup at this time...
>  
>  # NOTE:  If you do something more than once in here, make it table driven.
> +
> +use Modern::Perl;

The warnings is commented. I don't think this substitution is necessarily good.
Comment 13 Katrin Fischer 2018-05-15 23:58:18 UTC
installer/data/mysql/update22to30.pl
Mark, do you mean we should remove the use Modern::Perl from this old script?

I'd like to unlock this one, as it's the last blocking bug 2505.
Comment 14 Jonathan Druart 2018-05-16 14:13:25 UTC
We should not touch these scripts unless they have been tested with the changes (which is quite a lot of work for the gain it will bring).

Ok for install.pl
Comment 15 Aleisha Amohia 2018-12-18 22:15:37 UTC
(In reply to Jonathan Druart from comment #14)
> We should not touch these scripts unless they have been tested with the
> changes (which is quite a lot of work for the gain it will bring).
> 
> Ok for install.pl

Do you mean the only file in this set we should use Modern::Perl in is install.pl?
Comment 16 Jonathan Druart 2018-12-19 12:56:38 UTC
(In reply to Aleisha Amohia from comment #15)
> (In reply to Jonathan Druart from comment #14)
> > We should not touch these scripts unless they have been tested with the
> > changes (which is quite a lot of work for the gain it will bring).
> > 
> > Ok for install.pl
> 
> Do you mean the only file in this set we should use Modern::Perl in is
> install.pl?

Re-reading the patch I'd say we can use Modern::Perl when strict *and* warnings were used before.
For others remove the FIXME and the commented lines.
Comment 17 Aleisha Amohia 2018-12-19 20:36:10 UTC
Created attachment 83402 [details] [review]
Bug 20000: Use Modern::Perl in installer scripts

To test:

Check that we are using Modern::Perl when strict *and* warnings were used before,
and commented lines and FIXMEs have been removed from the rest, out of
files listed below.

data/mysql/backfill_statistics.pl
data/mysql/labels_upgrade.pl
data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl
data/mysql/updatedatabase.pl
externalmodules.pl
html-template-to-template-toolkit.pl
install.pl

Sponsored-by: Catalyst IT
Comment 18 Maryse Simard 2019-01-04 22:11:50 UTC
Created attachment 83672 [details] [review]
Bug 20000: Use Modern::Perl in installer scripts

To test:

Check that we are using Modern::Perl when strict *and* warnings were used before,
and commented lines and FIXMEs have been removed from the rest, out of
files listed below.

data/mysql/backfill_statistics.pl
data/mysql/labels_upgrade.pl
data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl
data/mysql/updatedatabase.pl
externalmodules.pl
html-template-to-template-toolkit.pl
install.pl

Sponsored-by: Catalyst IT
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 19 Katrin Fischer 2019-02-02 20:39:21 UTC
Created attachment 84671 [details] [review]
Bug 20000: Use Modern::Perl in installer scripts

To test:

Check that we are using Modern::Perl when strict *and* warnings were used before,
and commented lines and FIXMEs have been removed from the rest, out of
files listed below.

data/mysql/backfill_statistics.pl
data/mysql/labels_upgrade.pl
data/mysql/patroncards_upgrade.pl
data/mysql/update22to30.pl
data/mysql/updatedatabase.pl
externalmodules.pl
html-template-to-template-toolkit.pl
install.pl

Sponsored-by: Catalyst IT
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Nick Clemens 2019-02-04 17:43:29 UTC
Awesome work all!

Pushed to master for 19.05
Comment 21 Martin Renvoize 2019-02-11 13:51:37 UTC
Nice cleanup here, backporting to 18.11.x for 18.11.03