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
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 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.
Looks like this is a Failed QA
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
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
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>
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>
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>
Squashed patches as both had the same author and were signed by Owen.
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).
Created attachment 72297 [details] [review] Bug 20000: Removing Modern::Perl from old scripts as per Jonathan's comment 10 Sponsored-by: Catalyst IT
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.
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.
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
(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?
(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.
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
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>
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>
Awesome work all! Pushed to master for 19.05
Nice cleanup here, backporting to 18.11.x for 18.11.03